Skip to content
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

Merged
merged 8 commits into from
Mar 27, 2024

Conversation

hadjri
Copy link
Contributor

@hadjri hadjri commented Mar 18, 2024

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"

@hadjri hadjri requested a review from a team March 20, 2024 19:43
Copy link
Member

@malikchaya2 malikchaya2 left a 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())
Copy link
Member

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.

"message": "failed to compare commits to find merge base commit",
"op": "GetGithubMergeBaseRevision",
"message": "failed to compare commits to determine order of commits",
"op": "IsMergeBaseAllowed",
Copy link
Member

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

@hadjri hadjri merged commit aa5aed2 into evergreen-ci:main Mar 27, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants