-
Notifications
You must be signed in to change notification settings - Fork 694
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
Modify workflow trigger to push on main #1251
Conversation
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.
It looks like this badge is broken because we don't run the workflow on a push to main, only on PRs. I don't think the badge should show a fail if we get a bad PR. Instead we should either remove the badge, or (my preference) update the workflow to run on a push to main. E.g.
on:
push:
branches: [ 'main' ]
tags: [ 'v*.*.*' ]
pull_request:
branches: [ 'main' ]
Updated the badge to display the main branch, modified workflow trigger as suggested 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.
Overall LGTM, please squash the commits and I'll add my approval.
Signed-off-by: Scott Brenner <[email protected]>
push: | ||
branches: [ 'main' ] | ||
tags: [ 'v*.*.*' ] | ||
pull_request: | ||
branches_ignore: [] | ||
branches: [ 'main' ] |
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.
Is this heavy enough to be worth gating? Why not simply let it run on all pushes and all PRs?
push: | |
branches: [ 'main' ] | |
tags: [ 'v*.*.*' ] | |
pull_request: | |
branches_ignore: [] | |
branches: [ 'main' ] | |
push: | |
pull_request: |
(this is perhaps more of a question for @sudo-bmitch than @ScottBrenner, to be clear ❤️)
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.
The downside of running on all pushes and PRs is if a maintainer pushes directly to a branch rather than their own fork, you end up with multiple runs of the same job. I'm not opposed to running on all PRs, but I think that's solving a problem we don't have since we don't have feature branches in the spec today, and I don't think many people are reviewing the generated artifacts anyway.
Implementing #1251 (review)