Refresh Azure CLI options from current output - #4546
Conversation
thomhurst
added
dependencies
|
@claude Please review this generated Azure CLI synchronization. |
|
Too many files changed for review (3000 files, 100 file limit). |
|
Caution CodeRabbit couldn't post its review summary. Error details |
GitHub Actions
Inactive
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refresh Azure CLI options from current output (#4546)
Scope note: GitHub's diff view truncates at 300 files; this PR touches ~6,260 files. I reconstructed the full diff locally via git diff be8fc008^ be8fc008 (single commit, chore(azure): refresh generated options). It consists of ~6,260 auto-generated *.Generated.cs files rebuilt from current az CLI help output, one generated coverage JSON, and hand-written updates to test/ModularPipelines.Azure.UnitTests/AzureCommandTests.cs for the new required positional constructors and renamed ACR properties (ResourceGroupValue→ResourceGroup, TargetValue→Target, plus removal of the old [Obsolete] bool shims).
Verified clean:
- No duplicate property/constructor-parameter names introduced (no compile breakage from the new required-positional-parameter constructors).
- No other source in the repo still references the old ACR property names or constructs the newly-required-constructor types via object initializers that would now fail to compile.
AzureCommandTests.csis internally consistent with the regenerated options it exercises.- The regenerated coverage JSON is well-formed.
- This aligns with CLAUDE.md's guidance that "current tool output is the sole source of truth for generated options" — no hand-preserved shims were kept, which is correct per project convention.
Finding (BLOCKING): the refresh is inconsistent within the very same generated records — some value-taking options were correctly promoted from CliFlag bool? to CliOption, while sibling options with identically-worded doc comments were left as broken boolean flags.
-
src/ModularPipelines.Azure/Options/AzAcrBuildOptions.Generated.cs(lines ~34, ~91) —BuildArgandSecretBuildArgremain[CliFlag("--build-arg")] public bool? BuildArgeven though their doc comments explicitly describe aname[=value]argument ("Build argument in '--build-arg name[=value]' format..."). In this exact same file/commit,AuthMode,Image, andSourceAcrAuthIdwere correctly reclassified fromCliFlag/bool?toCliOption/string?for the same reason. SettingBuildArg = trueemits the bare--build-argswitch with no value — a silently brokenaz acr buildinvocation. -
src/ModularPipelines.Azure/Options/AzSynapseSparkJobSubmitOptions.Generated.cs(lines ~55-65) —MainClassName,Configuration, andReferenceFilesremainCliFlag/bool?despite doc comments describing value-carrying arguments (a class identifier, a job configuration, a file list). This same commit correctly fixedArchives,Language,PythonFiles, andTagsin the identical record fromCliFlag/bool?toCliOption/string?orIEnumerable<string>?.
Why this matters / suggested approach: Per CLAUDE.md, the fix belongs in the scraper/generator (tools/ModularPipelines.OptionsGenerator/), not in hand-edits to the generated files. The generator's flag-vs-option classification heuristic appears to key off some signal (likely presence of an explicit value placeholder or specific phrasing) that misses arguments described via name[=value]-style syntax or multi-word value descriptions — worth checking whether the classifier is keying off the CLI help's argument-metavar pattern and failing for these non-standard forms. Since this PR's diff shows the generator getting some properties in a record right and others in the same record wrong, this looks like a narrow gap in that heuristic rather than a systemic breakage, but it should be fixed in the generator so it doesn't need to be manually re-caught on every future refresh. A heuristic scan for the same doc-text-vs-attribute mismatch pattern across the diff surfaced this as widespread (order of 100+ occurrences), so an isolated manual patch of just these two files would not be sufficient — the generator itself needs the fix, then a re-run of the refresh.
|
Addressed the exact-head Claude blocker at 01f8860. Extended the Azure scraper—not generated files—to recognize description-only value families, including build/secret arguments, paths, platforms, timeouts, configurations, class identifiers, commands, and file collections; added numeric/list cardinality detection and exact ACR/Synapse regression tests. Validation: Azure scraper tests 26/26; full generator tests 1,265/1,265. A full local Azure regeneration reached command 413 but hit the repository-mandated 2-GB process-tree guard, so I did not raise/retry it. Dispatched the 60-minute CI generator on this exact branch to produce the complete regenerated artifact set; auto-merge is disabled. |
|
Caution CodeRabbit couldn't post its review summary. Error details |
GitHub Actions
Failure
There was a problem hiding this comment.
Review: Refresh Azure CLI options from current output (#4546)
Re-review scope: since the last Claude review (head be8fc008), this branch adds exactly one commit, 01f886096b ("fix(azure): detect description-only values"), touching only AzCliScraper.cs and its test file — no *.Generated.cs files changed. I diffed be8fc008..01f886096b directly to confirm this.
The prior BLOCKING finding is not actually resolved yet.
The PR comment states the scraper was extended to "recognize description-only value families" and that CI was dispatched to "produce the complete regenerated artifact set." That scraper fix is real, narrowly scoped, and well covered by two new tests (Nonstandard_Description_Only_Values_Are_Not_Flags, Synapse_Description_Only_Values_Are_Not_Flags) — this is the right place to fix it per CLAUDE.md ("update the scraper or generator", not hand-edit generated files). However, the regenerated output implied by those tests has not actually landed on this branch. At the current head (01f886096b8df9b4ae49d6283a91f23c1d5f8fe0), the exact same generated files from the original finding are unchanged and still broken:
AzAcrBuildOptions.Generated.cs:BuildArg,SecretBuildArg,File,Platform, andTimeoutare all still[CliFlag] public bool?despite value-carrying descriptions (the new scraper test literally asserts these should now beIEnumerable<string>?/string?/int?):
AzSynapseSparkJobSubmitOptions.Generated.cs:Configuration,MainClassName, andReferenceFilesare likewise still[CliFlag] public bool?:
Setting BuildArg = true (or Configuration = true, etc.) still emits a bare switch with no value — the same silently-broken CLI invocation as before. Also worth noting: File, which shares the same bug pattern, wasn't called out in the original finding but is covered by the new scraper test, and it's still broken in the committed output too — reinforcing that this is a mechanical "generator changed, artifacts didn't" gap rather than something that needs more heuristic work.
Why this matters / suggested approach: the fix isn't done until the regenerated *.Generated.cs files are actually committed to this branch — a scraper change with no corresponding regeneration leaves the shipped bug in place regardless of how correct the scraper logic now is. Since local regeneration hits the repo's 2GB agent memory guard, this has to come from the dispatched CI run's artifact being committed back onto the branch before this can be considered resolved; please push that regenerated output (or confirm the CI run failed/is still pending) rather than merging on the strength of the scraper-only fix.
As a structural improvement worth considering separately from this PR: a lightweight CI check that fails when tools/ModularPipelines.OptionsGenerator/** changes without a corresponding diff in the generated packages (or that just runs the generator and diffs its output against what's committed) would catch this "scraper fixed, artifacts stale" gap automatically, instead of relying on a reviewer to manually diff two heads of a 6,000+ file PR to notice regeneration didn't happen.
|
Ubuntu failure is outside this PR's Azure-generated diff: |
|
Caution CodeRabbit couldn't post its review summary. Error details |
There was a problem hiding this comment.
Review
This PR adds new scraper detection logic (AzCliScraper.cs) for description-only Azure CLI options that don't declare an explicit value placeholder, so they're correctly typed instead of misclassified as boolean flags. The scraper-logic tests (AzCliScraperTests.cs) are well-targeted and pass.
Blocking issue: the fix isn't actually applied to the shipped generated code.
The PR's commits land in this order:
be8fc008—chore(azure): refresh generated options(bulk regeneration, 6263 files)01f88609—fix(azure): detect description-only values(the actual scraper regex fix, touches onlyAzCliScraper.cs+ its tests)f99a80db—ci(options): retain Azure generation patch(CI workflow only)
The regeneration happened before the scraper fix, and nothing in commit 2 or 3 re-runs generation. So the checked-in .Generated.cs files still contain the exact bug this PR claims to fix.
Concrete, verifiable contradiction: the new test Nonstandard_Description_Only_Values_Are_Not_Flags asserts that for az acr build, --build-arg, --secret-build-arg should be IEnumerable<string>?, --platform/--file should be string?, and --timeout should be int?. But src/ModularPipelines.Azure/Options/AzAcrBuildOptions.Generated.cs (shipped in this same PR, via commit 1) still has all four of BuildArg, SecretBuildArg, Platform, Timeout as [CliFlag] bool? presence-only flags:
[CliFlag("--build-arg")]
public bool? BuildArg { get; set; }
...
[CliFlag("--platform")]
public bool? Platform { get; set; }
...
[CliFlag("--secret-build-arg")]
public bool? SecretBuildArg { get; set; }
...
[CliFlag("--timeout")]
public bool? Timeout { get; set; }Notably, other properties in that same file (AuthMode, Image, SourceAcrAuthId, ResourceGroup, Target) were correctly fixed by commit 1 (from an earlier scraper improvement) — so this isn't a case where the file was untouched; it's specifically the new description-only detection pattern from commit 2 that never got applied. Same mismatch pattern applies to AzSynapseSparkJobSubmitOptions.Generated.cs vs. the new Synapse_Description_Only_Values_Are_Not_Flags test (Configuration, MainClassName, ReferenceFiles).
Impact: A consumer calling new AzAcrBuildOptions(...) { Platform = true } cannot actually pass a platform value (e.g. "linux/amd64") to az acr build --platform — the property is still boolean presence-only. The PR's own tests prove the shipped API is wrong; CI is green only because the scraper-level unit tests check the scraper's output in isolation, not that the checked-in generated corpus matches current scraper output.
Suggested fix: re-run the Azure options generation after the scraper fix (i.e., squash/reorder so generation happens after 01f88609, or add a follow-up regeneration commit) so the .Generated.cs files reflect the new detection logic. Given AzDescriptionOnlyValuePattern/AzNumericValueDescriptionPattern/AzCollectionValueDescriptionPattern look like broadly-applicable patterns, it's worth doing a full diff of the regenerated Azure corpus against what's currently checked in — there are likely more than the two files above affected (a substantial number of other [CliFlag] properties elsewhere in the Azure package could match these same patterns).
As a process improvement: consider adding a CI check that fails if the checked-in generated corpus differs from a fresh run of the current generator/scraper — this class of "logic fixed, output not regenerated" bug would be easy to detect automatically before merge rather than requiring a manual cross-reference like this review did.
GitHub Actions
Inactive
…ent-output # Conflicts: # .github/workflows/generate-cli-options.yml
|
Azure generation completed 6,422 files at run 33752281684, then PublicAPI snapshot collection failed before the recovery artifact step. Moved temporary patch staging/upload ahead of that sync and merged current main/provenance changes at exact head 5ef57b6. Replacement authoritative run: https://github.com/thomhurst/ModularPipelines/actions/runs/33766998203 |
|
Caution CodeRabbit couldn't post its review summary. Error details |
There was a problem hiding this comment.
Review: Refresh Azure CLI options from current output (#4546)
Re-review scope: this is the fourth Claude review round on this PR. Three prior reviews (heads be8fc008, 01f886096b, f99a80dba) all landed the same BLOCKING verdict for the same root cause. I re-checked whether it's resolved at the current head, 5ef57b68, and it is not — plus a targeted spot-check across ~20 other generated files surfaced two more instances of the same failure class, including one outright regression.
The previously-flagged blocker is still unresolved
Across three rounds, this PR has claimed the scraper fix (AzCliScraper.cs, commit 01f886096) for "description-only value" options was regenerated into the shipped corpus. It still hasn't been. At the current head:
AzAcrBuildOptions.Generated.cs—BuildArg,File,Platform,SecretBuildArg, andTimeoutare all still[CliFlag] public bool?, even though their own doc comments describe value-carrying arguments ("Build argument in '--build-arg name[=value]' format...","The timeout in seconds.") and the PR's own new scraper test (Nonstandard_Description_Only_Values_Are_Not_Flags) asserts these should beIEnumerable<string>?/string?/int?.AzSynapseSparkJobSubmitOptions.Generated.cs—Configuration,MainClassName,ReferenceFilesare likewise still[CliFlag] bool?, contradicted by the PR's ownSynapse_Description_Only_Values_Are_Not_Flagstest.
Setting e.g. BuildArg = true or Configuration = true still emits a bare switch with no value — the same silently-broken CLI invocation flagged three reviews ago. The comment thread describes several CI regeneration attempts (a run reaching 6,422 files, then failing "before the recovery artifact step") that were apparently never actually applied back onto this branch — the merge to the current head brought in main, not the regenerated Azure output.
Two more instances of the same failure class, one a regression
Spot-checking beyond the two files above (Keyvault, Network, Cosmosdb, Functionapp, Monitor, Storage, Eventhubs):
AzEventhubsNamespaceAuthorizationRuleShowOptions.Generated.cs(newly added) —AuthorizationRuleName("The authorization rule name.") andNamespaceName("The Namespace name.") are generated as[CliFlag] bool?instead of[CliOption] string?, and neither was promoted to the constructor. As written, this record cannot specify which namespace or rule to target at all — worse than "drops a value," this command is unusable. The siblingAzEventhubsEventhubConsumerGroupDeleteOptions.Generated.cshas the identical pattern forConsumerGroupName/EventhubName/NamespaceName, andAzMonitorAccountCreateOptions.Generated.cs'sLocationshows the same misclassification — this looks systemic for at least theeventhubsextension command family, not isolated to the two files already reported.AzAcrTaskCreateOptions.Generated.cs— this is an actual regression, not just an unresolved gap: onmain,ImageisImageValues : IEnumerable<string>?(correctly multi-valued, with a deprecated bool shim for back-compat). On this branch it was narrowed toImage : string?— single-valued — while the doc comment is unchanged and still reads "Multiple tags are supported by passing -t multiple times." This refresh makes an already-correct property worse.
Why this matters / suggested approach
Per CLAUDE.md, fixes belong in the scraper/generator, and "current tool output is the sole source of truth" — so a breaking regen is expected and fine on its own. The problem here isn't the scraper logic (which is narrowly scoped and well-tested in isolation); it's that the shipped .Generated.cs corpus in this PR doesn't reflect that scraper's own output, in both directions (some fixes never applied, and at least one prior-correct property got worse). Given three review rounds have now independently hand-verified the same "generator changed, artifacts stale" gap on different file pairs, this isn't something a reviewer should need to keep manually spot-checking — as suggested in the prior review, a CI gate that runs the generator and fails if its output differs from what's committed (or a required "generator diff is empty" check gating merge) would catch this mechanically before every future refresh, rather than relying on someone diffing two heads of a 6,000+ file PR by hand each time.
Minor, non-blocking
.github/workflows/generate-cli-options.yml still carries the "Stage generated Azure output for recovery" / "Preserve generated Azure patch" / "Upload generated Azure patch" steps added purely to recover from this PR's own troubled regeneration attempts, hardcoded to matrix.tool == 'az' only (no other of the ~40 tools in this matrix has an equivalent). The PR's own comment thread says "the temporary workflow instrumentation will be reverted after applying the exact generated output" — worth reverting now that generation is expected to land, or generalizing into the matrix if it's meant to be permanent, rather than leaving a one-off special case for az.
Looked clean
Required-parameter promotion into record constructors (e.g. AzAcrCreateOptions), removal of obsolete bool shims, the AzureCommandTests.cs/AzureCommandTests updates for renamed/positional ACR properties, AzExtensions.Generated.cs's service registrations (renames like AzComputefleet→AzComputeFleet are consistent, additions match new commands), and Az.CommandCoverage.json (valid, commandCount/commands.length both 5131, matches the PR's claimed figures).
GitHub Actions
Failure
|
#4609 (empty-baseline handling in the shared sync script) has merged, so the Azure regeneration that previously died in |
|
Cancelled the just-dispatched |
1 participant
Part of #4331.
Refreshes the generated Azure integration from Azure CLI 2.90.0 output.
Changes:
Validation:
git diff --checkclean