-
Notifications
You must be signed in to change notification settings - Fork 129
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
DEVPROD-1008 Add setting to define oldest allowed merge base for PR patches #7656
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.
Approved with two change requests.
+1 small nit: can you please remove the default docs text in the description? We should remember to update the docs once the spruce change is done. Can you please add a note to the ticket that docs should be added?
ctx, cancel := context.WithTimeout(context.Background(), time.Minute) | ||
defer cancel() | ||
baseOwnerAndRepo := strings.Split(pr.Base.Repo.GetFullName(), "/") | ||
mergeBase, err = thirdparty.GetGithubMergeBaseRevision(ctx, "", baseOwnerAndRepo[0], baseOwnerAndRepo[1], pr.Base.GetRef(), pr.Head.GetRef()) |
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.
We should check the length before accessing baseOwnerAndRepo[1] to make sure it doesn't panic.
thirdparty/github.go
Outdated
"message": "failed to compare commits to find merge base commit", | ||
"op": "GetGithubMergeBaseRevision", | ||
"message": "failed to compare commits to determine order of commits", | ||
"op": "IsMergeBaseAllowed", |
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 this should be getCommitComparison
DEVPROD-1008
Description
This introduces a new optional project ref setting field that defines the oldest allowed commit hash that can be the merge base for PR patches. If the field is set for the project and the PR patch's merge base is older than the defined commit sha, the patch will not run.
In order to make it such that the git commit comparison check API only hits for projects that have this new field set, I had to refactor the github PR intent logic a bit to move the project ref query earlier in the logic.
This will require a followup Spruce PR.
Testing
Tested in staging by setting an oldest allowed merge base on a sandbox project, and confirmed that PRs opened with merge bases before the merge base ran tasks, and a PR opened with a merge base older than the oldest allowed base failed with "merge base is too old"