-
Notifications
You must be signed in to change notification settings - Fork 2k
Replace obsolete ENV VAR VALUE with ENV VAR=VALUE #2254
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix update.sh & functions.sh
Fixed. This PR now depends on: The failing test is because https://news.ycombinator.com is down. I reported it, hope they'll fix it soon. |
It's up again. All should pass now. |
Why should this one depend on #2258?
|
Because they touch the same line in update.sh. |
That's something should be prevented. I don't want to mix different topic in the same pull request.
|
I agree. So which one of them do you want to accept first? You can either merge the other one and then I'll rebase this one, or I can swap them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove unrelated commit changes that are not relevant to the topic.
Maybe this one as it was sent earlier. |
Done |
Fixes these warnings: - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the legacy Dockerfile ENV KEY VALUE
syntax with the recommended ENV KEY=VALUE
format across scripts, templates, versioned Dockerfiles, and documentation to resolve linter warnings.
- Updated parsing and emission in helper scripts to handle
KEY=VALUE
- Mass-updated Dockerfile templates and versioned Dockerfiles to use
ENV KEY=VALUE
- Updated examples in documentation to match new syntax
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
update.sh | Adjusted sed patterns to match and emit ENV KEY=VALUE |
stackbrew.js | Updated regex to parse ENV NODE_VERSION= |
genMatrix.js | Updated regex to extract ENV NODE_VERSION= |
functions.sh | Updated version extraction to use ENV NODE_VERSION= |
docs/BestPractices.md | Updated Dockerfile examples to use ENV KEY=VALUE |
README.md | Updated example to use ENV KEY=VALUE |
Dockerfile-slim.template, Dockerfile-debian.template, Dockerfile-alpine.template |
Replaced ENV KEY VALUE with ENV KEY=VALUE in templates |
20/, 22/, 24/* Dockerfiles | Mass replace ENV KEY VALUE with ENV KEY=VALUE across versions |
Comments suppressed due to low confidence (1)
update.sh:136
- [nitpick] Consider adding unit or integration tests for the
update_node_version
function to verify that the new sed patterns correctly handle theKEY=VALUE
syntax and update Dockerfiles as expected.
sed -Ei -e 's/^(ENV NODE_VERSION)=.*/\1='"${nodeVersion}"'/' "${dockerfile}-tmp"
@@ -239,7 +239,7 @@ function get_full_version() { | |||
default_dockerfile="${version}/Dockerfile" | |||
fi | |||
|
|||
grep -m1 'ENV NODE_VERSION ' "${default_dockerfile}" | cut -d' ' -f3 | |||
sed -n 's/^ENV NODE_VERSION=//p' "${default_dockerfile}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sed command may print multiple matches since it's not limited to the first occurrence like the original grep -m1; consider adding a quit (q) directive or a similar mechanism to stop after the first match and preserve intended behavior.
sed -n 's/^ENV NODE_VERSION=//p' "${default_dockerfile}" | |
sed -n 's/^ENV NODE_VERSION=//p;q' "${default_dockerfile}" |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s avoid making unnecessary changes in this section.
|
||
currentYarnVersion="$(grep "ENV YARN_VERSION" "${dockerfile}" | cut -d' ' -f3)" | ||
sed -Ei -e 's/^(ENV YARN_VERSION ).*/\1'"${currentYarnVersion}"'/' "${dockerfile}-tmp" | ||
currentYarnVersion="$(sed -n "s/^ENV YARN_VERSION=//p" "${dockerfile}")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the approach of using grep and cut to sed here is an unnecessary modification.
@@ -174,9 +174,9 @@ function update_node_version() { | |||
else | |||
if [ "${SKIP}" = true ]; then | |||
# Get the currently used Yarn version | |||
yarnVersion="$(grep "ENV YARN_VERSION" "${dockerfile}" | cut -d' ' -f3)" | |||
yarnVersion="$(sed -n "s/^ENV YARN_VERSION=//p" "${dockerfile}")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Description
Fixes these warnings:
Testing Details
Build passed.
Types of changes
Checklist