dependabot corrupts versions of other packages (nuget / dotnet)

CZEMacLeod commented 1 month ago

Is there an existing issue for this?

[X] I have searched the existing issues

Package ecosystem

nuget

Package manager version

No response

Language version

dotnet c#

Manifest location and content before the Dependabot update

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="6.0.31" Condition="'$(TargetFramework)'=='net6.0'" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="7.0.20" Condition="'$(TargetFramework)'=='net7.0'" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="8.0.6" Condition="'$(TargetFramework)'=='net8.0'" />
    <PackageReference Include="Microsoft.Playwright" Version="1.46.0" />
  </ItemGroup>

https://github.com/CZEMacLeod/C3D.Extensions.Playwright.AspNetCore/pull/70/files#diff-bf19e3b9129de562b09dd63b4286aa6b64b2f58d14c7cc3778950a3c479a6671

dependabot.yml content

https://github.com/CZEMacLeod/C3D.Extensions.Playwright.AspNetCore/blob/main/.github/dependabot.yml

# To get started with Dependabot version updates, you'll need to specify which
# package ecosystems to update and where the package manifests are located.
# Please see the documentation for all configuration options:
# https://docs.github.com/github/administering-a-repository/configuration-options-for-dependency-updates

version: 2
updates:
  - package-ecosystem: "nuget" # See documentation for possible values
    directory: "/" # Location of package manifests
    schedule:
      interval: "weekly"
    ignore:
      - dependency-name: "Microsoft.AspNetCore.Mvc.Testing"
        update-types: ["version-update:semver-major"] # Don't try to upgrade 6.x to 7.x as we target both frameworks.

Updated dependency

Bump Microsoft.Playwright from 1.46.0 to 1.47.0

https://github.com/CZEMacLeod/C3D.Extensions.Playwright.AspNetCore/pull/70/files#diff-bf19e3b9129de562b09dd63b4286aa6b64b2f58d14c7cc3778950a3c479a6671

What you expected to see, versus what you actually saw

Expected:

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="6.0.31" Condition="'$(TargetFramework)'=='net6.0'" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="7.0.20" Condition="'$(TargetFramework)'=='net7.0'" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="8.0.6" Condition="'$(TargetFramework)'=='net8.0'" />
    <PackageReference Include="Microsoft.Playwright" Version="1.47.0" />
  </ItemGroup>

Actual: https://github.com/CZEMacLeod/C3D.Extensions.Playwright.AspNetCore/pull/70/files#diff-bf19e3b9129de562b09dd63b4286aa6b64b2f58d14c7cc3778950a3c479a6671

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="8.0.6" Condition="'$(TargetFramework)'=='net6.0'" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="8.0.6" Condition="'$(TargetFramework)'=='net7.0'" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="8.0.6" Condition="'$(TargetFramework)'=='net8.0'" />
    <PackageReference Include="Microsoft.Playwright" Version="1.47.0" />
  </ItemGroup>

Native package manager behavior

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="6.0.31" Condition="'$(TargetFramework)'=='net6.0'" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="7.0.20" Condition="'$(TargetFramework)'=='net7.0'" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="8.0.6" Condition="'$(TargetFramework)'=='net8.0'" />
    <PackageReference Include="Microsoft.Playwright" Version="1.47.0" />
  </ItemGroup>

Images of the diff or a link to the PR, issue, or logs

https://github.com/CZEMacLeod/C3D.Extensions.Playwright.AspNetCore/pull/70

Smallest manifest that reproduces the issue

I think if there are packages with different versions it resets them all (despite conditionals). It also ignored the ignore in the yml. This is a pretty minimal file to repo it on in the first place.

brettfo commented 3 weeks ago

This appears to be the same issue as #9299. Closing this issue to track in the other one.

CZEMacLeod commented 3 weeks ago

@brettfo Similar yes, but I think there are actually 3 different issues.

The PR is just for an unrelated package, and it should not be updating or touching the lines for Microsoft.AspNetCore.Mvc.Testing when updating Microsoft.Playwright.

If you look at the PR it is just

Bumps Microsoft.Playwright from 1.46.0 to 1.47.0.

When it generates a PR for Microsoft.AspNetCore.Mvc.Testing, it should take the condition into account and not update them all to the same version.

And it doesn't obey the ignore semver-major where it should update 8.0.6 -> 8.0.8 and 7.0.20 -> 7.0.21 etc.

brettfo commented 3 weeks ago

Internally the NuGet updater tries to make a coherent package set when performing an update and since the Condition attributes aren't correctly handled, they are all seen as the same thing and it's not valid to have 3 different versions of the same package present so the issue is "fixed" (not correctly) by setting them all to the latest version.

The work to properly honor the Condition attribute should fix this.

Regarding the ignore parameter, there are scenarios where that might not be workable. For example, if you said to ignore some package, but updating some other package required the first package to be updated, then we're kind of stuck: do we update the initial package and pull along the one we're supposed to ignore, or do we honor the ignore condition and fail to update the initial package?

CZEMacLeod commented 3 weeks ago

@brettfo I feel that if it does anything other than the bump in the description, it should include that in the PR description notes and/or make multiple commits. In my case - a commit with just the bump of Microsoft.Playwright which is what it has in the description, and a second commit of any consistency updates or fixes it has had to apply. It would make it much clearer about what happened and why, as well as offering insight into ensuring that whatever was needing to be fixed, doesn't happen again. As this is put into a PR, and it gets squashed when merged, I feel that something like this would be a big improvement for visibility and explaining why dependabot does certain things.

brettfo commented 3 weeks ago

@CZEMacLeod Thank you for the feedback on structuring the PR. We're currently re-writing the NuGet updater and I'll remember this as we get to that part of the work.

dependabot / dependabot-core