-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
release: add prompt to draft GitHub release #18895
Conversation
Signed-off-by: Ivan Valdes <ivan@vald.es>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ivanvc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 23 files with indirect coverage changes @@ Coverage Diff @@
## main #18895 +/- ##
==========================================
- Coverage 68.78% 68.74% -0.04%
==========================================
Files 420 420
Lines 35558 35558
==========================================
- Hits 24457 24446 -11
- Misses 9671 9685 +14
+ Partials 1430 1427 -3 Continue to review full report in Codecov by Sentry.
|
@ivanvc: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
@@ -337,6 +337,9 @@ main() { | |||
log_warning "WARNING: If not running on DRY_MODE, please do the GitHub release manually." | |||
log_warning "" | |||
else | |||
read -p "Create GitHub draft release in ${REPOSITORY} [y/N]? " -r confirm | |||
[[ "${confirm,,}" == "y" ]] || 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.
Should the release script continue to execute when users input 'N'? It just skips creating github draft release?
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 copied the code/model from previous prompts. It's also the last step, so a failure exit here prevents nothing from later steps.
I'm okay changing it not to exit. Let me know what you think.
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.
A user's valid input should NOT lead to release failure.
Then why add this prompt? In what cases we may select "Y" or "N"?
I think we should
- either do not add this prompt question at all; which means we always create draft github release; [this option is better?]
- or continue to execute if users input "N"; and provide a warning
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.
That's a good point. I added this to be consistent with other steps in the release script. I'm fine dropping this pull request. Or with your latest suggested option. @jmhbnz, WDYT?
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'm dropping this pull request and keeping the current behavior. Please chime in if you think otherwise.
Closed to keep the current behavior. Refer to #18895 (comment). |
This is a cherry-pick from #18886. This pull request can be merged soon without adding the automated step to undraft/release the GitHub release.
We identified the potential improvement while doing the 3.5.17 / 3.4.35 releases.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.