Improve fallout-migrate: NuGet-scoped version pin, TFM warning/bump, confirmation prompt, async pipeline#509
Improve fallout-migrate: NuGet-scoped version pin, TFM warning/bump, confirmation prompt, async pipeline#509dennisdoomen wants to merge 13 commits into
Conversation
Migration.Run() hard-coded four migration actions as private methods sharing private helpers, making it awkward to add the steps planned next. Split into: - Common/ -- MigrationContext (plain data), Summary, RewriteResult, IMigrationStep, and MigrationFileOperations (shared EnumerateFiles / ApplyRewrite / RelativePath helpers) - Steps/ -- one class per step (ResolveFalloutVersionStep, RewriteCsprojsStep, RewriteCsFilesStep, RewriteBootstrapScriptsStep, RenameNukeDirectoryStep), each paired with the rewriter it drives (CsprojRewriter, CodeRewriter, ScriptRewriter) Migration.Run() now just builds a MigrationContext and iterates a fixed, ordered step list. Adding a future step is one new class plus one line in that list. No behavior change; MigrationIntegrationSpecs pass unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the plain Console.WriteLine calls in MigrateCommand's banner and summary printing with Spectre.Console's AnsiConsole: bold banner, yellow dry-run notice and warnings, green completion messages, dimmed guide link. Interpolated values (paths, warning text) go through MarkupLineInterpolated so user-controlled content can't be misinterpreted as markup. Left Console.Error.WriteLine for the two error messages (stderr semantics matter for piping/exit codes) and the TextWriter-based per-file/per-rename logging in the Steps/Common classes untouched -- that log stays plain text so MigrationIntegrationSpecs can keep capturing it via TextWriter.Null without a console. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ajor ResolveFalloutVersionStep now queries NuGet's flat-container index for the latest non-prerelease Fallout.Common release matching the running tool's own major (calendar year), instead of only reading the tool's own AssemblyInformationalVersion. This keeps a migration pinned to what's actually installable without jumping to a newer, potentially breaking yearly major the tool wasn't built against. Falls back to the tool's own version when NuGet can't be reached or has no matching-major stable release yet, and logs via AnsiConsole which path was taken so the resolved version is never a silent guess. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fallout's tooling is built and tested against .NET 10; a build project still on an older TFM can hit tool incompatibilities that this migration's other, purely textual rewrites won't catch. VerifyBuildTargetFrameworkStep reads _build.csproj's TargetFramework(s) and adds a warning for any moniker that isn't a modern net10.0+ (catching net8.0, net48, netstandard2.0, netcoreapp3.1, etc.). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ConfirmMigrationStep runs immediately before RewriteCsprojsStep, the first step that mutates files, and prompts via AnsiConsole.Confirm before letting the migration proceed. Declining sets the new Summary.Cancelled flag, which Migration.Run checks to stop executing further steps, and MigrateCommand returns exit code 1 without printing the (now meaningless) summary. The prompt is skipped for --dry-run (nothing would be written) and when stdin is redirected (CI, piped input, automated tests) since there's nothing to prompt against. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CsprojRewriter.Rewrite had a single caller (RewriteCsprojsStep). Move its regex fields and Rewrite method directly into the step and delete the now-empty class. Rename the spec file/class to match. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add BumpDotNetVersionStep: rewrites _build.csproj's TargetFramework(s) to net10.0 and global.json's sdk.version to 10.0.100 during migration, matching the versions Fallout's own tooling targets. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| { | ||
| try | ||
| { | ||
| using HttpResponseMessage response = httpClient.GetAsync(FlatContainerIndex).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Can this be "real async" here?
There was a problem hiding this comment.
Yeah, was thinking about that as well.
BumpDotNetVersionStep previously overwrote any TargetFramework/SDK version that wasn't an exact match, including newer ones. Now it compares against the minimum (net10.0 / 10.0.100) and leaves anything already at or above it untouched. Multi-targeting bumps only the monikers that are behind. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Build projects don't multi-target, so only match the singular TargetFramework element instead of splitting/rejoining a TargetFrameworks list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
MinimumSupportedMajor (10) had to be kept in sync by hand with TargetFramework (net10.0). Parse it from the constant instead so there's a single source of truth. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the blocking GetAsync(...).GetAwaiter().GetResult() in ResolveFalloutVersionStep with a proper await. IMigrationStep.Execute is now Task ExecuteAsync; Migration.Run is Migration.RunAsync; MigrateCommand is now an AsyncCommand<MigrateSettings>; Program.Main awaits CommandApp.RunAsync. ConfirmMigrationStep now awaits ConfirmationPrompt.ShowAsync instead of the blocking AnsiConsole.Confirm. Steps with no async work simply return Task.CompletedTask. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…nvocation order, tag NuGet requests Extract the modern-moniker regex and IsOlderThanMinimumSupported logic duplicated between BumpDotNetVersionStep and VerifyBuildTargetFrameworkStep into a shared TargetFrameworkMonikers helper. VerifyBuildTargetFrameworkStep now reads BumpDotNetVersionStep.MinimumSupportedMajor instead of maintaining its own separate constant. Reorder ResolveFalloutVersionStep's private methods to match the order they're invoked from ExecuteAsync (assembly, then major, then NuGet). Add a User-Agent header to the NuGet flat-container HttpClient so requests aren't sent as anonymous script traffic. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ChrisonSimtian
left a comment
There was a problem hiding this comment.
some small things :-)
There was a problem hiding this comment.
probably something that could move a bit closer to the core instead of sitting in this satellite. Looks useful :-)
| /// <summary> | ||
| /// The version to fall back to when the assembly carries no build-metadata suffix. | ||
| /// </summary> | ||
| private const string Fallback = "10.3.49"; |
There was a problem hiding this comment.
how do we remember to bump this with a new release?
Should this be more central? collection of constants somewhere grouped together as a one-stop-shop maybe?
|
|
||
| /// <inheritdoc /> | ||
| public void Execute(MigrationContext context, Summary summary) | ||
| public async Task ExecuteAsync(MigrationContext context, Summary summary) |
There was a problem hiding this comment.
no CancellationToken?
| string localVersion = ResolveFromAssembly(); | ||
| int localMajor = ExtractMajor(localVersion); | ||
|
|
||
| string nuGetVersion = await ResolveFromNuGetAsync(localMajor); |
There was a problem hiding this comment.
no CancellationToken?
| /// <param name="major">The major version (calendar year) to restrict the lookup to.</param> | ||
| /// <returns>The latest published stable version within <paramref name="major"/>, or <c>null</c> | ||
| /// if NuGet couldn't be reached or has no matching-major stable version.</returns> | ||
| private static async Task<string> ResolveFromNuGetAsync(int major) |
There was a problem hiding this comment.
no CancellationToken?
| { | ||
| try | ||
| { | ||
| using HttpResponseMessage response = await httpClient.GetAsync(FlatContainerIndex); |
There was a problem hiding this comment.
pretty sure this one lets us pass through a token, mind having a look at that and passing it all the way through if so?
| /// script traffic. | ||
| /// </summary> | ||
| /// <returns>A configured <see cref="HttpClient"/>.</returns> | ||
| private static HttpClient CreateHttpClient() |
There was a problem hiding this comment.
nit: can we make this a factory instead?
38d0437 to
8affb45
Compare
Summary
A set of focused improvements to
fallout-migrate: pin the migratedFallout.Commonreference to the latest NuGet release instead of the running tool's own version, warn and optionally bump an outdated build-project TFM, ask for confirmation before writing files, and convert the migration pipeline to async.Changes
Bump Fallout.Migrate to the 10.x version line — version bump.
Resolve Fallout.Common version from NuGet, scoped to the tool's own major —
ResolveFalloutVersionStepnow queries NuGet's flat-container index for the latest non-prereleaseFallout.Commonrelease matching the running tool's own major (calendar year), instead of only reading the tool's ownAssemblyInformationalVersion. Falls back to the tool's own version when NuGet can't be reached or has no matching-major stable release yet.Warn when the build project targets an older TFM than .NET 10 —
VerifyBuildTargetFrameworkStepreads_build.csproj'sTargetFramework(s)and warns for any moniker older thannet10.0.Ask for confirmation before the migration writes any files —
ConfirmMigrationStepprompts viaAnsiConsole.Confirmimmediately before the first file-mutating step. Declining setsSummary.Cancelledand stops the pipeline. Skipped for--dry-runand when stdin is redirected.Fold CsprojRewriter into RewriteCsprojsStep —
CsprojRewriter.Rewritehad a single caller; merged directly into the step.Bump build project to net10.0 and global.json SDK to 10.0.100 —
BumpDotNetVersionSteprewrites_build.csproj'sTargetFrameworkandglobal.json'ssdk.versionwhen either is behind the minimum, leaving already-current or newer values untouched. Build projects don't multi-target, so only the singularTargetFrameworkelement is matched.Make IMigrationStep and the migration pipeline asynchronous —
IMigrationStep.Executeis nowTask ExecuteAsync; replaces the blockingGetAsync(...).GetAwaiter().GetResult()inResolveFalloutVersionStepwith a properawait.Deduplicate the TFM-minimum check shared by two migration steps — extract the modern-moniker regex and minimum-check logic duplicated between
BumpDotNetVersionStepandVerifyBuildTargetFrameworkStepinto a sharedTargetFrameworkMonikershelper.Reorder ResolveFalloutVersionStep by invocation order and tag NuGet requests — reorder private methods to match invocation order; add a
User-Agentheader to the NuGet flat-containerHttpClientso requests aren't sent as anonymous script traffic.Combined effect
fallout-migratenow pins migrated references to a real, publishedFallout.Commonrelease, flags/fixes an outdated build-project TFM, confirms before mutating files, and runs its pipeline asynchronously.