fix: link release checklist in release PR workflow#3802
fix: link release checklist in release PR workflow#3802hiSandog wants to merge 2 commits intofaker-js:nextfrom
Conversation
✅ Deploy Preview for fakerjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #3802 +/- ##
==========================================
- Coverage 98.89% 98.83% -0.07%
==========================================
Files 894 894
Lines 3087 3086 -1
Branches 564 564
==========================================
- Hits 3053 3050 -3
- Misses 30 32 +2
Partials 4 4 🚀 New features to boost your workflow:
|
|
Cool idea. How does this behave if the release checklist issue does not exist yet? |
|
Good question! If the release checklist issue does not exist yet, In practice, the checklist issue is created as part of the release prep process (it is an issue labeled |
|
Should we cancel/fail the process instead? |
xDivisionByZerox
left a comment
There was a problem hiding this comment.
Should we cancel/fail the process instead?
@faker-js/maintainers What do you think?
Usually I would be in favor of failing the PR creation, because not having a release issue would mean that someone is fundamentally going against our internal release process.
But since that would require reverting all other steps as well, which would increase the complexity of the workflow file more that we gain from the change, I'd say that we simply default to the old text inside the jq expression (see https://github.com/faker-js/faker/pull/3802/changes#r3069395214).
By doing that, the instantiation of the PR_BODY variable should also be more straight forward, since RELEASE_CHECKLIST_URL should not be empty anymore.
Does this change evaluate to incorrect issue numbers if we ever forget to close an old release issue? I don't think that this is worth addressing if it does, since the chance of this happening is very slim. I'm just curious if it would. I guess it wouldn't due to the --limit 1 and/or the default sort order being "newest first" 🤔
|
|
||
| - name: Create draft PR | ||
| run: | | ||
| RELEASE_CHECKLIST_URL="$(gh issue list --label 'c: release' --state open --limit 1 --json url --jq '.[0].url // ""')" |
There was a problem hiding this comment.
This is where I would default to the old text.
| RELEASE_CHECKLIST_URL="$(gh issue list --label 'c: release' --state open --limit 1 --json url --jq '.[0].url // ""')" | |
| RELEASE_CHECKLIST_URL="$(gh issue list --label 'c: release' --state open --limit 1 --json url --jq '.[0].url // "TODO add link to issue"')" |
|
@xDivisionByZerox Good call! Applied your suggestion — the jq expression now defaults to |
| - [ ] Completed manual changes/tasks for this release | ||
|
|
||
| --- | ||
| --- | ||
|
|
||
| - Checklist: TODO add link to issue | ||
| " | ||
| - Checklist: $RELEASE_CHECKLIST_URL" |
There was a problem hiding this comment.
Please retain the original indentation here, as it gets reflected in the final PR description.
| - [ ] Completed manual changes/tasks for this release | |
| --- | |
| --- | |
| - Checklist: TODO add link to issue | |
| " | |
| - Checklist: $RELEASE_CHECKLIST_URL" | |
| - [ ] Completed manual changes/tasks for this release | |
| --- | |
| - Checklist: $RELEASE_CHECKLIST_URL | |
| " |
Summary
Validation