-
Notifications
You must be signed in to change notification settings - Fork 27
Check if secrets are set #2844
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?
Check if secrets are set #2844
Conversation
If not, skip the build as the job will fail
|
This change makes it clear why PRs from forks fail to run. |
| echo "have-secrets=true" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "❌ Missing required secrets: ${missing[*]}" | ||
| echo "⚠️ PRs must be sent from branches, not forks!" |
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 is slightly contradicting. either we tell people to set the secrets (which they could on forks) or we forbid running from forks. that would be simpler to achieve:
if [ ${{github.repository }} ] != "SUSE/BCI-dockerfile-generator" ]; then
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 could run on a fork if you have the proper secrets. It is possible to share secrets with forks, but this is security issue, so a no-go IMHO.
Not everyone can create a branch to send a PR, so yes, the message does not help much.
We could skip the run instead of failing with an error, not sure what is the best user experience 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.
I don't think anyone has the right secrets (or we shouldn't advertise people putting their OSC access tokens into github, far too dangerous).
I suggest to only go with the "check if it is a fork, then don't run the jobs" approach instead. this doesn't advertise contributors to do an unsafe operation.
| obs-build: | ||
| needs: check-secrets | ||
| if: needs.check-secrets.outputs.have-secrets == 'true' |
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 think the needs alone causes it to be skipped so the if should be unnecessary? haven't tested it though.
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.
yes, the if might be removed if we dont write the file on failures perhaps, because we current write a value and read the value.
If not, skip the build as the job will fail