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(dockerfile) manage ENV legacy case without an equal sign #503

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

dduportal
Copy link
Contributor

@dduportal dduportal commented Feb 2, 2022

Fix #390

This PR add the support for ENV Dockerfile instruction without equal sign (which is considered "legacy" and not recommended by Docker as per https://docs.docker.com/engine/reference/builder/#env)

⚠️ Alternative syntax
The ENV instruction also allows an alternative syntax ENV , omitting the =. For example:

   ENV MY_VAR my-value

This syntax does not allow for multiple environment-variables to be set in a single ENV instruction, and can be confusing. For example, the following sets a single environment variable (ONE) with value "TWO= THREE=world":

   ENV ONE TWO= THREE=world

The alternative syntax is supported for backward compatibility, but discouraged for the reasons outlined above, and may be removed in a future release.

The behavior in these cases is to rewrite with the new "recommended" syntax which inserts an equal sign.

For instance in the reproduction case from #390:

updateReleaseInDockerfile
-------------------------
**Dry Run enabled**

🐋 On (Docker)file ".tmp/Dockerfile":

⚠ The line #3, matched by the keyword "ENV" and the matcher "WINDOWS_UPDATE_VERSION",
is changed from "ENV WINDOWS_UPDATE_VERSION 0.10.1" to "ENV WINDOWS_UPDATE_VERSION=v0.14.0".

Test

To test this pull request, you can run the following commands:

make test-short
go build -o ./dist/updatecli && ./dist/updatecli diff --config examples/updatecli.generic/dockerfile/
make test-e2e

Additional Information

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal dduportal added bug Something isn't working resource-dockerfile Resource of kind Dockerfile labels Feb 2, 2022
@dduportal dduportal marked this pull request as ready for review February 2, 2022 10:11
@lemeurherve
Copy link
Member

(out of scope?) I would add a test with more than one variable defined by ENV on a same line.

@dduportal
Copy link
Contributor Author

(out of scope?) I would add a test with more than one variable defined by ENV on a same line.

Yep, that would be a separated feature request :)

But I would not (personnaly, don't know for others) spend time on such feature request as even if supported by Dockerfile, multiline ENV are really a bad pattern and make no sense since buildkit is out of the box (the only reason to group ENV in a multiline is to shrink the layer that it was generating before Docker 19.03).

However, if you feel like it's a good idea, would you be willing to send a PR that adds this test + show a warning to end user to remind that it is not supported by updatecli (with a if contains ` at the end after trimming or something like this)?

@dduportal dduportal merged commit 1768064 into updatecli:main Feb 2, 2022
@dduportal dduportal deleted the gh390 branch February 2, 2022 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resource-dockerfile Resource of kind Dockerfile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

target ENV for Dockerfile is adding the new value instead of replacing the old one
2 participants