-
Notifications
You must be signed in to change notification settings - Fork 9.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
[RFC] release: use GitHub's gh to create GitHub release #18649
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
[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 |
@ahrtr, @jmhbnz, @serathius, PTAL :) |
c498282
to
9ea3c41
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted filessee 23 files with indirect coverage changes @@ Coverage Diff @@
## main #18649 +/- ##
==========================================
+ Coverage 68.77% 68.82% +0.05%
==========================================
Files 420 420
Lines 35535 35535
==========================================
+ Hits 24438 24456 +18
+ Misses 9668 9649 -19
- Partials 1429 1430 +1 Continue to review full report in Codecov by Sentry.
|
Signed-off-by: Ivan Valdes <[email protected]>
9ea3c41
to
2662ac0
Compare
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.
Great work @ivanvc - A few questions but this is an awesome start.
local gh_version | ||
gh_version=$(gh --version | head -n1 | awk 'match($0,/([0-9]+\.){2}[0-9]+/) {print substr($0,RSTART,RLENGTH)}') | ||
if [ "$(cut -d. -f1 <(echo "${gh_version}"))" -lt "2" ] || [ "$(cut -d. -f2 <(echo "${gh_version}"))" -lt "57" ]; then | ||
log_error "The minimum version required for gh is 2.57.0, got ${gh_version}" |
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.
Curious - Why do we have a dependency on this version? Is it purely from a standpoint of what we have tested?
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 was having issues with gh auth status
's exit code (the exit code was 0 even when it failed). I swear I saw a pull request/a release note for this behavior in 2.57.0 (which is also the latest version). But I just tested again with multiple versions and went back all the way to 2.37.0, and it works as expected.
So, I'm fine removing this check.
log_error "The minimum version required for gh is 2.57.0, got ${gh_version}" | ||
exit 1 | ||
fi | ||
if [ "${DRY_RUN}" != "true" ] && ! gh auth status &>/dev/null; then |
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 we validate gh auth status
irrespective of DRY_RUN
? If I was using DRY_RUN=true
I would still want to know that I had a problem with gh
.
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 avoided the check because it would fail when running on the CI. One option would be to add a new execution flag (i.e., --no-github-release
) so we can set that in the release test and still keep it green. 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.
Makes sense, having a new flag we can use in CI sounds good to me.
maybe_run gh release create "${RELEASE_VERSION}" \ | ||
--repo "${gh_repo}" \ | ||
--draft \ | ||
--prerelease \ |
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.
Do we want to be marking as pre-release? I think this would depend on which branch we were creating a release for? I.e. if we were publishing an alpha/rc for 3.6.0 vs publishing a patch release for 3.5 or 3.4.
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.
For main
, it should be --prerelease
, for release-3.5
, it should be --latest
and for release-3.4
, it should not have an additional option.
I'm open to documenting it or adding a branch check. Let me know your thoughts.
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 think we need to have some conditional logic in the script to provide the appropriate parameter based on which branch we are performing release steps for. Otherwise we would need to edit the instructions to make sure we update draft release manually?
docker exec etcd-gcr-${ETCD_VER}/usr/local/bin/etcdctl endpoint health | ||
docker exec etcd-gcr-${ETCD_VER}/usr/local/bin/etcdctl put foo bar | ||
docker exec etcd-gcr-${ETCD_VER}/usr/local/bin/etcdctl get foo | ||
``` |
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.
Here is the markdown for a recent release if you wanted to diff it:
release.txt
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.
Thanks, it looks the same. I guess, maybe it would be a good idea to update the second paragraph (at least in the two latest release [3.5.16 and 3.4.34]) to the following (to address the broken links):
For installation guides, please check out [play.etcd.io](http://play.etcd.io) and [operating etcd](https://etcd.io/docs/v3.6/op-guide). Latest support status for common architectures and operating systems can be found at [supported platforms](https://github.com/etcd-io/website/blob/main/content/en/docs/v3.5/op-guide/supported-platform.md).
Co-authored-by: James Blair <[email protected]>
@@ -0,0 +1,91 @@ | |||
Please check out [CHANGELOG](https://github.com/etcd-io/etcd/blob/main/CHANGELOG/CHANGELOG-3.5.md) for a full list of changes. And make sure to read [upgrade guide](https://github.com/etcd-io/website/blob/main/content/en/docs/v3.6/upgrades/upgrade_3_6.md) before upgrading etcd (there may be breaking changes). |
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.
Note; we wanted to simplify the release notes. #15185
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 agree with that issue/improvement, but I'd like to avoid scope creeping this pull request and implementing it by only creating the release with gh
. I think it would be a good idea to create an umbrella issue for the improvements we want to make to the release process. I'll note this and create it when I close some of the tasks I currently have open.
Implements #18604.
While we verify that this works as expected, it will create the release as a draft. Then, we can verify that it looks correct and manually publish them.
I tested this feature in my fork. Refer to the latest release (and its assets).
I introduced a
scripts/release_notes.tpl.txt
. Because I don't have access to edit a previous release and copy the Markdown, I manually crafted the Markdown based on what I saw and improved how we reference the version for the Docker instructions. But, because it was manual, I may have an error.While working on importing the release notes, I realized that we had been copy-pasting the release text with a broken link.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.