fix: correctly handle local environment value pass-through

[estesp]

The -e,--env flag as well as values within an --env-file may contain only a variable name with no "={VALUE}" portion. These entries provide a shorthand to say "pass the existing environment value of this variable into the container". Because of the VM boundary we need to extrapolate these values on the Finch side and pass them as discrete values into the Lima VM on the nerdctl command line. Also the file referenced by --env-file may not be accessible inside the VM, so we translate each entry in the file into -e entries on the command line rather than fail on the VM side, unable to locate the file being referenced.

Signed-off-by: Phil Estes [email protected]

Issue #, if available: Fixes #35

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

[QuinnyPig]

Confirmed that this works (once #154 is cherry-picked into the PR branch) in my testing.

[estesp]

Rebased on main to make it easier for testing since this commit was between the persistent disk feature and the fix for unique persistent disk naming.

[ningziwen]

The tests can be added here:

Unit tests

e2e tests

[ningziwen]

The existing e2e tests expect the local env value is missing in the result. https://github.com/runfinch/common-tests/blame/main/tests/run.go#L130

So this may be a feat instead of fix as this is an expected unimplemented feature based on e2e tests, assuming e2e tests can be the workaround to check if one behaviour is expected given the situation where we don't have unimplement feature documentation.

[estesp]

The existing e2e tests expect the local env value is missing in the result. https://github.com/runfinch/common-tests/blame/main/tests/run.go#L130

That test passes even though the core nerdctl problem (of -eVAR being unimplemented) was fixed since Finch was launched. It is almost an inadvertent pass, though, because that fix (since nerdctl v1.1.0) was not carried into the Finch side because of the VM boundary issue. To align with nerdctl function, we should have fixed the same bug in Finch when we included v1.1.0.

Of course, the other test framework issue is that to really test the exact behavior you need -eVAR1 where VAR1 is unset in the environment and -eVAR2 where VAR2 is set in the environment. I will update this PR with a modification in the test to validate both variations as that is the correct behavior from both Docker, and now nerdctl, since v1.1.0.

So this may be a feat instead of fix as this is an expected unimplemented feature based on e2e tests, assuming e2e tests can be the workaround to check if one behaviour is expected given the situation where we don't have unimplement feature documentation.

This seems like an expectation that the e2e tests are based on concrete list of known functionality (versus just how nerdctl worked when the test was written). I'm OK if we would rather call this a feature, but I don't think the e2e tests are the defacto standard on specific capabilities that were inadvertently broken at the time.

[ningziwen]

Released the new version of common-tests. You can bump the version of common-tests and use it in this PR. https://github.com/runfinch/common-tests/releases

[estesp]

Updated common-tests; ran locally to verify e2e tests:

Ran 274 of 274 Specs in 610.032 seconds
SUCCESS! -- 274 Passed | 0 Failed | 0 Pending | 0 Skipped

[ningziwen]

@estesp Thanks. Could you please also add unit tests? #158 (comment)

make coverage can be used to self-validate unit test coverage.

[estesp]

@ningziwen added unit tests; testing env-file gets a bit messy as we need an input file to get coverage over the file reading/parsing code blocks. Please advise if you want to use another way other than tmpfile creation. Also had to break it out of the test table as the flag name won't be known until the tmpfile is created.