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: add stricter check for DOCKER_CONFIG var #127

Closed
wants to merge 1 commit into from

Conversation

pendo324
Copy link
Member

Signed-off-by: Justin Alvarez [email protected]

Issue #, if available: Closes #126

Description of changes:

  • Previously, this code was just checking for any substring containing export DOCKER_CONFIG, now we check for a line starting with the fully configured DOCKER_CONFIG variable (like export DOCKER_CONFIG="/Users/justin/.finch")

Testing done:

  • added unit test for this scenario

  • I want to test this practically, but its difficult to change the user on my mac and keep the same VM. Thinking of ways to test before merging

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

@pendo324 pendo324 added the bug Something isn't working label Dec 20, 2022
@pendo324 pendo324 self-assigned this Dec 20, 2022
@ollypom
Copy link
Contributor

ollypom commented Dec 22, 2022

@pendo324 I was wondering if it would simplify things if the DOCKER_CONFIG variable was passed in as a Env variable in the lima config? Similar to #129

@pendo324
Copy link
Member Author

pendo324 commented Jan 5, 2023

@pendo324 I was wondering if it would simplify things if the DOCKER_CONFIG variable was passed in as a Env variable in the lima config? Similar to #129

Sorry for the late response @ollypom, I was out for the holidays and I'm still catching up on things. I believe I tried this before but wasn't able to get it working because of the fact that the DOCKER_CONFIG is dependent on the $USER (since it's set to /Users/$USER/.finch).

Some of this complication is due to the fact that the Lima env map adds to /etc/environment, which can't evaluate variables, by definition.

It's definitely possible that I overlooked something though, open to changing this behavior as its kinda hacky the way it is currently. The other advantage of the way it is right now is that its dynamically evaluated on vm init, which means it will get updated in the rare case that the $USER changes (which is pretty hard to test actually, which is the reason I haven't tried to merge this yet).

@pendo324
Copy link
Member Author

Deduping for all strings added to bashrc was already added by a separate PR (see here), closing

@pendo324 pendo324 closed this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

finch can not login
2 participants