-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
refactor: GITHUB_ENV command / remove env.PATH #1503
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
2f9d59c
to
65b339a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1503 +/- ##
==========================================
+ Coverage 61.22% 61.91% +0.68%
==========================================
Files 46 46
Lines 7141 7246 +105
==========================================
+ Hits 4372 4486 +114
+ Misses 2462 2457 -5
+ Partials 307 303 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
I got it working, inclusive tests for custom PATH in docker actions etc.
err := rc.JobContainer.UpdateFromImageEnv(step.getEnv())(ctx) | ||
if err != nil { | ||
return err | ||
} |
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.
This add PATH
to the env object, but in actions/runner env.PATH
is empty
- run: | | ||
FROM ubuntu:latest | ||
ENV PATH="/opt/texlive/texdir/bin/x86_64-linuxmusl:${PATH}" | ||
ENV ORG_PATH="${PATH}" |
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.
ORG_PATH is a copy of the expected PATH value, this enshures PATH is unset in env ctx for docker actions
runs-on: ubuntu-latest | ||
container: | ||
image: node:16-buster-slim | ||
options: --user node |
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 previous non root user image has PATH issues with node, due to intentional changes.
@ChristopherHX this pull request is now in conflict 😩 |
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.
Looks good.
@catthehacker provided some details about the act images I didn't know, so this PR should be blocked for now to not break users. This PR removes also reading other envs than PATH from /etc/environment, so this break everything for these images. |
I opened a PR to the act docker images repo catthehacker/docker_images#83 to resolve the issues caused by this PR. I'm not shure how to enshure that docker images are semi uptodate, because this is a breaking change for the docker_images repo.
|
What do you mean? |
I mean act doesn't pull images by default. Some may end up creating an issue about missing node in PATH. |
Change so it pulls by default 😛 or add feature that fetches latest meta about used image and warn user when image is outdated/doesn't match |
See #1585 for a test
I added more tests and fixed an issue, which leaked the step env of the composite actions. You can now define env variables with the same name as a env var in the step env as new global variables, tests has been added. I hope this PR doesn't cause new issues and I think this is ready now. |
No, only coverage upload failed with a weird error. |
I'm potentially breaking non standard act specfic workflows
With this change are all file commands in a single place again.
Closes #1384
Closes #1421