Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix to be able to run finch build with --ssh option #696

Merged
merged 2 commits into from
Nov 23, 2023
Merged

fix: Fix to be able to run finch build with --ssh option #696

merged 2 commits into from
Nov 23, 2023

Conversation

haytok
Copy link
Member

@haytok haytok commented Nov 15, 2023

In the current implementation, the following error occurs when running finch build with the --ssh default option. This issue is also reported in issue/452.

error: invalid empty ssh agent socket: make sure SSH_AUTH_SOCK is set
FATA[0000] no image was built
FATA[0000] exit status 1

This is because the ssh agent is not starting on the vm started using lima and the SSH_AUTH_SOCK variable is empty. As a result, this error occurs.

On the other hand, setting forwardAgent to true in finch.yaml will configure the vm to forward the ssh agent.

As a result, the ssh agent will be started in the vm and finch build can be executed using the --ssh default option.

Therefore, this commit will fix it so that finch build can be run using the --ssh default option.

Issue #, if available: #452

Description of changes:

Details are described in commit messages.

Testing done:

Yes, confirmation of the operation is added in the comments of the issue below.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

In the current implementation, the following error occurs when running
finch build with the `--ssh default` option. This issue is also reported
in issue/452.

  ```
  error: invalid empty ssh agent socket: make sure SSH_AUTH_SOCK is set
  FATA[0000] no image was built
  FATA[0000] exit status 1
  ```

This is because the ssh agent is not starting on the vm started using lima
and the SSH_AUTH_SOCK variable is empty. As a result, this error occurs.

On the other hand, setting forwardAgent to true in finch.yaml will
configure the vm to forward the ssh agent.

As a result, the ssh agent will be started in the vm and finch build can
be executed using the `--ssh default` option.

Therefore, this commit will fix it so that finch build can be run using
the `--ssh default` option.

Signed-off-by: Hayato Kiwata <[email protected]>
@mharwani
Copy link
Member

Hi @haytok, is it possible to add e2e tests for build with --ssh option? Thanks

@haytok
Copy link
Member Author

haytok commented Nov 16, 2023

Hi, @mharwani

Sure, add an e2e test for the --ssh option.

By the way, is it OK if I add the test scenario to the following test?

Thanks.

@mharwani
Copy link
Member

By the way, is it OK if I add the test scenario to the following test?

* https://github.com/runfinch/common-tests/blob/main/tests/build.go

Yes, that's where the tests should go. Feel free to open a PR in that repo and tag me. Thanks

haytok added a commit to haytok/common-tests that referenced this pull request Nov 17, 2023
The following pull request attempts to modify the finch build command to
allow the --ssh option.

- runfinch/finch#696

Therefore, this commit adds a test to run finch build with the --ssh
option.

Signed-off-by: Hayato Kiwata <[email protected]>
@haytok
Copy link
Member Author

haytok commented Nov 17, 2023

Hi, @mharwani

I have added an e2e test for --ssh option. When you have time, could you please review below PR ? Thanks.

mharwani pushed a commit to runfinch/common-tests that referenced this pull request Nov 21, 2023
The following pull request attempts to modify the finch build command to
allow the --ssh option.

- runfinch/finch#696

Therefore, this commit adds a test to run finch build with the --ssh
option.

Issue #, if available: N/A

*Description of changes:* Details are described in this commit message.

*Testing done:* Yes



- [x] I've reviewed the guidance in CONTRIBUTING.md


#### License Acceptance

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

Signed-off-by: Hayato Kiwata <[email protected]>
@mharwani
Copy link
Member

mharwani commented Nov 21, 2023

@haytok The e2e-test is merged. Pls update github.com/runfinch/common-tests from 0.7.8 to 0.7.9 in Go modules. Thanks!

@haytok
Copy link
Member Author

haytok commented Nov 22, 2023

Hi, @mharwani

I have updated e2e test version from v0.7.8 to v0.7.9 in go.mod and go.sum.

Could you please review when you have time?

@mharwani
Copy link
Member

You need to run go mod tidy and push again to fix the tests.

…m/runfinch/common-tests

Issue #, if available: #452

*Description of changes:*

This commit updates Go Modules by running go mod tidy so that we can test
the --ssh option in finch build.

*Testing done:* Yes

- [x] I've reviewed the guidance in CONTRIBUTING.md

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

Signed-off-by: Hayato Kiwata <[email protected]>
@haytok
Copy link
Member Author

haytok commented Nov 23, 2023

@mharwani

Thanks for the advice, I missed the go mod tidy failing and the GitHub Actions WF failing.
I've run go mod tidy and updated go.mod and go.sum, could you review when you have time?

Copy link
Member

@mharwani mharwani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@mharwani mharwani merged commit 4d1e6cf into runfinch:main Nov 23, 2023
13 checks passed
@haytok
Copy link
Member Author

haytok commented Nov 24, 2023

Thanks for merged !

@haytok haytok deleted the fix-to-finch-build-with-ssh-option branch November 24, 2023 06:40
This was referenced Dec 4, 2023
KevinLiAWS pushed a commit that referenced this pull request Dec 7, 2023
🤖 I have created a release *beep* *boop*
---


## [1.0.1](v1.0.0...v1.0.1)
(2023-12-07)


### Bug Fixes

* Clean up all previous finch version installation registries in
postinstall and uninstall
([#688](#688))
([9afc0b9](9afc0b9))
* Fix to be able to run finch build with --ssh option
([#696](#696))
([4d1e6cf](4d1e6cf))
* Fix to delete ~/.finch when uninstalling finch
([#703](#703))
([8d7389f](8d7389f))
* remove virtual machine image when running make clean
([98c8ee4](98c8ee4))
* resolve shellcheck warnings
([#684](#684))
([d9f695a](d9f695a))
* Use LimaUser method instead of host user name
([#712](#712))
([7c02e08](7c02e08))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
KevinLiAWS added a commit that referenced this pull request Dec 11, 2023
🤖 I have created a release *beep* *boop*
---


## [1.0.1](v1.0.0...v1.0.1)
(2023-12-11)


### Bug Fixes

* Change the default behavoir for deleting .finch folder to false when
uninstall ([#732](#732))
([e818743](e818743))
* Clean up all previous finch version installation registries in
postinstall and uninstall
([#688](#688))
([9afc0b9](9afc0b9))
* Fix to be able to run finch build with --ssh option
([#696](#696))
([4d1e6cf](4d1e6cf))
* Fix to delete ~/.finch when uninstalling finch
([#703](#703))
([8d7389f](8d7389f))
* remove virtual machine image when running make clean
([98c8ee4](98c8ee4))
* resolve shellcheck warnings
([#684](#684))
([d9f695a](d9f695a))
* Use LimaUser method instead of host user name
([#712](#712))
([7c02e08](7c02e08))

### Reverts

* "fix: resolve shellcheck warnings"
([#725](#725))
([8ea255a](8ea255a))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Signed-off-by: Kevin Li <[email protected]>
Co-authored-by: Kevin Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants