Remove legacy context APIs - #4248
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR consolidates context APIs for v4. It renames and expands the environment context, removes duplicate legacy contexts, updates file and installer access, migrates consumers, and updates tests and documentation. ChangesContext API consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR consolidates and removes public context APIs and changes PowerShell installer selection to treat all non-X86 processes as x64. It is mergeable with explicit owner awareness for external consumer migration/rollback coordination and for defining or validating behavior on ARM and other architectures. Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The changes satisfy the linked issue requirements for removing legacy environment, file-system, and installer APIs; renaming and extending the environment contract; migrating consumers; updating documentation; and changing IBuildSystemDetector visibility. The provided summary does not confirm whether BuildSystemDetector is internal.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR removes legacy context APIs, consolidates environment and filesystem capabilities under domain contexts, and migrates dependent integrations and tests.
Confidence Score: 4/5The PR is not yet safe to merge because GitVersioning can still bypass a configured filesystem provider and write GitVersion.yml to the host repository. The temporary-folder fix preserves the injected provider, but the same GitVersioning operation constructs its configuration file through the system provider, leaving provider-isolated executions able to access and mutate the real filesystem. Files Needing Attention: src/ModularPipelines.Git/GitVersioning.cs
|
| Filename | Overview |
|---|---|
| src/ModularPipelines.Git/GitVersioning.cs | The temporary directory now retains the injected provider, but GitVersion.yml handling still bypasses it and can mutate the host filesystem. |
| src/ModularPipelines/FileSystem/Folder.cs | Adds provider-aware temporary-folder creation used by the Git integration. |
| src/ModularPipelines/Context/Domains/Implementations/EnvironmentContext.cs | Consolidates host and pipeline environment data into the renamed domain context with provider-backed Folder values. |
| src/ModularPipelines/Context/Domains/Implementations/FilesContext.cs | Replaces the legacy filesystem context dependency with direct provider-backed path resolution. |
| src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs | Updates registrations for the consolidated context implementations and removed legacy services. |
Comments Outside Diff (1)
-
src/ModularPipelines.Git/GitVersioning.cs, line 113 (link)Configuration bypasses filesystem provider
When a pipeline or
ModuleTesterreplacesIFileSystemProviderand invokes GitVersion without an existingGitVersion.yml,new File(string)uses the system provider for the existence check and write, causing the real host repository to be read or modified instead of the configured filesystem.
Reviews (5): Last reviewed commit: "fix(context): respect environment abstra..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GitHub Actions
Inactive
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
Reviewed the context-API cleanup (removal of IEnvironmentContext/IFileSystemContext/IInstaller legacy wrappers, rename of IEnvironmentDomainContext → IEnvironmentContext, and the DI/test updates that follow). The consolidation itself is clean and the test/doc updates track the renames correctly. One regression stood out.
Blocking
GitVersioning now bypasses the pipeline's configured IFileSystemProvider
GitVersioning's constructor used to take IFileSystemContext and call fileSystemContext.CreateTemporaryFolder(), which internally used the DI-registered IFileSystemProvider. This PR replaces that with the static Folder.CreateTemporaryFolder(), which is hardcoded to SystemFileSystemProvider.Instance (see src/ModularPipelines/FileSystem/Folder.cs:506-515) and ignores whatever provider is registered in the container.
This matters because ModularPipelines.Testing.ModuleTester replaces IFileSystemProvider with an InMemoryFileSystemProvider for isolated module tests (src/ModularPipelines.Testing/ModuleTester.cs:162,186), and the framework generally supports swapping in custom IFileSystemProvider implementations. Any module depending on IGitVersioning will now silently touch the real disk during isolated tests instead of respecting the in-memory/fake provider, breaking the isolation the testing package is built around. (Independently flagged by the greptile-apps bot review on this PR as well.)
Suggested fix: keep injecting the provider abstraction (e.g. take IFileSystemProvider in GitVersioning's constructor and build the temp folder from it directly, the way FilesContext does now) rather than routing through the static Folder.CreateTemporaryFolder() helper.
Minor — abstraction consistency
Environment.Is64BitOperatingSystem used directly instead of the existing IEnvironmentContext.Architecture abstraction
Powershell7Async reads _environmentContext.OperatingSystem (mockable, as used in PredefinedInstallersTests.cs) just above, but then falls back to the raw static System.Environment.Is64BitOperatingSystem for architecture instead of _environmentContext.Architecture, which already exists on the same interface and is exercised elsewhere in this PR's own test updates (EnvironmentContextTests.cs now asserts context.Architecture). Using the static call re-introduces an untestable dependency in a method that otherwise reads entirely through the injected context, and is inconsistent with the rest of the class. Not blocking, but worth tidying up while this file is already being touched for the domain-context migration.
Other notes
- This PR currently shows as having a merge conflict with
main(mergeStateStatus: DIRTY) — will need a rebase before merge, independent of the above.
d6b0b16 to
b90314f
Compare
|
Rebased onto current main after #4247. Resolved the environment API overlap by using the renamed domain Variables.Get/GetAll/Set surface. Core test solution and Azure Pipelines Release builds pass; EnvironmentContext tests pass 6/6. |
There was a problem hiding this comment.
Code review
Solid cleanup: the environment/file-system/installer context consolidation is internally consistent — I traced every deleted type (IEnvironmentDomainContext, IFileSystemContext, IInstaller, the old top-level IEnvironmentContext/EnvironmentContext) and confirmed no remaining references anywhere in src/test/docs, the DI registrations are deduplicated correctly (single IEnvironmentContext → domain EnvironmentContext), and the new IFilesContext.GetFile/GetFolder blank-path validation is covered by the new tests.
Finding
GitVersioning's temporary folder now silently bypasses the DI-configured IFileSystemProvider, breaking ModuleTester's in-memory filesystem isolation.
ModularPipelines/src/ModularPipelines.Git/GitVersioning.cs
Lines 45 to 51 in b90314f
Before this PR, GitVersioning took IFileSystemContext in its constructor and called fileSystemContext.CreateTemporaryFolder(), which resolved the temp path through the injected IFileSystemProvider. Now it calls the static Folder.CreateTemporaryFolder() (src/ModularPipelines/FileSystem/Folder.cs:506), which is hard-coded to SystemFileSystemProvider.Instance and has no way to honor a different provider.
This matters because IFileSystemProvider is registered with TryAddSingleton (DependencyInjectionSetup.cs:249) specifically so it can be swapped out — and ModularPipelines.Testing.ModuleTester does exactly that, replacing it with InMemoryFileSystemProvider (ModuleTester.cs:186) to give module tests filesystem isolation. Any module that depends on IGitVersioning and is exercised through ModuleTester will now have GitVersioning's constructor create a real directory on disk the moment it's resolved, regardless of the in-memory provider the test set up — silently defeating the isolation ModuleTester promises.
This is the same concern the Greptile bot flagged on this PR ("GitVersion still creates its tool directory through SystemFileSystemProvider.Instance..."), and it doesn't look like it's been addressed in the latest commit.
Suggested fix: inject IFileSystemProvider into GitVersioning and build the temp folder through it, mirroring how EnvironmentContext now correctly does new Folder(path, fileSystemProvider):
public GitVersioning(
IGitInformation gitInformation,
ICommandContext command,
IModuleLoggerProvider moduleLoggerProvider,
IFileSystemProvider fileSystemProvider)
{
_gitInformation = gitInformation;
_command = command;
_moduleLoggerProvider = moduleLoggerProvider;
var path = fileSystemProvider.Combine(
fileSystemProvider.GetTempPath(),
fileSystemProvider.GetRandomFileName().Replace(".", string.Empty));
fileSystemProvider.CreateDirectory(path);
_temporaryFolder = new Folder(path, fileSystemProvider);
}This restores the provider-aware behavior the deleted FileSystemContext.CreateTemporaryFolder() had, keeps GitVersioning consistent with the rest of the domain contexts in this PR (which all correctly thread IFileSystemProvider through), and preserves ModuleTester's filesystem-isolation guarantee for any module under test that depends on Git versioning.
(Note: PredefinedInstallers.cs and TempFolder.cs already call the same static Folder.CreateTemporaryFolder() — that's pre-existing and out of scope here; flagging only because this PR is the one that newly introduced this pattern into GitVersioning, which previously was provider-aware.)
|
Addressed the current-head Claude blocking review in b7058c9: GitVersioning temp-folder creation is provider-aware again, preserving ModuleTester in-memory filesystem isolation. The matching Greptile thread has the implementation and validation details. |
There was a problem hiding this comment.
Code review
Re-reviewed at head b7058c96. The previously-flagged blocking issue — GitVersioning bypassing the DI-configured IFileSystemProvider — is now fixed: GitVersioning.cs:54 calls Folder.CreateTemporaryFolder(fileSystemProvider), correctly threading the injected provider through and preserving ModuleTester's in-memory filesystem isolation. Good fix.
Two issues remain, one carried over from the earlier review and unaddressed, one new:
1. Environment.Is64BitOperatingSystem still used directly instead of the IEnvironmentContext.Architecture abstraction (carried over, not addressed)
src/ModularPipelines/Context/PredefinedInstallers.cs:121-123:
if (operatingSystem == OSPlatform.Windows)
{
var arch = Environment.Is64BitOperatingSystem ? "x64" : "x86";Powershell7Async reads _environmentContext.OperatingSystem (mockable) on the line above, but falls back to the raw static System.Environment.Is64BitOperatingSystem for architecture instead of _environmentContext.Architecture — which this very PR introduced onto the interface and which EnvironmentContextTests.cs now exercises. This re-introduces an untestable, unmockable dependency in a method that otherwise reads entirely through the injected context, and is inconsistent with the rest of the class after the domain-context consolidation. It also means a test that mocks OperatingSystem == OSPlatform.Windows to exercise the 32-bit download URL branch can no longer control which URL is chosen, since bitness now comes from the real test-runner machine rather than the mockable context.
Suggested fix: var arch = _environmentContext.Architecture == Architecture.X64 ? "x64" : "x86"; (or equivalent mapping), consistent with how OperatingSystem is read two lines above.
2. EnvironmentContext's Folder properties now throw instead of degrading to null/empty on blank input
src/ModularPipelines/Context/Domains/Implementations/EnvironmentContext.cs:35-38:
WorkingDirectory = new Folder(workingDirectory.Path, fileSystemProvider);
AppDomainDirectory = new Folder(AppDomain.CurrentDomain.BaseDirectory, fileSystemProvider);
ContentDirectory = new Folder(hostEnvironment.ContentRootPath, fileSystemProvider);The internal Folder(string, IFileSystemProvider) constructor (Folder.cs:41) passes the path straight into new DirectoryInfo(path) with no null/blank guard, unlike the public implicit string -> Folder? conversion operator (Folder.cs:524-532), which returns null for a null/empty path. Previously, ContentDirectory = _hostEnvironment.ContentRootPath!; went through that implicit conversion and degraded gracefully. Now, if a consumer replaces IHostEnvironment via the public PipelineBuilder.Services DI surface with one that returns a null/empty ContentRootPath (the default PipelineBuilder.CreateHostEnvironment path always guarantees non-blank values via FirstNonEmpty(...), so this is only reachable through a custom override), construction now throws ArgumentException/ArgumentNullException instead of yielding a null/empty Folder.
This is a low-probability edge case given the default construction path is safe, but it's a behavior change worth being deliberate about — either guard the three call sites the same way the old implicit conversion did, or confirm a hard failure is the intended contract now that these properties are non-nullable Folder (as opposed to the old nullable-friendly string conversion).
Other notes
Everything else in the consolidation checks out: I traced the renamed/removed symbols (EnvironmentVariables→Variables, OperatingSystemIdentifier→OSPlatform, GetFolder(SpecialFolder) removal, IBuildSystemDetector becoming internal) across src/test/docs and found no stale references. Test coverage for the deleted FileSystemContextTests.cs behaviors is still present via FileTests.cs/FolderTests.cs/FolderProviderTests.cs/FileProviderTests.cs, so that removal isn't a coverage loss.
GitHub Actions
Failure
|
Addressed the current-head blocking review in
Validation: focused installer tests 4/4, environment-context tests 7/7, and guarded |
There was a problem hiding this comment.
Code review
Re-reviewed at head b0b4ce0320. Both issues from the previous review round are now resolved:
Environment.Is64BitOperatingSystem→IEnvironmentContext.Architecture—PredefinedInstallers.cs:123now reads_environmentContext.Architectureinstead of the raw static call, restoring mockability. Covered by the newPowershell7_On_Windows_Uses_Context_Architecturetheory inPredefinedInstallersTests.cs, which asserts both X86 and X64 select the correct MSI URL.EnvironmentContext'sFolderproperties throwing on blank input — the author chose to make this an explicit, deliberate contract:EnvironmentContext.csnow callsArgumentException.ThrowIfNullOrWhiteSpace(contentRootPath)before constructingContentDirectory, rather than silently degrading. This is a reasonable resolution givenContentDirectoryis now non-nullable, and it's covered by the newConstructor_Rejects_Blank_Content_Roottest.
I also re-traced the rest of the consolidation at this head: IBuildSystemDetector is now correctly internal (resolving the CodeRabbit pre-merge inconclusive check about its accessibility), GitVersioning continues to thread IFileSystemProvider through Folder.CreateTemporaryFolder(provider) with dedicated coverage in the new GitVersioningTests.cs, FilesContext.GetFile/GetFolder validate blank paths via ArgumentException.ThrowIfNullOrWhiteSpace, and the LegacyContextInterfaces_ShouldNotExist test now asserts IEnvironmentContext/IFileSystemContext/IInstaller no longer exist under the old ModularPipelines.Context namespace. No stale references to any removed/renamed types (IEnvironmentDomainContext, EnvironmentDomainContext, IFileSystemContext, IInstaller, OperatingSystemIdentifier) remain anywhere in src, test, or docs.
No new issues found in this round.
b0b4ce0 to
0f564e5
Compare
GitHub Actions
Inactive
Track the context API consolidation merged in #4248 after rebasing the public API gate.
Track the context API consolidation merged in #4248 after rebasing the public API gate.
1 participant
Summary
IEnvironmentContextand expose richFolderpaths plus host metadataTest plan
ModularPipelines.Tests.slnfRelease buildCloses #4227
Summary by CodeRabbit
New Features
Changes
Documentation