-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Require sha when creating snapshot releases on forks #11794
Conversation
|
size-limit report 📦
|
- name: Fail job | ||
if: steps.parse-sha.outcome == 'failure' | ||
run: | | ||
exit 1 |
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's the difference between these two exit 1
?
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.
Oops, I forgot to copy a critical line from my testing. The Get sha
step should have continue-on-error: true
set, which would mean that instead of failing the entire job, it would allow it to continue. The idea was that if it failed, it would add the comment to the PR "Did you forget...", then fail early, hence this step.
The other option would be to update every step with if: steps.parse-sha-outcome != 'failure'
, but that felt like a lot and figured I'd just exit early instead. That plus the job itself would fail rather than succeed in that case so we'd get alerts that way too.
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.
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Then it looks good to me and I think we can't actually test this without merging, so LGTM? :)
@phryneas I tested this over in https://github.com/alessbell/good-snow-tire-demo/blob/main/.github/workflows/comment.yml with alessbell/good-snow-tire-demo#83 and alessbell/good-snow-tire-demo#82, so from what I can tell, we should be good 🙂 |
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.
Yep, this was tested - we paired on it last week :) LGTM 🚀
No description provided.