Add support for Directory.Packages.props file as entrypoint #7086

TobiasLaving · 2023-04-14T14:41:11Z

This PR will enable dependabot to use a Directory.Packages.props file as entry point, allowing the setup below:

Repo:

src/
├─ Backend/
│  ├─ Project1/
│  │  ├─ Project1.csproj
│  ├─ Project2/
│  │  ├─ Project2.csproj
│  ├─ Directory.Packages.props
├─ App/
├─ Solution.sln

dependabot.yml:


version: 2
updates:
  - package-ecosystem: "nuget"
    directory: "/src/Backend"
    schedule:
      interval: "daily"
    open-pull-requests-limit: 5
    target-branch: master

Note: I have never written ruby before and I'm a bit out of my depth
This is working locally towards our repo, but I'm guessing there is a bunch of things I am missing.
I'm happy to get any feedback

yeikel · 2023-04-15T03:40:06Z

Please add tests for this change

Also, is there any issue where this was discussed? This is usually recommended before any pull request is created

TobiasLaving · 2023-04-16T09:51:21Z

The issue is discussed in the issue: #5635

I will look into adding tests!
On another note, as seen in the PR two files have identical functions, is this intentional?
Should the function "project_files" be broken out?

yeikel · 2023-04-16T17:42:03Z

The issue is discussed in the issue: #5635

I will look into adding tests!
On another note, as seen in the PR two files have identical functions, is this intentional?
Should the function "project_files" be broken out?

Refactoring it sounds reasonable to me

Thank you for pointing it out

TobiasLaving · 2023-04-17T14:59:38Z

I now added some tests.

I used this repo to create the fixtures

The repo has drifted from what the PR originally described and looks more like this:

[tobias@tobias-expertbook nuget-directory-packages-props-example]$ tree -P "*.sln|*.csproj|Directory.Packages.props" --prune
.
├── App1
│   ├── App1.sln
│   └── src
│       └── Backend
│           ├── Project1
│           │   └── Project1.csproj
│           └── Project2
│               └── Project2.csproj
└── Directory.Packages.props

The intended dependabot.yml would look like this:

version: 2
updates:
  - package-ecosystem: "nuget"
    directory: "/"
    schedule:
      interval: "daily"
    open-pull-requests-limit: 5
    target-branch: master

The reason for the drift is entirely selfish, it is more like our private repository. Still, the repo in the PR should also work.

For testing I added

Simple test for getting only directory.packages.props if it is found in the root
Test to make sure the new code does not override the old logic for sln and all files are still fetched.

I wanted to do some refactoring but quickly felt out of my depth so I fell back onto doing the least work possible.
Happy to hear any feedback, in particular it would be helpful if we can figure out more tests cases.

jeffwidman

👋 Thanks for putting this up!

I'm not a NuGet expert, but the code change itself looks pretty straightforward to me, so pretty sure we'll be 👍 to ship it.

Two things:

I have a slight preference to land Support NuGet lockfiles #6031 first if possible, as that appears to have a lot more user impact. I don't think it'll create merge conflicts since IIUC the lockfile is unrelated but just in case I'm wrong let's see. That said, if that PR author isn't responsive, then we can ship this first.
I see tests for file_fetcher but not file_parser or file_updater... can you add one for each of those? Don't need anything fancy, just a simple proof that it catches a Directory.Packages.props file so that if someone ever refactors those regexes there's no chance of regression.

TobiasLaving · 2023-04-20T13:02:13Z

@jeffwidman
Ok, sounds good!

jeffwidman · 2023-04-21T02:03:27Z

@TobiasLaving just to clarify, did you see 2️⃣ in my comment above? Can you add those two simple tests?

TobiasLaving · 2023-04-21T09:10:58Z

@jeffwidman Sorry about that, I intended to make the change straight away but life got in the way

Yes, I saw it and just pushed it :)

danquirk · 2023-04-28T20:45:55Z

We have a ton of repos affected by this bug. Curious roughly when we would expect to be able to leverage this fix? I'm trying to avoid having to check in slngen'd .sln files to 100s of repos as a workaround.

DkSkydancer · 2023-05-08T17:56:28Z

@TobiasLaving What is the status on this?
@jeffwidman How long we should expect this PR to hold of for #6031

TobiasLaving · 2023-05-09T06:20:16Z

@DkSkydancer This PR is ready in my opinion

DkSkydancer · 2023-05-12T17:45:37Z

@jeffwidman can we move forward with this please?

DkSkydancer · 2023-06-01T21:07:55Z

@TobiasLaving @jeffwidman ping. we are still waiting for this!

TobiasLaving · 2023-06-05T07:01:01Z

Seems @jeffwidman is out (?)
Do you know the next steps @yeikel ?

Nishnha · 2023-06-05T14:40:36Z

Hey folks, Dependabot maintainer here - we've had a bit of a deploy freeze and have fallen behind on reviews. Sorry about that!

I took a look at the changes and I agree that this PR is in a good state. We should be able to take a look at merging it some time this week.

In the meantime, would you be able to bring it up-to-date with the base branch @TobiasLaving? The CI tests should be able to run now

DkSkydancer · 2023-06-12T21:39:50Z

@TobiasLaving Can you the update as requested above pl?

TobiasLaving · 2023-06-13T07:56:37Z

@DkSkydancer @Nishnha
PR is now rebased and all checks has passed 👍

DkSkydancer · 2023-06-13T13:44:05Z

@Nishnha the PR should be ready now

DkSkydancer · 2023-06-14T18:47:48Z

@Nishnha @jeffwidman Can we get a maintainer review please?

DkSkydancer · 2023-06-14T23:54:38Z

@TobiasLaving it looks like this branch is out of date again I'm afraid :(

Nishnha · 2023-06-15T00:11:03Z

nuget/lib/dependabot/nuget/file_parser.rb

@@ -70,7 +70,7 @@ def project_file_parser
      end

      def project_files
-        dependency_files.select { |df| df.name.match?(/\.[a-z]{2}proj$/) }
+        dependency_files.select { |df| df.name.match?(/\.[a-z]{2}proj$|[Dd]irectory.[Pp]ackages.props/) }


Could we split this regex into two different captures and try to match against both?

Something like

Suggested change

dependency_files.select { |df| df.name.match?(/\.[a-z]{2}proj$|[Dd]irectory.[Pp]ackages.props/) }

projfile = /\.[a-z]{2}proj$/

packageprops = /[Dd]irectory.[Pp]ackages.props/

dependency_files.select { |df|

df.name.match?(projfile) ||

df.name.match?(packageprops)

}

and do the same in the file updater?

Nishnha · 2023-06-15T00:11:28Z

nuget/spec/dependabot/nuget/file_fetcher_spec.rb

+        )
+    end
+
+    it "fetches the files the packages props file" do


typo

Suggested change

it "fetches the files the packages props file" do

it "fetches the packages props file" do

Nishnha · 2023-06-15T00:14:52Z

nuget/spec/fixtures/github/contents_dotnet_directory_packages_props.json

@@ -0,0 +1,19 @@
+{
+  "name": "Directory.Packages.props",


For anyone else reviewing, this response is for the file https://github.com/TobiasLaving/nuget-directory-packages-props-example/blob/main/Directory.Packages.props

Nishnha · 2023-06-15T00:19:21Z

Thank you for this contribution! I left a comment on the regex capture that I would like to see addressed before approving.

We can bring this PR up-to-date before we merge it, so it's fine that it has become out-of-date with the base branch.

@jeffwidman should we fork https://github.com/TobiasLaving/nuget-directory-packages-props-example into the dependabot org since it's referenced in the specs?

TobiasLaving · 2023-06-15T08:32:46Z

Good points, I have made the changes locally sadly the dev container seems to be broken for nuget atm so I cannot run it locally: #7438

Will push once I can validate locally

TobiasLaving · 2023-06-16T12:35:56Z

@Nishnha

Fixed your comments, thanks for finding and fixing the docker issue!

Nishnha

The linter is complaining about one issue but once that's fixed the rest LGTM!

TobiasLaving · 2023-06-21T11:13:03Z

@Nishnha There, the linter is now happy 👍

Struggeled a little bit with running the lint script in the dev container, sorry about the delay

TobiasLaving · 2023-06-26T07:24:15Z

@Nishnha
For my understanding, is anything expected of my right now? Who will press "Merge to master"? :)

Nishnha · 2023-06-26T17:59:45Z

We have a deploy-then-merge process so it's not as straightforward as pressing the merge button. That said, this PR has been merged now 😄

Thank you for the contribution!

After looking into dependabot/dependabot-core#7086 this maybe possible Signed-off-by: Henrique Graca <999396+hjgraca@users.noreply.github.com>

Add support for Directory.Packages.props file as entrypoint

TobiasLaving requested a review from a team as a code owner April 14, 2023 14:41

github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Apr 14, 2023

jeffwidman reviewed Apr 19, 2023

View reviewed changes

TobiasLaving force-pushed the main branch 3 times, most recently from ba8139d to b90e654 Compare April 25, 2023 15:13

TobiasLaving force-pushed the main branch from b90e654 to 9ceefae Compare June 13, 2023 07:47

TobiasLaving force-pushed the main branch from 9ceefae to 3962824 Compare June 14, 2023 06:10

Nishnha reviewed Jun 15, 2023

View reviewed changes

TobiasLaving added 4 commits June 16, 2023 14:15

nuget: Add support for Directory.Packages.props file as entrypoint

c342f96

Add tests

fc5f87f

Add tests

279e8ca

Fix pr comments

db333e6

TobiasLaving force-pushed the main branch from 3962824 to db333e6 Compare June 16, 2023 12:34

Nishnha approved these changes Jun 16, 2023

View reviewed changes

Fixing lint issues

ee7d17b

TobiasLaving force-pushed the main branch from a4e29c1 to ee7d17b Compare June 21, 2023 10:42

Nishnha added 2 commits June 22, 2023 18:06

Merge branch 'main' into main

29afd2e

Merge branch 'main' into main

c645d88

Merge branch 'main' into main

a56bdcd

Nishnha merged commit 89526c2 into dependabot:main Jun 26, 2023
83 checks passed

hjgraca added a commit to aws-powertools/powertools-lambda-dotnet that referenced this pull request Sep 14, 2023

Add dependabot again

3111ee3

After looking into dependabot/dependabot-core#7086 this maybe possible Signed-off-by: Henrique Graca <999396+hjgraca@users.noreply.github.com>

hjgraca mentioned this pull request Sep 14, 2023

chore: Add dependabot again aws-powertools/powertools-lambda-dotnet#478

Open

7 tasks

brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023

Merge pull request dependabot#7086 from TobiasLaving/main

801a5ce

Add support for Directory.Packages.props file as entrypoint

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Directory.Packages.props file as entrypoint #7086

Add support for Directory.Packages.props file as entrypoint #7086

TobiasLaving commented Apr 14, 2023

yeikel commented Apr 15, 2023 •

edited

TobiasLaving commented Apr 16, 2023

yeikel commented Apr 16, 2023 •

edited

TobiasLaving commented Apr 17, 2023

jeffwidman left a comment

TobiasLaving commented Apr 20, 2023

jeffwidman commented Apr 21, 2023

TobiasLaving commented Apr 21, 2023

danquirk commented Apr 28, 2023

DkSkydancer commented May 8, 2023

TobiasLaving commented May 9, 2023

DkSkydancer commented May 12, 2023

DkSkydancer commented Jun 1, 2023

TobiasLaving commented Jun 5, 2023

Nishnha commented Jun 5, 2023 •

edited

DkSkydancer commented Jun 12, 2023

TobiasLaving commented Jun 13, 2023

DkSkydancer commented Jun 13, 2023

DkSkydancer commented Jun 14, 2023

DkSkydancer commented Jun 14, 2023

Nishnha Jun 15, 2023

Nishnha Jun 15, 2023

Nishnha Jun 15, 2023

Nishnha commented Jun 15, 2023

TobiasLaving commented Jun 15, 2023

TobiasLaving commented Jun 16, 2023

Nishnha left a comment

TobiasLaving commented Jun 21, 2023

TobiasLaving commented Jun 26, 2023 •

edited

Nishnha commented Jun 26, 2023

-        dependency_files.select { |df| df.name.match?(/\.[a-z]{2}proj$|[Dd]irectory.[Pp]ackages.props/) }
+        projfile = /\.[a-z]{2}proj$/
+        packageprops = /[Dd]irectory.[Pp]ackages.props/
+        dependency_files.select { |df|
+          df.name.match?(projfile) ||
+          df.name.match?(packageprops)
+        }

	it "fetches the files the packages props file" do
	it "fetches the packages props file" do

Add support for Directory.Packages.props file as entrypoint #7086

Add support for Directory.Packages.props file as entrypoint #7086

Conversation

TobiasLaving commented Apr 14, 2023

yeikel commented Apr 15, 2023 • edited

TobiasLaving commented Apr 16, 2023

yeikel commented Apr 16, 2023 • edited

TobiasLaving commented Apr 17, 2023

jeffwidman left a comment

Choose a reason for hiding this comment

TobiasLaving commented Apr 20, 2023

jeffwidman commented Apr 21, 2023

TobiasLaving commented Apr 21, 2023

danquirk commented Apr 28, 2023

DkSkydancer commented May 8, 2023

TobiasLaving commented May 9, 2023

DkSkydancer commented May 12, 2023

DkSkydancer commented Jun 1, 2023

TobiasLaving commented Jun 5, 2023

Nishnha commented Jun 5, 2023 • edited

DkSkydancer commented Jun 12, 2023

TobiasLaving commented Jun 13, 2023

DkSkydancer commented Jun 13, 2023

DkSkydancer commented Jun 14, 2023

DkSkydancer commented Jun 14, 2023

Nishnha Jun 15, 2023

Choose a reason for hiding this comment

Nishnha Jun 15, 2023

Choose a reason for hiding this comment

Nishnha Jun 15, 2023

Choose a reason for hiding this comment

Nishnha commented Jun 15, 2023

TobiasLaving commented Jun 15, 2023

TobiasLaving commented Jun 16, 2023

Nishnha left a comment

Choose a reason for hiding this comment

TobiasLaving commented Jun 21, 2023

TobiasLaving commented Jun 26, 2023 • edited

Nishnha commented Jun 26, 2023

yeikel commented Apr 15, 2023 •

edited

yeikel commented Apr 16, 2023 •

edited

Nishnha commented Jun 5, 2023 •

edited

TobiasLaving commented Jun 26, 2023 •

edited