Fix EnsureCachedAsync to discover repo-local nuget.config (#37)#62
Merged
Conversation
The public EnsureCachedAsync overload called Settings.LoadDefaultSettings(null), which never scans the working directory (or any ancestor) for a project/repo-local nuget.config - only machine/user-wide settings are loaded. Any repository that defines its real package sources in a repo-local nuget.config (with only packageSourceCredentials living in the user-level config) had those sources silently invisible to callers, unlike the dotnet CLI / MSBuild restore pipeline. Add an optional root parameter to EnsureCachedAsync, forwarded to Settings.LoadDefaultSettings(root ?? Directory.GetCurrentDirectory()), so callers can point the settings lookup at their project/repo directory and have nuget.config discovered the same way dotnet restore discovers it. Add a WireMock-backed regression test proving a repo-local nuget.config is honored when root is supplied, and update existing tests/design docs accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes NuGet settings discovery in NuGetCache.EnsureCachedAsync so repository-local nuget.config files can be found (matching the dotnet restore/MSBuild behavior) by rooting Settings.LoadDefaultSettings(...) at a caller-provided (or current working) directory. It also adds a WireMock-backed regression test and updates documentation/tests accordingly.
Changes:
- Updated the public
NuGetCache.EnsureCachedAsyncto load default NuGet settings fromroot ?? Directory.GetCurrentDirectory()instead ofnull. - Added a regression test verifying a repo-local
nuget.configis honored whenrootis supplied. - Updated existing tests and design docs to reflect the new public API shape/usage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DemaConsulting.NuGet.Caching.Tests/NuGetCachingTests.cs | Updated calls to EnsureCachedAsync to use named cancellationToken argument. |
| test/DemaConsulting.NuGet.Caching.Tests/NuGetCacheTests.cs | Updated calls to EnsureCachedAsync to use named cancellationToken argument. |
| test/DemaConsulting.NuGet.Caching.Tests/NuGetCacheServerTests.cs | Added WireMock regression test for repo-local nuget.config discovery via root. |
| src/DemaConsulting.NuGet.Caching/NuGetCache.cs | Changed default settings load to be rooted at root/CWD; updated XML docs accordingly. |
| docs/design/nuget-caching/nuget-cache.md | Updated design documentation to describe rooted default settings discovery and new API signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rewrap paragraph so GitHub issue reference '#37' no longer starts a line, which markdownlint-cli2 (MD018) misinterprets as a malformed ATX heading.
The user guide's EnsureCachedAsync signature block and Example 2 still showed CancellationToken as the 3rd positional parameter, but the fix in b06e039 inserted an optional root parameter before it. The documented example would no longer compile (CancellationToken has no implicit conversion to string). Updated the signature block, parameter list, and Example 2 to use the root parameter and pass cancellationToken by name, matching the pattern already used in the test files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The public EnsureCachedAsync overload called Settings.LoadDefaultSettings(null), which never scans the working directory (or any ancestor) for a project/repo-local nuget.config - only machine/user-wide settings are loaded. Any repository that defines its real package sources in a repo-local nuget.config (with only packageSourceCredentials living in the user-level config) had those sources silently invisible to callers, unlike the dotnet CLI / MSBuild restore pipeline.
Add an optional root parameter to EnsureCachedAsync, forwarded to Settings.LoadDefaultSettings(root ?? Directory.GetCurrentDirectory()), so callers can point the settings lookup at their project/repo directory and have nuget.config discovered the same way dotnet restore discovers it.
Add a WireMock-backed regression test proving a repo-local nuget.config is honored when root is supplied, and update existing tests/design docs accordingly.