Flatten the core installer API - #4276
Conversation
Drop platform and predefined installer wrappers in favor of dedicated tool integrations. Keep generic local and web installation directly on IInstallersContext.
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (43)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Greptile SummaryThe PR flattens the core installer API by exposing generic local and web installation directly through
Confidence Score: 5/5The PR appears safe to merge, with the flattened installer service resolving correctly and no changed-code regression identified. The generic installer implementation preserves the prior execution flow, its command, downloader, and Bash dependencies are registered with compatible lifetimes, and no surviving source consumer depends on the deleted wrappers.
|
| Filename | Overview |
|---|---|
| src/ModularPipelines/Context/Domains/IInstallersContext.cs | Replaces nested platform installer properties with direct generic local and web installation methods. |
| src/ModularPipelines/Context/Domains/Implementations/InstallersContext.cs | Relocates the existing file-installer execution and download flow without introducing a changed runtime failure. |
| src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs | Removes obsolete wrapper registrations while retaining compatible scoped registrations for the flattened installer and its dependencies. |
| src/ModularPipelines/PublicAPI.Shipped.txt | Records the intentional removal of platform-specific installer contracts and option types from the v4 public surface. |
| RELEASE_NOTES_V4.md | Documents the flattened API and directs consumers toward dedicated tool integrations for removed package-manager functionality. |
| test/ModularPipelines.UnitTests/Helpers/FileInstallerTests.cs | Updates focused installer tests to exercise the flattened context API and web-to-local delegation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
M[Pipeline or module context] --> I[IInstallersContext]
I --> L[InstallAsync]
I --> W[InstallFromWebAsync]
W --> D[IDownloaderContext]
D --> L
L --> O{Operating system}
O -->|Windows| C[ICommandContext]
O -->|Linux or macOS| X[chmod via IBashContext]
X --> B[Run Bash file]
Reviews (1): Last reviewed commit: "refactor(installers)!: flatten core API" | Re-trigger Greptile
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 this PR flattening the installer API (IInstallersContext.InstallAsync/InstallFromWebAsync) and removing the platform-specific installer wrappers (ILinuxInstallerContext, IMacInstallerContext, IWindowsInstallerContext, IPredefinedInstallersContext, AptGet, etc.).
Findings:
InstallersContextis a faithful, direct move of the previousFileInstallerlogic — the execution flow (download → chmod on Linux/macOS via Bash, run via Command on Windows) is preserved with no behavioral regressions.- All call sites, DI registrations, and public API tracking files (
PublicAPI.Shipped.txt/PublicAPI.Unshipped.txt) were updated consistently with the removed surface. - Test coverage was moved rather than dropped —
FileInstallerTestsnow exercises the flattened API, and entries were correctly relocated between the relevant test lists (no coverage gap introduced). RELEASE_NOTES_V4.mddocuments the breaking change and points consumers with a need for package-manager-specific installers (apt/brew/msi/exe) toward dedicated tool integrations, which is a reasonable migration story for a v4 breaking change.
No architectural or design concerns beyond what's already noted in the release notes as an intentional flattening. This is a clean, well-scoped removal — no actionable issues found.
1 participant
Summary
Validation
Closes #4237