-
Notifications
You must be signed in to change notification settings - Fork 130
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 oldest allowed merge base to resolvers #7692
Conversation
@@ -18,6 +18,7 @@ input RepoRefInput { | |||
gitTagVersionsEnabled: Boolean | |||
manualPrTestingEnabled: Boolean | |||
notifyOnBuildFailure: Boolean | |||
oldestAllowedMergeBase: String |
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 intend to show this field for repos? (I guess the question I'm asking is, is it valid to set this field on a repo?)
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.
Yes
@@ -203,6 +203,11 @@ For security reasons, commits by users outside of your organization will | |||
not automatically be run. A patch will still be created and must be | |||
manually authorized to run by a logged-in user. | |||
|
|||
#### Limiting when PR patches will run |
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't check the ticket description / comments rn since Jira is being chaotic... but one thing I can think of with implementing the feature this way is that the oldest allowed merge base will get outdated very easily (sorry I missed the PR where the functionality was actually introduced). Do we want this to be a number of days instead, so it stays up to date, or is it meant more to prevent specific bad commits from being included kind of? (Fine with whatever, just wondering if we considered that)
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 field won't get outdated since its purpose is to reject any PRs that come in that have a merge base before the "oldest allowed merge base". From the ticket description and the ask, it seems setting an explicit SHA to not allow merge bases before is the desired behavior.
#### Limiting when PR patches will run | ||
You can optionally specify the oldest commit SHA that is allowed to be a merge base | ||
for a PR, otherwise a PR patch will not be created. To do so, input the oldest SHA | ||
on your project's branch you want to accept as the merge base via the 'Oldest Allowed Merge Base' field. |
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 you plan to update the Spruce settings yourself shortly, or are you planning to ask a UI engineer to do this? I think this should be relatively simple to add, and merge these together. Otherwise, I don't think we should document this until the feature is available.
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.
Sounds good, I created a matching Spruce PR here
DEVPROD-1008
Description
This adds the new field to our resolvers so it can be set via Spruce.
Testing
Unit test.
Documentation
Added doc section for the feature