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

Fix Duplicate Dependencies Showing in PR summary table #9436

Merged
merged 9 commits into from Apr 5, 2024

Conversation

honeyankit
Copy link
Contributor

Context

This PR resolves the issue of duplicated entries in the group update PR's summary table by ensuring that only unique dependencies are considered in the message builder. These are then displayed in the PR summary table.

Fixes: #7695

@honeyankit honeyankit self-assigned this Apr 4, 2024
@honeyankit honeyankit requested a review from a team as a code owner April 4, 2024 18:20
@honeyankit
Copy link
Contributor Author

I started searching about why we are getting duplicate dependencies and got my answer here so made changes in the message builder.

 #<Dependabot::Dependency:0x0000ffffa6e74c48
  @metadata={:directory=>"/"},
  @name="tapioca",
  @package_manager="bundler",
  @previous_requirements=[{:requirement=>">= 0", :groups=>[:development], :source=>nil, :file=>"Gemfile"}],
  @previous_version="0.12.0",
  @removed=false,
  @requirements=[{:requirement=>">= 0", :groups=>[:development], :source=>nil, :file=>"Gemfile"}],
  @version="0.13.1">,

 #<Dependabot::Dependency:0x0000ffffa6e85458
  @metadata={},
  @name="tapioca",
  @package_manager="bundler",
  @previous_requirements=[{:requirement=>">= 0", :groups=>[:development], :source=>nil, :file=>"Gemfile"}],
  @previous_version="0.12.0",
  @removed=false,
  @requirements=[{:requirement=>">= 0", :groups=>[:development], :source=>nil, :file=>"Gemfile"}],
  @version="0.13.1">,

@jurre
Copy link
Member

jurre commented Apr 4, 2024

Can we add a test to prevent regression?

@honeyankit
Copy link
Contributor Author

Can we add a test to prevent regression?

I have tested against the test repo. I am adding the test.

@Nishnha
Copy link
Member

Nishnha commented Apr 4, 2024

There's also a mention of the table showing a dependency updating from a commit to a version, even though they didn't actually update: #7695 (comment)

Is a fix for that going to be part of this PR?

@honeyankit
Copy link
Contributor Author

There's also a mention of the table showing a dependency updating from a commit to a version, even though they didn't actually update: #7695 (comment)

Is a fix for that going to be part of this PR?

No. This will fix the duplicate entry. I can look into that as well. But first will let this ship

@honeyankit honeyankit force-pushed the honeyankit/fix-duplicate-dependency-in-pr-summary branch from d8215ab to 7d078fa Compare April 4, 2024 19:25
@Nishnha
Copy link
Member

Nishnha commented Apr 4, 2024

Since we're only uniq'ing by name, wouldn't the dependencies that gets removed from the PR body be random? I wonder if the "to" and "from" version can change for a duplicate dependency if you run the same job twice

The note at

# We should retain a list of all dependencies that we change, in future we may need to account for the folder
# in which these changes are made to permit-cross folder updates of the same dependency.
#
# This list may contain duplicates if we make iterative updates to a Dependency within a single group, but
# rather than re-write the Dependency objects to account for the changes from the lowest previous version
# to the final version, we should defer it to the Dependabot::PullRequestCreator::MessageBuilder as a
# presentation concern.
recommends keeping the dependency with the lowest previous version but idk if that's why it was suggested

@honeyankit
Copy link
Contributor Author

honeyankit commented Apr 4, 2024

@Nishnha : For less then 5 dependencies for group PR, similar approach is used here where by the dependency name the unique dependencies are filtered. So I have just refer that for more then 5 dependencies.

@Nishnha
Copy link
Member

Nishnha commented Apr 4, 2024

@Nishnha : For less then 5 dependencies for group PR, similar approach is used here where by the dependency name the unique dependencies are filtered. So I have just refer that for more then 5 dependencies.

Right but the dependency version isn't displayed for <5 dependencies

@jurre
Copy link
Member

jurre commented Apr 5, 2024

We could check for unique by name, from and to to be thorough?

@honeyankit
Copy link
Contributor Author

@jurre and @Nishnha : I have updated the code to remove duplicates based on name, to and from versions.

  1. Both Dep A will be shown
Dep A - previous version 1.2.4 latest version 1.2.5 
Dep A - previous version 1.2.4 latest version 1.2.6 
  1. Both Dep A will be shown
Dep A - previous version 1.2.1 latest version 1.2.5 
Dep A - previous version 1.2.4 latest version 1.2.5

@honeyankit
Copy link
Contributor Author

All the smoke test errors are due to rate limit exceeded.

@honeyankit honeyankit force-pushed the honeyankit/fix-duplicate-dependency-in-pr-summary branch from 67ebcd9 to 3ebb6fb Compare April 5, 2024 17:47
@honeyankit honeyankit force-pushed the honeyankit/fix-duplicate-dependency-in-pr-summary branch from f689a4f to beb7acc Compare April 5, 2024 18:24
Copy link
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@honeyankit honeyankit merged commit 47857e8 into main Apr 5, 2024
139 of 140 checks passed
@honeyankit honeyankit deleted the honeyankit/fix-duplicate-dependency-in-pr-summary branch April 5, 2024 20:28
@edmorley
Copy link

@honeyankit @Nishnha @landongrindheim Could this have caused the regression seen in #9457?

thavaahariharangit pushed a commit to thavaahariharangit/dependabot-core that referenced this pull request Apr 12, 2024
* Fix the duplicate dependencies in PR summary table
Co-authored-by: Jurre <jurre@github.com>
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.

Listing unchanged dependency twice in the update PR
5 participants