-
Notifications
You must be signed in to change notification settings - Fork 36
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 run-in-container wrapper script and associated instructions #275
base: master
Are you sure you want to change the base?
Conversation
ce5370a
to
b8217bd
Compare
run-in-container.sh
Outdated
@@ -14,5 +14,6 @@ podman run --rm --privileged \ | |||
-e GIT_COMMITTER_NAME="$GIT_USER_NAME" \ | |||
-e GIT_AUTHOR_EMAIL="$GIT_USER_EMAIL" \ | |||
-e GIT_COMMITTER_EMAIL="$GIT_USER_EMAIL" \ | |||
--entrypoint=python3 \ |
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.
Doesn't this mean run-in-container.sh
won't run f-e-d-c anymore, unless its script is manually passed on cli?
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.
You're right, I somehow missed that run-in-container.sh
was also used for just running f-e-d-c. I've changed it so it selects python3
as the entrypoint if python3
happens to be the first argument. It works for both running f-e-d-c normally and running the CONTRIBUTING.md
tests.
The container image was adapted for local use in 064fdea, which means the default entrypoint is f-e-d-c itself. However, test suites from CONTRIBUTING.md don't run in the container, since they need to be run from python3 normally. So fix by selectively setting entrypoint to python3 in the run-in-container wrapper script. Black was removed since it is run by CI and no longer present in the image, and should be easy to install in a Toolbox or elsewhere.
b8217bd
to
d4a2c2a
Compare
I have been using this for my important source PR, it works as intended, even if a bit hacky |
Honestly, I'm not sure why we need this wrapper script at all. Running fedc with |
I am not attached to this script. |
@wjt I suppose Endless doesn't need it on its CI anymore (IIRC it did)? |
No, all that is long gone. |
Stumbled back upon this. At this point I think the wrapper script is actively misleading and I guess we should kill it and update the documentation. But I did find its old usage useful for local development… |
The container image was adapted for local use in 064fdea, which means the default entrypoint is f-e-d-c itself.
However, test suites from CONTRIBUTING.md don't run, since they need to be run from python3 normally. So fix by setting entrypoint to python3 in the run-in-container wrapper script.
Black was removed since it is run by CI and no longer present in the image, and should be easy to install in a Toolbox or elsewhere.