Include `directory` when serializing dependencies #10002

landongrindheim · 2024-06-14T18:00:49Z

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

I have run the complete test suite to ensure all tests and linters pass.
I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
I have written clear and descriptive commit messages.
I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
I have ensured that the code is well-documented and easy to understand.

When considering whether a pull request matches an existing pull request, we should consider whether the directories match.

jakecoffman · 2024-06-14T19:39:21Z

updater/lib/dependabot/dependency_change.rb

+
+    sig { returns(T::Boolean) }
+    def should_consider_directory?
+      grouped_update? && Dependabot::Experiments.enabled?("dependency_has_directory")


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?

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 😄

jakecoffman

LGTM besides the question

jurre · 2024-06-15T06:51:24Z

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.

Can we do this before merging using dry-run/the CLI and attach the results from before/after to this PR?

landongrindheim · 2024-06-18T16:30:57Z

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.

updater | +---------------------------------------------------------------------+
updater | |                 Changes to Dependabot Pull Requests                 |
updater | +---------+-----------------------------------------------------------+
updater | | created | @aws-sdk/credential-providers ( from 3.588.0 to 3.598.0 ) |
updater | | created | aws-sdk ( from 2.1602.0 to 2.1643.0 )                     |
updater | | created | eslint ( from 9.0.0 to 9.5.0 )                            |
updater | | created | loglevel ( from 1.5.1 to 1.9.1 )                          |
updater | | created | axios ( from 1.6.2 to 1.7.2 )                             |
updater | | created | fast-jwt ( from 3.0.0 to 4.0.1 )                          |
updater | | created | follow-redirects ( from 1.13.3 to 1.15.6 )                |
updater | | created | json-web-token ( from 3.0.1 to 3.2.0 )                    |
updater | | created | payload ( from 1.5.9 to 2.21.0 )                          |
updater | | created | sharp ( from 0.32.0 to 0.33.4 )                           |
updater | | created | yaml ( from 2.1.0 to 2.4.5 )                              |
updater | +---------+-----------------------------------------------------------+

Manual testing shows my changes didn't solve the problem

Nishnha · 2024-06-18T17:51:28Z

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.

updater | +---------------------------------------------------------------------+
updater | |                 Changes to Dependabot Pull Requests                 |
updater | +---------+-----------------------------------------------------------+
updater | | created | @aws-sdk/credential-providers ( from 3.588.0 to 3.598.0 ) |
updater | | created | aws-sdk ( from 2.1602.0 to 2.1643.0 )                     |
updater | | created | eslint ( from 9.0.0 to 9.5.0 )                            |
updater | | created | loglevel ( from 1.5.1 to 1.9.1 )                          |
updater | | created | axios ( from 1.6.2 to 1.7.2 )                             |
updater | | created | fast-jwt ( from 3.0.0 to 4.0.1 )                          |
updater | | created | follow-redirects ( from 1.13.3 to 1.15.6 )                |
updater | | created | json-web-token ( from 3.0.1 to 3.2.0 )                    |
updater | | created | payload ( from 1.5.9 to 2.21.0 )                          |
updater | | created | sharp ( from 0.32.0 to 0.33.4 )                           |
updater | | created | yaml ( from 2.1.0 to 2.4.5 )                              |
updater | +---------+-----------------------------------------------------------+

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 will be used when calculating ungrouped dependencies in #9988

Nishnha

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

…-updates

luzfcb · 2024-09-05T02:16:39Z

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 {{ and }} It's the same error described in #10525 . This issue did not exist previously, but started occurring when trying to update the dependabot configuration.

@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?

landongrindheim · 2024-09-05T16:14:11Z

@luzfcb That, and other configuration validation, is happening in the Dependabot service. The service's code is not open sourced.

landongrindheim added 3 commits June 14, 2024 17:46

Consider directory when doing grouped updates

9578df8

When considering whether a pull request matches an existing pull request, we should consider whether the directories match.

Feature flag directory comparison

992f41c

Add missing types

224573f

jakecoffman reviewed Jun 14, 2024

View reviewed changes

jakecoffman previously approved these changes Jun 14, 2024

View reviewed changes

landongrindheim marked this pull request as ready for review June 14, 2024 20:14

landongrindheim requested a review from a team as a code owner June 14, 2024 20:14

landongrindheim changed the title ~~Consider directories when performing grouped updates~~ Include directory when serializing dependencies Jun 18, 2024

Nishnha approved these changes Jun 18, 2024

View reviewed changes

Merge branch 'main' into consider-directories-when-performing-grouped…

5498b24

…-updates

landongrindheim merged commit 6f19702 into main Jun 18, 2024
121 checks passed

landongrindheim deleted the consider-directories-when-performing-grouped-updates branch June 18, 2024 19:10

boomshakalakah mentioned this pull request Jul 3, 2024

rebase command runs an action but doesn't actually rebase the branch #10118

Closed

1 task

jakecoffman mentioned this pull request Jul 11, 2024

updating models due to recent multi-dir changes in Core dependabot/cli#340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include `directory` when serializing dependencies #10002

Include `directory` when serializing dependencies #10002

landongrindheim commented Jun 14, 2024 •

edited

Loading

jakecoffman Jun 14, 2024

landongrindheim Jun 14, 2024

jakecoffman left a comment

jurre commented Jun 15, 2024

landongrindheim commented Jun 18, 2024

Nishnha commented Jun 18, 2024 •

edited

Loading

Nishnha left a comment •

edited

Loading

luzfcb commented Sep 5, 2024

landongrindheim commented Sep 5, 2024

Include directory when serializing dependencies #10002

Include directory when serializing dependencies #10002

Conversation

landongrindheim commented Jun 14, 2024 • edited Loading

What are you trying to accomplish?

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

jakecoffman Jun 14, 2024

Choose a reason for hiding this comment

landongrindheim Jun 14, 2024

Choose a reason for hiding this comment

jakecoffman left a comment

Choose a reason for hiding this comment

jurre commented Jun 15, 2024

landongrindheim commented Jun 18, 2024

Nishnha commented Jun 18, 2024 • edited Loading

Nishnha left a comment • edited Loading

Choose a reason for hiding this comment

luzfcb commented Sep 5, 2024

landongrindheim commented Sep 5, 2024

Include `directory` when serializing dependencies #10002

Include `directory` when serializing dependencies #10002

landongrindheim commented Jun 14, 2024 •

edited

Loading

Nishnha commented Jun 18, 2024 •

edited

Loading

Nishnha left a comment •

edited

Loading