-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add GitHub tagging and release creation automation using GitHub and Azure Pipelines APIs #40
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.
Did an initial review and everything looks great!
I didn't find anything to comment, so I will let it sink in and tomorrow I'll give it another shot 😄
cmd/git-go-patch/apply.go
Outdated
return err | ||
} | ||
} | ||
goDir := filepath.Join(rootDir, "go") |
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.
The extract,
applyand
rebase` commands have the following pattern:
rootDir, err := findOuterRepoRoot()
if err != nil {
return err
}
goDir := filepath.Join(rootDir, "go")
Is it worth factoring that 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.
I can see rootDir, goDir, err := findProjectRoots()
working out.
I'd probably want to rename to projectDir, submoduleDir, err := findProjectRoots()
to be more forward-looking, because I think we'll be generalizing the patching tools beyond our forks. The findProjectRoots
function will then let us migrate off the hard-coded go
subdir in one place (finding/reading a config file instead).
These lines are only touched by this PR to change the indentation, but it would be pretty hard to mess up a refactor like this, so I might as well while it's in mind.
cmd/git-go-patch/extract.go
Outdated
@@ -43,145 +56,138 @@ Each command starts with "` + commandPrefix + `", then: | |||
distinct groups of patches. Making the first patch in each group start at a consistent number can | |||
help to avoid unnecessary filename conflicts when porting changes between branches. | |||
` + repoRootSearchDescription | |||
} | |||
func (e extractCmd) Handle(p subcmd.ParseFunc) error { |
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.
One thought I had reading through this is there is a lot of IO happening and when it fails we resort to
return err
I'm considering how that will look when we have an error in an AzDO run. It will result in a failure that has an IO message in the logs but it seems like it will be really hard to track that back down to what failed. Basically if we have a "Access Denied" error in the logs how will we tell which of the various IO paths in this code was responsible?
Is it worth adding logs on the failure cases so we have a trail to look at?
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.
In a lot of cases I think we can pretty easily figure out where an error happened by looking at the last positive/progress log line. But, I try to always remember to wrap errors when I think there might be some ambiguity, like return fmt.Errorf("unable to chop the carrot: %w", err)
when err
could be knife is dull
but the func is making an entire bowl of soup.
In my experience, IO errors always mention the filename, so there are a decent number of situations where even without wrapping, the error would still be obvious.
This seems pretty subjective to me, and wrapping too much is definitely better than not wrapping enough, so I'd be happy to put more in. (Just maybe not in git-go-patch
because this isn't new code, only unindented.)
file) | ||
return err | ||
}); err != nil { | ||
return fmt.Errorf("failed to upload %#q to release %v: %w", p, *release.ID, err) |
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 we fail to upload a file, we should fail the release. Does that happen? Possibly it does, just my lack of go knowledge showing :)
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, we're outside of the retry
so this will return from Handle
which will kick in the defered cleanup?
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.
Yep, err := retry(...)
gets the last inner error if it exceeds the retry count, then if err := retry(...); err != nil {
continues here, and this return is in the function's scope.
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!
Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>
ef19aa7
to
3754403
Compare
I recommend reviewing with "hide whitespace" enabled. I changed the subcommand implementation to use an interface rather than have a func in a struct, so a lot of code is unindented by one level.
Finish off post-build release process automation for microsoft/go. This gets rid of a lot of copy-pasting version numbers so the process is less tedious and error-prone.
Here's an example of the comments that the pipeline (optionally) leaves on the release issue: https://github.com/dagood/go-release-test/issues/3.
Example release/tag: https://github.com/dagood/go-release-test/releases/tag/v1.18.1-1
Pipeline run UI looks like this:
I plan to have the variable group pass through all the release steps to make it reasonable to run a fake release without worrying too much about messing up the main repos.
Individual steps are toggleable so if a build partially fails, you can run a retry that only includes the steps that still need to happen. If the steps were each idempotent, these toggles wouldn't be needed, but I didn't want to try to rig up reliable idempotency on top of the GitHub APIs--at least not yet. I think keeping it simple will work out fine.
This adds a few libraries:
I didn't switch the existing code over to use go-github yet. Note: go-github uses the GitHub v3 API, so it actually can't do auto-merges, which requires v4. But the GitHub v4 API is incomplete (it doesn't support releases), so we can't simply use a newer library that supports v4.
github.com/microsoft/go-infra/subcmd
This package makes it easier to implement subcommands. I moved the subcommand functionality from git-go-patch over here and changed it to use interfaces, which are more typical for this kind of sharing.
I changed the existing
update-aka-ms
command into thereleasego akams
subcommand, which meant some tweaks like adding the ability to handle arbitrary args by implementingOptionArgTaker
instead of onlyOption
.I wanted to use subcommands for
releasego
to give the commands a clear namespace (so subcommand names don't have to be as descriptive and can trivially share code) and so we can build a single app that gets used throughout the steps of the pipeline for simplicity.