Make NuGet V2 support generic for any endpoint#509
Conversation
ksigmund
commented
Jul 17, 2025
- Remove hardcoded _repositoryUrl field from NuGetV2ProjectManager
- Add comprehensive test coverage for multiple NuGet V2 endpoints
- Support both PowerShell Gallery and NuGet.org endpoints
- Maintain backward compatibility with existing PowerShell Gallery usage
- Remove hardcoded _repositoryUrl field from NuGetV2ProjectManager - Add comprehensive test coverage for multiple NuGet V2 endpoints - Support both PowerShell Gallery and NuGet.org endpoints - Maintain backward compatibility with existing PowerShell Gallery usage
There was a problem hiding this comment.
Pull Request Overview
This PR generalizes the NuGet V2 project manager to support any NuGet V2 API endpoint, not just the PowerShell Gallery. The changes enable the system to work with multiple V2 endpoints including NuGet.org while maintaining backward compatibility with existing PowerShell Gallery usage.
- Remove hardcoded PowerShell Gallery URL dependency from NuGetV2ProjectManager
- Implement endpoint detection logic to automatically select V2 manager for any /api/v2 endpoint
- Add comprehensive test coverage for both PowerShell Gallery and NuGet.org V2 endpoints
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| NuGetV2ProjectManager.cs | Removes hardcoded repository URL restriction and updates nupkg URL generation to use any V2 endpoint |
| BaseNuGetProjectManager.cs | Adds generic V2 endpoint detection logic based on URL pattern matching |
| NuGetPackageActions.cs | Removes default parameter to require explicit repository URL specification |
| NuGetProjectManagerV2Tests.cs | Adds comprehensive test coverage for multiple V2 endpoints with both mocked and live API tests |
| BaseNuGetProjectManagerTests.cs | Adds test cases for generic V2 endpoint detection |
| BaseNuGetProjectManagerIntegrationTests.cs | Adds integration tests to verify real-world functionality with multiple endpoints |
Comments suppressed due to low confidence (2)
src/oss-tests/ProjectManagerTests/NuGetProjectManagerV2Tests.cs:195
- The live API tests (MetadataSucceeds_Live_NuGetOrg, EnumerateVersionsSucceeds_Live_NuGetOrg, etc.) make real HTTP calls to external services, which makes tests unreliable and slow. Consider using mocked responses or marking these as integration tests that run separately.
[InlineData("pkg:nuget/Microsoft.Extensions.Logging@8.0.0?repository_url=https://www.nuget.org/api/v2/", false, "Logging infrastructure default implementation for Microsoft.Extensions.Logging.")]
| } | ||
|
|
||
| _projectManager = new NuGetV2ProjectManager(".", nugetPackageActions, _httpFactory); | ||
| projectManager = new NuGetV2ProjectManager(".", nugetPackageActions, _httpFactory); |
There was a problem hiding this comment.
The projectManager variable is being reassigned after being created by CreateRepositorySpecificProjectManager. This creates confusion about which instance is being used and makes the code harder to follow. Consider creating the manager with the mock actions directly instead of reassigning.
| projectManager = new NuGetV2ProjectManager(".", nugetPackageActions, _httpFactory); | |
| var projectManager = new NuGetV2ProjectManager(".", nugetPackageActions, _httpFactory); |
There was a problem hiding this comment.
Agree with Copilot here.
The var projectManager = CreateRepositorySpecificProjectManager(purl); code on line 334 seems unnecessary and can be removed.
danfiedler-msft
left a comment
There was a problem hiding this comment.
Largely LGTM except for a few small improvements to tests suggested by Copilot.
Outside of this PR and longer term, we need to improve test organization and isolate integration tests (#477). The new tests follow existing patterns and are appreciated.
| } | ||
|
|
||
| _projectManager = new NuGetV2ProjectManager(".", nugetPackageActions, _httpFactory); | ||
| projectManager = new NuGetV2ProjectManager(".", nugetPackageActions, _httpFactory); |
There was a problem hiding this comment.
Agree with Copilot here.
The var projectManager = CreateRepositorySpecificProjectManager(purl); code on line 334 seems unnecessary and can be removed.
Add additional integration tests for downloading packages from NuGet v2 upstreams