-
Notifications
You must be signed in to change notification settings - Fork 578
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
Adds initial release automator #633
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Nice start. One comment for a typo, otherwise lgtm. Have you considered a simple bash script instead of a Go file? I don't mind either way, more curious if you have a plan here.
cmd/release/main.go
Outdated
) | ||
|
||
/* | ||
required parameteres |
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.
required parameteres | |
required parameters |
cmd/release/main.go
Outdated
const ( | ||
// TODO figure this out based on directory name | ||
repository = "cluster-api-provider-aws" | ||
artifactsDir = "out" |
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.
These should probably be flags at some point
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.
💯
@vincepri I'm really hoping to avoid this https://github.com/kubernetes/release/blob/master/anago with regards to go vs bash. There is nothing wrong with that bash script, in fact it's one of the best bash scripts/libraries I've ever seen, but I find it harder to maintain a bash script than a go script. I do expect there to be more than just The other piece is Integrating this with a configuration language. As you pointed out, flags/config would be really helpful here. It will be a lot easier to ingest and reference values in a config file via go than bash. The goal here (eventually) would be that if we have a nice configuration file for providers to fill out, they get release tooling for free |
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 outside of the question
cmd/release/main.go
Outdated
|
||
// create draft release | ||
// TODO: not everyone names the remotes after the username | ||
RunCommand("gothub", "release", "--tag", version, "--user", remote, "--repo", repository, "--draft") |
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.
Would overriding the upstream for the working branch (using git branch --set-upstream-to
affect this? For example I override the upstream for my master branch to upstream/master
?
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.
Nope, that would be fine. Since this is a github specific release tool, it will assume the remote as "github.com//" regardless of local git settings. So I think you're pointing at a misconception that I had when writing this -- it actually is GitHub user and my variable name is bad. Will fix.
77eae78
to
a9ce6bc
Compare
cmd/release/main.go
Outdated
RunCommand("make", "release-artifacts") | ||
|
||
// Creates git tag | ||
RunCommand("git", "tag", "--force", 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.
I would advise against --force
. If you need to wipe & recreate a tag, delete it manually first.
I would also recommend either doing an annotated tag or a GPG signed tag.
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.
ah yes, this sounds great, thanks for the feedback
/cc |
cmd/release/main.go
Outdated
// TODO(chuckha): it would be ideal if we could release major/minor/patch and have it | ||
// automatically bump the latest tag git finds | ||
// until then, hard code it | ||
remote := os.Args[1] // default to origin probably |
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.
How would you feel about these being flags instead of positional arguments?
I'm curious (with no strong feelings necessarily either way) why did you choose to write this in Go instead of bash or make or python or whatever? |
|
Thanks, SGTM. I'd recommend thinking about how you want to unit test this tool, if applicable. To me, that's one major advantage to writing something like this in NotBash |
Testing yeah, it's definitely an advantage. Standard DI testing is the way to go here. That will both make the code better and let use more easily get rid of the gothub dependency. Looks like flags got requested twice, so that is enough signal that people really think flags should be in the initial release. I'll update this PR |
lgtm for the initial take |
/hold for updates |
/hold for updates* |
cmd/release/main.go
Outdated
|
||
// attach tarball of yaml and binaries for systems to the release | ||
for _, file := range expectedFiles { | ||
RunCommand("gothub", "upload", "--tag", version, "--user", user, "--repo", repository, "--file", path.Join(artifactsDir, file), "--name", file) |
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.
What are the failure modes for this?
Should we perform any cleanup should this fail, before allowing a retry?
cmd/release/main.go
Outdated
|
||
// Build all the release binaries | ||
RunCommand("make", "release-artifacts") | ||
|
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.
suggest adding a validate artifacts step to confirm all expected files are present.
cmd/release/main.go
Outdated
version := os.Args[3] | ||
|
||
// Build all the release binaries | ||
RunCommand("make", "release-artifacts") |
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.
RunCommand("make", "release-artifacts") | |
RunCommand("make", "clean", "release-artifacts") |
cmd/release/main.go
Outdated
} | ||
} | ||
|
||
func mustSatisfyDependencies() { |
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.
nit: suggest rename ensureDependencies
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.
Can we include documentation on usage as well in the docs/releasing.md
, that includes any gotchas and hardcoded parameters?
I'll plan to copy this into capz and use it for our releases as well, but I want to make I do so in the way that it was intended.
/milestone v1alpha1 |
@justaugustus I don't think it's ready for that. We'll start using it for this repo and make it fit other uses cases as we go forward. |
/hold cancel |
Signed-off-by: Chuck Ha <[email protected]>
docs/releasing.md
Outdated
@@ -1,17 +1,25 @@ | |||
# Release process | |||
|
|||
## Semi-automatic | |||
|
|||
1. make the release artifacts `make release artifacts` |
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.
1. make the release artifacts `make release artifacts` | |
1. make the release artifacts `make clean release-artifacts` |
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.
what's the benefit of adding clean? release-artifacts are a PHONY target and will always overwrite whatever exists.
Clean will simply clean up the dev environment a user may already have
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.
clean
will delete existing examples prior to running release-artifacts
target. This will ensure that the release will have newly generated examples in it.
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.
Oh.. I see. the release-artifacts
doesn't call other make targets. so clean
not required.
5. Attach the tarball to the drafted release | ||
6. Attach `clusterawsadm` and `clusterctl` to the drafted release (for darwin | ||
2. Tag the repository and push the tag `git tag -s $VERSION ` | ||
3. Run `make release-artifacts` |
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.
3. Run `make release-artifacts` | |
3. Run `make clean release-artifacts` |
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
Bumped golang to 1.20.2
Signed-off-by: Chuck Ha [email protected]
What this PR does / why we need it:
This PR adds a basic release tool to help us get started with automated releases. The idea is to run it to create a draft release that we edit and publish by hand when we're satisfied with the release notes.
Here is an example run:
And this is what the generated release will look like:
There is opportunity to improve this script greatly and push it up to cluster-api to help with the releases there.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #122
Special notes for your reviewer:
Has a documented dependency on github.com/itchio/gothub that could be removed by using GitHub API. This was installed and used for expediency.
install with
go get github.com/itchio/gothub
Release note:
No user facing changes