-
Notifications
You must be signed in to change notification settings - Fork 61.8k
Improve docs to be more explanatory #1256
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
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@avinal 👋
Would you be able to change this to the original
chmod +x entrypoint.sh
version? The feedback I've received is that this approach adds another layer to the container, which would affect pull performance.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.
Yeah, this adds another layer to the container. But as much I have used Dockerized GitHub Action, the script will not run without permission, this is also evident from the issue comment #974 (comment) for which this PR was submitted. You may want to see into that matter again.
I can suggest an improvement to this, we can combine this line with other commands using
&&
such that it gets executed without an extra layer.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.
@avinal - sorry about the delay here 🙇 It looks like the approach in the existing documentation is to run
chmod +x entrypoint.sh
separately: (step 2 here) https://docs.github.com/en/free-pro-team@latest/actions/creating-actions/creating-a-docker-container-action#writing-the-action-code. Was this step not working for you?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.
@martin389 Sorry for the delay, In the step two it is mentioned that run this command to make your file executable, but it is not mentioned that where you are supposed to run
chmod +x entrypoint.sh
, is it our system or inside docker? And that is the point of whole confusion. Secondly I am unsure about if the file will remain executable inside docker too.Secondly I had to add this line before running the entrypoint.sh file(maybe because I didn't already made them executable, no body does !!) Please give me 1 hour and I will test the exact steps described in the docs.
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.
Thanks @avinal - the intention is for
chmod +x entrypoint.sh
to run on the user's system, and not within Docker. I agree that this is something that could be clearer 👍I tested these steps yesterday on macOS and the script remained executable, but I'd be interested to know if it works for you too. Thanks for all your help here! 🎊
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.
Here are the test cases I did (updated)
chmod +x entrypoint.sh
- YES(expected) only thing is to keep in mind is that this command must be run after the file has been written, the other way around didn't workSo if you allow I would like to add an updated PR including this small change and other changes
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.
That would be great, thank you! 🎊