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.

Sign up for GitHub

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

Jump to bottom

feature-intern-ResolveDependencyConflictsNew to main #10343

Merged
merged 3 commits into from
Aug 13, 2024
Merged

feature-intern-ResolveDependencyConflictsNew to main #10343

merged 3 commits into from
Aug 13, 2024

Conversation

NadiamB
Copy link
Contributor

@NadiamB NadiamB commented Aug 1, 2024 •
edited
Loading

My code is currently behind an environment variable not yet set. Will work with @brettfo to put this behind the experiment flag.

Looking for feedback on implementation, unit tests, variable names, etc.

DependencyConflictResolver.cs is the main code.

MSBuildHelper.cs and MSBuildHelperTests.cs were used for tests

UpdateWorkerTests.Sdk.cs was lightly used with testing with Brett

Added temporary commit to make smoke tests pass.

Smoke Test PR:
dependabot/smoke-tests#219

This is the PR that adds the experiment:
https://github.com/github/dependabot-api/pull/5511

@NadiamB NadiamB requested review from a team as code owners August 1, 2024 22:04
@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Aug 1, 2024
@NadiamB NadiamB force-pushed the feature-intern-ResolveDependencyConflictsNew branch 4 times, most recently from f689e8f to 437c713 Compare August 8, 2024 17:24
jpinz
jpinz approved these changes Aug 8, 2024
View reviewed changes
Copy link
Contributor

@jpinz jpinz left a comment

Choose a reason for hiding this comment

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

This is amazing to see!
I know your last day is tomorrow, so no pressure to resolve any of these comments, I just wanted to provide some feedback!
Thanks for all of your hard work this summer!

.github/workflows/smoke.yml
@@ -14,7 +14,7 @@ concurrency:

env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
SMOKE_TEST_BRANCH: main
SMOKE_TEST_BRANCH: feature-DependencySolver
Copy link
Contributor

Choose a reason for hiding this comment

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

q: was this change meant to be checked in?

Copy link
Contributor

Choose a reason for hiding this comment

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

We added a smoke test, this was just to make sure it was green in CI. The commit that added this will be removed prior to merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

We just revert back this change because we are getting error.

Copy link
Contributor

Choose a reason for hiding this comment

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

#10437

nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Cli.Test/EntryPointTests.Update.cs Outdated Show resolved Hide resolved
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.Sdk.cs Outdated Show resolved Hide resolved
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.Sdk.cs Outdated Show resolved Hide resolved
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Utilities/MSBuildHelperTests.cs Outdated Show resolved Hide resolved
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/SdkPackageUpdater.cs Outdated Show resolved Hide resolved
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/SdkPackageUpdater.cs Outdated Show resolved Hide resolved
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/MSBuildHelper.cs Show resolved Hide resolved
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/MSBuildHelper.cs Outdated Show resolved Hide resolved
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/MSBuildHelper.cs Outdated Show resolved Hide resolved
@NadiamB NadiamB force-pushed the feature-intern-ResolveDependencyConflictsNew branch from 0a45eb8 to cfee286 Compare August 9, 2024 17:38
@NadiamB NadiamB mentioned this pull request Aug 9, 2024
Open
ryanbrandenburg
ryanbrandenburg reviewed Aug 9, 2024
View reviewed changes
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Utilities/MSBuildHelperTests.cs
@@ -493,6 +497,786 @@ public async Task DependencyConflictsCanBeResolved()
}
}

#region
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: It's generally good to name regions so that people know what's inside if they're collapsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do these in follow up PR

nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/SdkPackageUpdater.cs
int dependencyIndex = Array.FindIndex(updatedTopLevelDependencies, d => string.Equals(d.Name, dependencyName, StringComparison.OrdinalIgnoreCase));
if (dependencyIndex != -1)
{
var originalDependency = updatedTopLevelDependencies[dependencyIndex];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to preserve this modified dependency somewhere?

nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/DependencyConflictResolver.cs
}

// Method alterted from VersionFinder.cs to find the metadata of a given package
private async Task<IPackageSearchMetadata?> FindPackageMetadataAsync(PackageIdentity packageIdentity, CancellationToken cancellationToken)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Generally private methods go under public methods so it's easier to find the "entry-points".

@sachin-sandhu sachin-sandhu force-pushed the feature-intern-ResolveDependencyConflictsNew branch from f411b68 to 144fc23 Compare August 13, 2024 13:47
@sachin-sandhu sachin-sandhu merged commit d09bc1e into main Aug 13, 2024
123 checks passed
@sachin-sandhu sachin-sandhu deleted the feature-intern-ResolveDependencyConflictsNew branch August 13, 2024 14:32
@rhyskoedijk rhyskoedijk mentioned this pull request Aug 18, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbukum1 kbukum1 kbukum1 left review comments

@ryanbrandenburg ryanbrandenburg ryanbrandenburg left review comments

@jpinz jpinz jpinz approved these changes

@JamieMagee JamieMagee JamieMagee left review comments

@brettfo brettfo brettfo approved these changes

@abdulapopoola abdulapopoola abdulapopoola approved these changes

Assignees
No one assigned
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@NadiamB @abdulapopoola @brettfo @JamieMagee @jpinz @ryanbrandenburg @kbukum1 @sachin-sandhu