-
Notifications
You must be signed in to change notification settings - Fork 779
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
ci: Add a workflow for creating releases #3804
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.
LGTM, I would like @straker's review of this too before we merge.
I want to test this manually before merging. Haven't had the time to do so yet, but will soon. |
npm ci | ||
npm run release | ||
|
||
git push origin "$Branch" --force |
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.
Why the force push?
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.
If the branch already has changes in it, we'll want to overwrite them.
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.
Wouldn't the checkout action fail then and kill the build before it got to this point? Not that it's preventing the pr from going through, was just curious
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.
No, the checkout action is taking whatever branch we're releasing from (usually develop
). We then create this branch, add the release commit, then push. The --force
is to ensure any prior pending release (maybe someone made a mistake) is overwritten.
This is, however, fixing a problem we haven't had (yet?). I'm happy to pull it out if you aren't comfortable force pushing.
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.
Wilco didn't seem to think it was a problem when he reviewed it, so I'm fine with it.
edit: actually turns out wilcos review did not cover this line. let me confirm with him first before merging
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 see how it matters. If the branch already exists checkout -b
will fail. I'm good with it as-is.
Co-authored-by: Steven Lambert <[email protected]>
Co-authored-by: Steven Lambert <[email protected]>
npm ci | ||
npm run release | ||
|
||
git push origin "$Branch" --force |
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.
Wilco didn't seem to think it was a problem when he reviewed it, so I'm fine with it.
edit: actually turns out wilcos review did not cover this line. let me confirm with him first before merging
npm ci | ||
npm run release | ||
|
||
git push origin "$Branch" --force |
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 see how it matters. If the branch already exists checkout -b
will fail. I'm good with it as-is.
Example release PR: #3814
Ref https://github.com/dequelabs/axe-api-team/issues/304
<< Describe the changes >>
Closes issue: