-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add build workflow (fix issue #35) #37
Conversation
694c01f
to
31555da
Compare
@ivanperez-keera - please take a look. This and space-ros/space-ros#200 give the expected results. If you agree, I'll remove the temporary test commit from this (the "Delete this commit before merging") and merge this into the repo. |
Seems good to me. Just to make it clear, the check for the docker secrets does not in itself constitute any kind of protection: people could modify the We still want to be careful with any PRs sent against the |
31555da
to
20489ad
Compare
226c20f
to
8834ea8
Compare
I removed the test commit and updated the README with instructions, @ivanperez-keera and @EzraBrooks please approve and merge |
.github/workflows/docker-image.yml
Outdated
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v3 | ||
- name: Run the build.sh scripts | ||
env: | ||
DOCKER_HUB_TOKEN: ${{ secrets.DOCKER_HUB_RW_TOKEN }} | ||
run: | | ||
if [ -n "$DOCKER_HUB_TOKEN" ] | ||
then | ||
echo "Secrets detected, can't run build script" | ||
exit 1 # terminate and indicate error | ||
else | ||
# find the build.sh scripts and execute them | ||
find . -iname 'build.sh' -exec ./{} \; | ||
fi |
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'm not sure the check is necessary; GitHub doesn't forward secrets into CI jobs dispatched from forks.
So, the only case in which the DOCKER_HUB_TOKEN would be populated is if one of us maintainers pushes a branch directly to this repo.
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.
OK, I just tested a PR from a fork into space-ros/space-ros and it looks like you're correct. I'll remove the secret check if we don't need it
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.
Well, the check itself won't matter, because the PR could alter the GA workflow.
What we wanted was to check that it was also not making the secret available to other repos.
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.
What we wanted was to check that it was also not making the secret available to other repos.
which isn't possible anyway AFAIK. The only workflow type that is allowed to do this is pull_request_target
, which we should be sure to never approve someone adding to the repo.
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.
So, you can turn the step into:
- name: Run the build.sh scripts
run: |
find . -iname 'build.sh' -exec ./{} \;
```
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 started to do that but then I realized that doesn't fail if the build.sh
fails. find
only fails if it fails to traverse a tree. I want to fail if any build.sh
fails. Right now I've tried a few variations and can't seem to find one that works correctly, in both the passing and failing case
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.
Probably need to change from find -exec
to find | xargs
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.
oh, interesting, GNU find (AKA what you have in Ubuntu) apparently has a -quit
argument?
929481f
to
b527b9d
Compare
Updated to call scripts directly, this works, please approve & merge |
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.
ah, wait, we need an issue reference in the commits here
b527b9d
to
19ce14c
Compare
Good catch, updated commit message and PR name to include issue #35 |
19ce14c
to
a1e83cf
Compare
Checks to make sure no secrets are exposed, then searches for and runs any build script found