-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Include directory
when serializing dependencies
#10002
Include directory
when serializing dependencies
#10002
Conversation
When considering whether a pull request matches an existing pull request, we should consider whether the directories match.
|
||
sig { returns(T::Boolean) } | ||
def should_consider_directory? | ||
grouped_update? && Dependabot::Experiments.enabled?("dependency_has_directory") |
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 know this is targeted to fix a grouping bug, but any reason not to do it consistently for any kind of update instead of just grouped updates?
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 my testing, the logic relying on this method would eagerly result in closing existing/creating new pull requests. Focusing on grouped updates seemed like a lighter touch with a lower likelihood of generating unwanted churn. It'd be great if we could get to a place of setting directory
reliably so that we can consider all types of changes 😄
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 besides the question
Can we do this before merging using dry-run/the CLI and attach the results from before/after to this PR? |
Sharing the current status of the output. In its current form, this does not prevent creating individual PRs when a grouped update exists. I'm going to dismiss the approval and seek assistance in figuring out how to prevent creating individual PRs.
|
Manual testing shows my changes didn't solve the problem
The Updater doesn't yet consider the directory when calculating ungrouped dependencies, so I'm not surprised to see the individual PRs still getting created here. I think we should rename this PR to "Serialized dependencies have directory" or something of that sort since that's what it's really doing. Serializing the |
directory
when serializing dependencies
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 we should rename this PR to "Serialized dependencies have directory" or something like that before merging, but the changes LGTM.
In a follow-up PR, we will use this serialized directory when calculating the ungrouped dependencies
I'm probably having some kind of character validation issue with what dependabot thinks is or isn't a directory. My problem is that I have directories containing @landongrindheim Since you may have used/changed some related Ruby code, do you have any insight to help me find where exactly the dependabot code does the character validation used in directory names? |
@luzfcb That, and other configuration validation, is happening in the Dependabot service. The service's code is not open sourced. |
What are you trying to accomplish?
Currently, when Dependabot attempts to do a multi-directory update, it doesn't consider the directory of the dependencies in existing pull requests. This results in ungrouped pull requests being created for the dependencies which are already in a grouped pull request. By considering the directory of existing dependencies, we can avoid this issue.
Anything you want to highlight for special attention from reviewers?
How will you know you've accomplished your goal?
I've created a (private) repository which is impacted by the existing behavior. Once this pull request is merged, I'll re-run Dependabot on that repository and confirm that the grouped updates are correctly grouped and single-dependency pull requests are not created.
Checklist