[Automated] Update liquibase CLI Options - #4026
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 regenerates the Liquibase CLI integration and now records a machine-readable Liquibase version.
Confidence Score: 5/5The PR appears safe to merge because the previously reported malformed Liquibase version metadata has been corrected and no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/ModularPipelines.Liquibase/Generated/Liquibase.CommandCoverage.json | The coverage manifest now records the machine-readable Liquibase version 5.0.3, resolving the previous malformed-version finding. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/LiquibaseCliScraper.cs | Adds Liquibase-specific extraction of a stable release identifier from CLI version output. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/Cli/LiquibaseCliScraperTests.cs | Verifies extraction of 5.0.3 from representative Liquibase version output. |
| src/ModularPipelines.Liquibase/Services/ILiquibase.Generated.cs | Regenerates the command interface with compatibility behavior for renamed members. |
| src/ModularPipelines.Liquibase/Options/LiquibaseOptions.Generated.cs | Updates generated option naming while retaining obsolete aliases for source compatibility. |
Reviews (7): Last reviewed commit: "fix(liquibase): preserve regenerated API" | Re-trigger Greptile
There was a problem hiding this comment.
Code Review
This is an automated regeneration of ModularPipelines.Liquibase from a fresh scrape of the liquibase CLI. The scrape data driving this PR appears to be corrupted, and it produced a breaking API change that's inconsistent with the compatibility pattern used elsewhere in the same PR.
🔴 Scrape corruption (root cause)
src/ModularPipelines.Liquibase/Generated/Liquibase.CommandCoverage.json (new toolVersion field)
"toolVersion": "#################################################### ## _ _ ...This is liquibase's ASCII-art startup banner, not a version string — clear evidence the CLI output used to drive this regeneration was mis-parsed/corrupted rather than a clean --help/version capture.
🔴 Mass removal of legitimate, still-supported options
Consistent with that corruption, ~46 well-documented, currently-supported liquibase global options were converted to no-op [Obsolete] properties with the [CliOption] attribute stripped — e.g. across LiquibaseOptions.Generated.cs and derived option classes:
[Obsolete("LiquibaseCatalogName is no longer supported by the installed CLI and has no effect.")]
public virtual string? LiquibaseCatalogName { get; set; }
[Obsolete("ChangelogLockPollRate is no longer supported by the installed CLI and has no effect.")]
public virtual int? ChangelogLockPollRate { get; set; }
[Obsolete("DatabaseChangelogTableName is no longer supported by the installed CLI and has no effect.")]
public virtual string? DatabaseChangelogTableName { get; set; }Same pattern hits ReferencePassword on LiquibaseDiffChangelogOptions (was a real [SecretValue]-annotated --reference-password option, now a bare obsolete no-op). These flags are all still documented as valid liquibase options. Failure mode: code compiles (with an Obsolete warning) but the corresponding CLI flag is silently dropped from the generated command line — e.g. a diff-changelog run against a secured reference database would silently fail to pass --reference-password and fail auth with no indication why.
Suggestion: re-run the scrape against a clean CLI invocation (capture stdout without the banner, or use --version/a dedicated version flag), regenerate, and diff against the previous generated output to sanity-check that removals are real deprecations, not scrape artifacts. Given the volume of removals across nearly every options class, I'd hold this PR until the source data is re-verified.
🟠 Breaking rename without the compatibility shim used elsewhere in this PR
src/ModularPipelines.Liquibase/Services/ILiquibase.Generated.cs
MarkNextChangesetRanAsync / MarkNextChangesetRanSqlAsync were renamed to MarkNextChangeSetRanAsync / MarkNextChangeSetRanSqlAsync with no backward-compatible overload/shim — unlike renamed properties elsewhere in this same PR (e.g. ChangesetId → ChangeSetId), which kept an [Obsolete]-forwarding member for compatibility. Existing caller code invoking the old method names will fail to compile after upgrading, with no deprecation path. This lines up with the PR description's own "API compatibility gate: failure" note.
Suggestion: apply the same Obsolete-forwarding pattern used for the property renames to these two service methods, or clarify in the generator why method renames are treated differently from property renames — if that's intentional, it's worth documenting since it's a real breaking change for consumers.
Summary
The mechanical regeneration process itself is sound (structure, obsolete-shim pattern for properties, etc.), but the input data for this specific run looks corrupted and should not be merged as-is. Recommend re-scraping against a clean CLI version string and re-running the generator before merging, and adding the same compatibility shim to the two renamed service methods.
|
Blocked on #4027, which corrects tool-specific version probes. After it lands, regenerate Liquibase; do not hand-edit |
548ed8d to
863dfa6
Compare
There was a problem hiding this comment.
Code review
Summary: Automated regeneration of the ModularPipelines.Liquibase options surface (option/enum classes, service methods, command-coverage metadata, and docs) from a fresh scrape of the installed liquibase CLI's help output.
Finding: this regeneration is based on a broken CLI version probe and should not be merged yet
The captured toolVersion in src/ModularPipelines.Liquibase/Generated/Liquibase.CommandCoverage.json is not a version string — it's the raw ASCII-art banner/logo that liquibase --version prints to stdout:
"toolVersion": "#### ## _ _ ... \\_____/_|\\__, |\\__,_|_|_.__/ \\__,_|___/\\___| ..."That confirms exactly the bug described in #4027 ("Use valid CLI version probes", still open, not merged). Because the generator couldn't parse a real version, it appears to have made incorrect decisions about option support:
- ~254 properties across
LiquibaseOptions.Generated.csand other global option classes were newly marked[Obsolete("... is no longer supported by the installed CLI and has no effect.")](e.g.AllowDuplicatedChangesetIdentifiers,AnalyticsEnabled,ChangelogLockPollRate, etc.) — most of these are legitimate, documented Liquibase global options, not actually removed from the CLI. - Some per-command options classes lost properties outright rather than being deprecated — e.g.
LiquibaseCalculateChecksumOptionsno longer hasChangesetAuthor,ChangesetIdentifier,DefaultCatalogName, orDefaultSchemaName. That's a real breaking removal from public API surface, which lines up with the PR description's own self-reportedAPI compatibility gate: failure. - The Liquibase CLI reference docs (
docs/docs/mp-packages/cli/liquibase.md) lost the majority of the global options table rows and the runnable module example, for the same reason.
This is also exactly what @thomhurst flagged in a review comment on this PR already: "Blocked on #4027, which corrects tool-specific version probes. After it lands, regenerate Liquibase; do not hand-edit Generated/Liquibase.CommandCoverage.json." #4027 is still open, so this PR predates its own stated prerequisite.
Suggested approach: Hold/close this PR (or convert to draft) until #4027 merges, then re-run the generator so it captures a real toolVersion and the obsoletion/removal decisions are made against accurate CLI capability data, rather than shipping ~254 incorrect [Obsolete] markers and a handful of real breaking removals now and needing a follow-up correction PR later.
One smaller, separate point worth a sanity check: ChangesetId/ChangesetPath were renamed to ChangeSetId/ChangeSetPath in this pass. If that casing fix is intentional (aligning with the rest of the "ChangeSet"-prefixed members), that's fine, but it's also a breaking rename for existing consumers and worth calling out explicitly in the PR description alongside the other compatibility-gate failures.
863dfa6 to
45febf8
Compare
There was a problem hiding this comment.
Code review
Status: unresolved from the last two review passes. The branch was force-pushed twice after my prior comments (21:57 and 22:23 UTC), but the current HEAD (45febf86) reproduces the exact same corrupted regeneration — nothing was fixed:
src/ModularPipelines.Liquibase/Generated/Liquibase.CommandCoverage.jsonstill capturestoolVersionas Liquibase's ASCII-art startup banner instead of a version string, confirming the scrape input is still garbage, not a clean--versionprobe.- The same 254 properties across the global/per-command options classes are still stamped
[Obsolete("... is no longer supported by the installed CLI and has no effect.")]— identical count to the previous pass, i.e. this is the same bad decision being replayed from the same corrupted input, not a fresh (and differently-wrong) run. MarkNextChangesetRanAsync/MarkNextChangesetRanSqlAsyncare still renamed toMarkNextChangeSetRanAsync/MarkNextChangeSetRanSqlAsyncwith no[Obsolete]-forwarding shim, unlike the property renames elsewhere in the same generated surface — still a hard compile break for existing callers.- The PR body still self-reports
API compatibility gate: **failure**.
Recommend closing/holding this PR (as before) until #4027 lands and a clean regeneration is produced.
Architectural suggestion: stop relying on repeated manual/AI review to catch this
This is now the third time the identical corrupted-scrape output has produced a PR for review. The workflow (.github/workflows/generate-cli-options.yml) already has an API-compatibility gate that blocks auto-merge on failure (steps.api-compat.outcome == 'failure'), but it still opens the PR — so every re-run of the automated job against the same broken liquibase --version probe burns another human/AI review cycle re-discovering the same root cause.
A more robust fix than only tracking this via #4027 is to make the input validation itself a hard pipeline gate, not just the output compatibility check:
- Before generation runs, validate that the captured
ToolVersion(CliToolDefinition.ToolVersion/CommandCoverageGuard) matches a plausible version pattern (e.g.^\d+(\.\d+){1,3}). If it doesn't, fail the job immediately with a clear "version probe returned non-version output" error, instead of feeding the garbled string through the full generator and letting it silently make ~254 incorrect obsoletion decisions. - This turns "scrape produced garbage" into a fast, self-explanatory CI failure at the source, rather than a large, misleading diff that a reviewer has to reverse-engineer from first principles each time the workflow happens to re-run before #4027 merges.
This is a general hardening for any tool's regeneration (not just Liquibase) — the same ToolVersion capture path is shared across all CLI tools driven by this generator, so the guard belongs in CommandCoverageGuard/CliToolDefinition rather than being special-cased per tool.
25bf720 to
b496ae3
Compare
Review: [Automated] Update liquibase CLI Options (#4026)This is an auto-generated CLI-options refresh, but the diff surfaces two real problems in the generator, not just cosmetic regen noise. No prior substantive review comments exist to reconcile against (only bot usage-limit/skip notices). 1. Uncompensated breaking rename — likely root cause of the failing API-compat gate
with no What makes this worth fixing at the generator level rather than waving through: the generator already has the right mechanism for exactly this case. Given this, this PR shouldn't be merged as-is — either the generator needs to actually apply the casing-preservation shim for these two methods, or a manual compat overload needs to be added to the non-generated partial before merging. 2. Corrupted
|
|
Local regeneration against Liquibase 5.0.3 and current |
|
Regenerated Liquibase 5.0.3 from a clean current-main baseline using the positional parser from #4003; no generated file was hand-edited. Generator completed 43 commands / 61 files / zero errors, API compatibility validation passed, manifest artifacts were copied byte-for-byte, and diff checks are clean. The full Liquibase solution and narrowed package build both exceeded the fixed 2GB agent guard (exit 137 at 2343MB and 2265MB); I did not raise the limit or retry further, so CI owns compilation validation. Dependency: do not merge this generated output before #4003. Verified remote head: 0774466. |
Code reviewCompared this against the prior review thread on this PR (the earlier The uncompensated breaking rename from the earlier review is still presentAt the current head (
This is inconsistent with how this same PR handles removed CLI options elsewhere (kept as Root cause, with a concrete lead: method.OptionsType.Equals(command.ClassName, StringComparison.Ordinal)
&& method.MethodName.Equals(currentMethodName, StringComparison.OrdinalIgnoreCase)
&& !method.MethodName.Equals(currentMethodName, StringComparison.Ordinal)The Separately, Why this matters architecturally, not just cosmetically: any pipeline calling Recommendation: don't merge until either (a) the generator emits proper What's fixed since the last review
|
Regenerate from the clean baseline and retain casing-compatible aliases.\n\nRefs #4026
0774466 to
413923a
Compare
|
@claude Please re-review exact head 413923a. The casing break is fixed by regenerating from exact clean target base 036032f rather than the contaminated first regeneration baseline. The generator now retains LiquibaseMarkNextChangesetRanOptions, emits obsolete MarkNextChangesetRanAsync/Sql forwarders beside the new ChangeSet-cased methods, and emits obsolete forwarding properties such as ChangesetAuthor and AllowDuplicatedChangesetIdentifiers. I also committed Liquibase source support for shared usage operands and stable 5.0.3 version parsing, with regressions. Three no-shrinkage generation runs were stable at 43 commands / 61 files / tree 41e289cc33bbedcb50c94a7ac63a85c7d6ddacbd20208a60c02222e6d2d3c647; the post-rebase run left the worktree clean. OptionsGenerator tests: 974/974. The Liquibase solution build reached the repository guard's 2 GB process-tree limit (2637 MB), so per AGENTS.md I did not raise/retry it; CI will perform that expensive validation. |
6ed8704
into
issue-3996-combined-generation-validation-20260823
Summary
This PR contains automatically generated updates to liquibase CLI options classes.
The generator scraped the latest CLI help output from the installed tool.
Changes
Command coverage
Command coverage report:
| | | | | '_ \ / _/ |/ _ \ ## ## | || | (| | || | | |) | (| __ \ / ## ## _/|_, |_,||./ _,|/_| ## ## | | ## ## || ## ## ): 43 commands, tree 41e289cc33bbedcb50c94a7ac63a85c7d6ddacbd20208a60c02222e6d2d3c647Verification
🤖 Generated with ModularPipelines.OptionsGenerator