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

add end-to-end C# update runner #10521

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Conversation

brettfo
Copy link
Contributor

@brettfo brettfo commented Aug 29, 2024

Add an option for the NuGet updater to handle the entire update process, from git clone all the way through mark_as_processed.

This is necessary because the concept of a dependency according to MSBuild/NuGet is nothing like the concept of a dependency for dependabot-core and it would require a huge rewrite of a bunch of the common code, which really means a rewrite of all 20+ updaters, so this is the best way forwards. The short version for why NuGet is so different: MSBuild allows arbitrary directory crawling and each dependency found could be top-level or transitive and each one could have different updatability semantics based on the project's TFM and/or NuGet.Config. Because of all of this, the NuGet updater needs to check every single dependency for upgradability; we can't do one big update check.

The new behavior has been placed behind an experimental flag nuget_native_updater. To enable this, the bin/run script was renamed to bin/run-original (only in this image) and a new bin/run checks the experiment flag in the job file and does the appropriate thing. To accomplish this, the Linux command jq was added to the image to query the job.json file for the flag. Powershell was also added as there's a new main.ps1 that serves as the entrypoint. This was placed in a Powershell script because it makes it easy to test the clone and SDK installation on both Linux and Windows.

The new RunWorker class is a simple wrapper around DiscoveryWorker, AnalyzeWorker, and UpdateWorker and the only change to those three classes was to add an overload that takes and returns the object files directly, instead of requiring (de)serialization to/from disk. The old behavior remains unchanged. The actual body of RunWorker is fairly straightforward; the bulk of new lines in this PR is to support the object model required for the HTTP API calls (e.g., create_pull_request, mark_as_processed, etc.)

The only scenario covered by this code is very narrow and will fail in weird ways for the vast majority of update operations, but I wanted to get this work reviewed and merged so it can be slowly iterated on.

In this PR I'm primarily looking for feedback on the architecture/implementation of main.ps1 and RunWorker.cs; the rest is mainly plumbing code.

@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Aug 29, 2024
@brettfo brettfo force-pushed the dev/brettfo/nuget-updater-rewrite branch 6 times, most recently from e9f9d69 to 0c71f53 Compare August 30, 2024 00:19
Copy link

@NomadsComputerllc NomadsComputerllc left a comment

Choose a reason for hiding this comment

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

Tahnks

@brettfo brettfo marked this pull request as ready for review August 30, 2024 16:35
@brettfo brettfo requested a review from a team as a code owner August 30, 2024 16:35
Copy link
Contributor

@ByAgenT ByAgenT left a comment

Choose a reason for hiding this comment

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

LGTM, left a few extra questions for my learning and better understanding

nuget/updater/main.ps1 Show resolved Hide resolved
nuget/updater/main.ps1 Outdated Show resolved Hide resolved
@brettfo brettfo force-pushed the dev/brettfo/nuget-updater-rewrite branch from 0c71f53 to 5c44007 Compare September 25, 2024 19:40
@sachin-sandhu sachin-sandhu force-pushed the dev/brettfo/nuget-updater-rewrite branch from 5c44007 to e97d96d Compare September 25, 2024 23:12
@sachin-sandhu sachin-sandhu merged commit ddb9722 into main Sep 26, 2024
68 checks passed
@sachin-sandhu sachin-sandhu deleted the dev/brettfo/nuget-updater-rewrite branch September 26, 2024 02:30
@martincostello martincostello mentioned this pull request Sep 26, 2024
5 tasks
@willibrandon
Copy link

Is this supposed to work when central package management is enabled? I’m seeing the version failing to be located in that scenario.

@andrcuns
Copy link
Contributor

FYI, this breaks ability to build nuget ecosystem docker image for arm64 arch because there is no powershell package it looks like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants