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: Correctly set and pick up environment variables #315

Merged
merged 13 commits into from
Mar 28, 2023

Conversation

azhouwd
Copy link
Member

@azhouwd azhouwd commented Mar 23, 2023

Issue #, if available:
Currently the environment variable is set in /root/.bashrc so that it is not easily accessible by regular users even by specifying sudo -E. Normally we would access the VM by regular user. In order to pick up the environment variables, we need to store the vars in the /home/.linux/.bashrc and added -E to sudo command as sudo itself won't preserve the environment variables.

Description of changes:
Added -E to sudo command when we call limactl command.
Update the profile path so that env vars are written in /home/.linux/.bashrc.
Added e2e tests to make sure the content is written correctly into the ~/.finch/.config.json file in host system.
Testing done:
Manual testing and E2E tests

  • 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.

@ningziwen
Copy link
Member

ningziwen commented Mar 23, 2023

Could you add e2e tests? Can be simply "logout -> in host, ~/.finch/config.json.assertNotContains() -> login -> in host, ~/.finch/config.json.assertContains()", unless you know better ways.

Can be directly added in finch repo instead of common-tests because this is finch specific.

@azhouwd azhouwd changed the title Correctly set and pick up environment variables fix: Correctly set and pick up environment variables Mar 23, 2023
azhouwd added 4 commits March 23, 2023 16:25
Signed-off-by: Ang Zhou <[email protected]>
Signed-off-by: Ang Zhou <[email protected]>
Signed-off-by: Ang Zhou <[email protected]>
@azhouwd azhouwd marked this pull request as ready for review March 24, 2023 17:26
@azhouwd
Copy link
Member Author

azhouwd commented Mar 24, 2023

Could you add e2e tests? Can be simply "logout -> in host, ~/.finch/config.json.assertNotContains() -> login -> in host, ~/.finch/config.json.assertContains()", unless you know better ways.

Can be directly added in finch repo instead of common-tests because this is finch specific.

Added e2e tests.

@azhouwd azhouwd requested review from ningziwen and vsiravar March 24, 2023 17:26
ningziwen
ningziwen previously approved these changes Mar 24, 2023
Signed-off-by: Ang Zhou <[email protected]>
Copy link
Contributor

@vsiravar vsiravar left a comment

Choose a reason for hiding this comment

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

We should add sudo -E in the version command as well to keep it consistent with the rest of nerdctl commands.

@azhouwd
Copy link
Member Author

azhouwd commented Mar 24, 2023

We should add sudo -E in the version command as well to keep it consistent with the rest of nerdctl commands.

Good catch, updated

@azhouwd azhouwd requested a review from vsiravar March 24, 2023 19:54
ningziwen
ningziwen previously approved these changes Mar 24, 2023
Copy link
Contributor

@vsiravar vsiravar left a comment

Choose a reason for hiding this comment

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

Looks good. golanci-lint failed on version_test.go due to to max line length. We should fix that before merge.

ningziwen
ningziwen previously approved these changes Mar 24, 2023
@vsiravar vsiravar self-requested a review March 25, 2023 00:38
vsiravar
vsiravar previously approved these changes Mar 25, 2023
Copy link
Contributor

@vsiravar vsiravar left a comment

Choose a reason for hiding this comment

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

LGTM

@vsiravar
Copy link
Contributor

vsiravar commented Mar 27, 2023

The newly added E2E tests are failing consistent on arm64, I haven't figured out the reasons. Maybe we can add them in separate PR to unblock the bug fix? @ningziwen @vsiravar

@azhouwd Did you run into this in local make test-e2e?

No the whole suite works fine locally, I tried several times

Does the test work if you execute it manually on the CI runners by logging in? What if we put a sleep after

"-dp", fmt.Sprintf("%d:5000", port),
since the registry is running in detached mode and might take a while to come up? WDYT

@vsiravar vsiravar self-requested a review March 28, 2023 00:18
@azhouwd azhouwd dismissed stale reviews from vsiravar and ningziwen via 15f8630 March 28, 2023 03:12
@azhouwd azhouwd force-pushed the angzh/fix-config-mount branch from 7b0f62b to 15f8630 Compare March 28, 2023 03:12
@azhouwd
Copy link
Member Author

azhouwd commented Mar 28, 2023

The newly added E2E tests are failing consistent on arm64, I haven't figured out the reasons. Maybe we can add them in separate PR to unblock the bug fix? @ningziwen @vsiravar

@azhouwd Did you run into this in local make test-e2e?

No the whole suite works fine locally, I tried several times

Does the test work if you execute it manually on the CI runners by logging in? What if we put a sleep after

"-dp", fmt.Sprintf("%d:5000", port),

since the registry is running in detached mode and might take a while to come up? WDYT

yeah you are right, I printed the images and containers trying to debug, but the issue happens to be fixed..

Signed-off-by: Ang Zhou <[email protected]>
@azhouwd azhouwd requested a review from ningziwen March 28, 2023 16:39
ningziwen
ningziwen previously approved these changes Mar 28, 2023
azhouwd added 2 commits March 28, 2023 10:12
# Conflicts:
#	cmd/finch/main.go
Signed-off-by: Ang Zhou <[email protected]>
e2e/vm/finch_config_file_test.go Outdated Show resolved Hide resolved
e2e/vm/finch_config_file_test.go Outdated Show resolved Hide resolved
@azhouwd azhouwd force-pushed the angzh/fix-config-mount branch from c4b5dfe to 6300a6c Compare March 28, 2023 21:06
ningziwen
ningziwen previously approved these changes Mar 28, 2023
Signed-off-by: Ang Zhou <[email protected]>
@azhouwd
Copy link
Member Author

azhouwd commented Mar 28, 2023

Merged due to the e2e failure is a known issue with Rosetta

@azhouwd azhouwd merged commit 05227ce into runfinch:main Mar 28, 2023
@azhouwd azhouwd deleted the angzh/fix-config-mount branch March 28, 2023 22:15
sam-berning pushed a commit that referenced this pull request Mar 29, 2023
🤖 I have created a release *beep* *boop*
---


## [0.5.0](v0.4.1...v0.5.0)
(2023-03-29)


### Features

* adds support bundles
([#210](#210))
([cc6be65](cc6be65))


### Bug Fixes

* Correctly set and pick up environment variables
([#315](#315))
([05227ce](05227ce))
* print debug logs after newline
([#273](#273))
([8faa7de](8faa7de))
* print debug logs when lima disk command fails
([#270](#270))
([78a3f50](78a3f50))


### Experimental

* **feat:** enable Virtualization.framework and Rosetta
([#282](#282))
([fd3bf19](fd3bf19))


### Build System or External Dependencies

* clean up finch-core _output directory in clean target
([#290](#290))
([4684a95](4684a95))
* **deps:** bump github.com/onsi/ginkgo/v2 from 2.8.4 to 2.9.0
([#265](#265))
([7e2d49e](7e2d49e))
* **deps:** bump github.com/onsi/ginkgo/v2 from 2.9.0 to 2.9.1
([#285](#285))
([d741a03](d741a03))
* **deps:** Bump github.com/onsi/gomega from 1.27.3 to 1.27.5
([#312](#312))
([e078234](e078234))
* **deps:** bump github.com/runfinch/common-tests from 0.6.1 to 0.6.2
([#300](#300))
([dd626a0](dd626a0))
* **deps:** bump github.com/spf13/afero from 1.9.4 to 1.9.5
([#263](#263))
([a0e277f](a0e277f))
* **deps:** bump golang.org/x/crypto from 0.6.0 to 0.7.0
([#264](#264))
([ec1c07f](ec1c07f))
* **deps:** bump golang.org/x/tools from 0.6.0 to 0.7.0
([#268](#268))
([8072e39](8072e39))
* **deps:** bump k8s.io/apimachinery from 0.26.2 to 0.26.3
([#306](#306))
([fe392cb](fe392cb))
* **deps:** Bump lima version
([#302](#302))
([0269743](0269743))
* **deps:** Bump submodules
([#281](#281))
([d4fd1f6](d4fd1f6))
* **deps:** Bump submodules
([#304](#304))
([b38af9f](b38af9f))
* **deps:** Bump submodules
([#307](#307))
([1a914ec](1a914ec))
* **deps:** Bump submodules
([#319](#319))
([e671224](e671224))

---
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>
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.

3 participants