Conversation
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. |
|
Caution CodeRabbit couldn't post its review summary. Error details |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Review
This PR regenerates the ModularPipelines.Google (gcloud) options library against gcloud SDK 585.0.0, adding 24 new commands and applying fixes (from prior PRs #5328/#5331/#5332) for nested resource-group constructors, conditional/mutually-exclusive requirement groups, comma-delimited vs. repeated list rendering, per-command enum collisions, and secret-value classification. The bulk of the ~7,700-file diff is unmodified generated output (Enums/Options/Commands/Settings under src/ModularPipelines.Google) plus a generated docs table update; the only handwritten changes are 6 test files under test/ModularPipelines.Google.UnitTests adapting to now-required resource constructor parameters and adding coverage for the fixed behaviors.
Review evidence
test/ModularPipelines.Google.UnitTests/GcloudRegenerationTests.cs: New test file covering the specific defect classes described in the PR (conditional schema/encoding validation, iceberg catalog+storage bundle requirement, sql config group independence, secret vs. non-secret property classification, provider-specific requirement branches, resource-value rendering); each assertion was cross-checked against the corresponding regenerated Options class and matched.
test/ModularPipelines.Google.UnitTests/GcloudResourceArgumentTests.cs: New reflection-based test verifying resource-group constructors (e.g. GcloudKmsKeyringsDeleteOptions, GcloudMetastoreServicesMigrationsDescribeOptions/DeleteOptions) now require their positional resource string and throw ArgumentNullException when null; confirmed against generated constructors.
test/ModularPipelines.Google.UnitTests/GcloudDelimitedListTests.cs: New test distinguishing comma-joined list options (CollectionSeparator set) from repeated-flag list options (no CollectionSeparator); verified against GcloudMetastoreServicesMigrationsStartOptions and GcloudDnsResponsePoliciesRulesUpdateOptions generated attributes, which matched exactly.
test/ModularPipelines.Google.UnitTests/GcloudCompositeValueTests.cs: Updated existing tests to pass the now-required positional resource argument (e.g. GcloudBmsNfsSharesUpdateOptions("nfs-share")) and include it in expected argument lists; confirmed the generated constructors require these parameters and the composite-value flag rendering is unchanged.
docs/docs/mp-packages/cli/gcloud.md: Mechanically regenerated command reference table adding entries for the 24 new gcloud commands (e.g. app-topology, compute image-views, memorystore token-auth-users); additions are consistent with the corresponding new generated Options classes present in the diff.
No actionable findings.
Optional follow-up notes
No OptionsGenerator source files are touched in this diff, matching the PR description's claim that this is an unmodified generated artifact plus handwritten test adaptation — consistent with the AGENTS.md rule against hand-editing generated options.
Verified several handwritten test assertions directly against their corresponding regenerated *.Generated.cs option classes (GcloudArtifactsFilesUploadOptions exactly-one-of Source/SourceDirectory validation, GcloudBmsNfsSharesUpdateOptions required NfsShare constructor + at-most-one-of groups, GcloudDnsResponsePoliciesRulesUpdateOptions LocalData rendering as repeated --local-data= options vs. GcloudMetastoreServicesMigrationsStartOptions HiveDatabases/IcebergNamespaces rendering as comma-joined single options, GcloudMemorystoreInstancesCreateTokenAuthUserOptions/TokenAuthUsersAuthTokensDescribeOptions TokenAuthUser correctly lacking [SecretValue] while GcloudDatabaseMigrationConnectionProfilesCreateMysqlOptions.PrivateKey/Password correctly carry it) — all matched, no discrepancies found.
This is a large, mechanically-generated PR; given the AGENTS.md guidance to treat current generator output as authoritative and not hand-review generated content line-by-line, review focused on the handwritten test surface and cross-checked it against the generated code it exercises rather than auditing all ~7,700 changed generated files individually.
No prior review comments or requested changes exist on this PR to reconcile against (only an in-progress automated Codex review with no findings yet posted).
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThe gcloud CLI reference table adds command-to-options mappings for ChangesGcloud CLI reference
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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. A rabbit maps commands in rows Comment |
|
Fresh Google SDK 585 output in #5333 (ee60d8b; generation run 35626984458) still exposes shared argument-group scope defects:
The exact SDK 585 help is captured locally for regression fixtures. I am closing the defective snapshot, cancelling its CI, and fixing |
GitHub Actions
Failure
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee60d8b1d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// <summary> | ||
| /// AuthConfig represents the authentication information. Arguments for the config. At most one of these can be specified: Config for authentication with API key. The location of the API key. The default value is QUERY. API_KEY_CONFIG_HTTP_ELEMENT_LOCATION must be one of: body Element is in the HTTP request body. cookie Element is in the HTTP request cookie. header Element is in the HTTP request header. path Element is in the HTTP request path. query Element is in the HTTP request query. This flag argument must be specified if any of the other arguments in this group are specified. | ||
| /// </summary> | ||
| [SecretValue] |
There was a problem hiding this comment.
When this option is set to a normal value such as query, header, or body, Command.RegisterSecrets treats that common word as a secret and globally masks its occurrences in command output and subsequent logs. This property is explicitly an enum-like HTTP location rather than credential material, so the generator's secret classification corrupts diagnostics; correct the classifier and regenerate the options.
AGENTS.md reference: AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The shared classifier is corrected in 425362e, confirmed in the remote head of source prerequisite #5334. Captured SDK 585 help reproduces the original failure; 249 focused classifier/enhancer/scraper tests now pass, including preservation of explicit secret overrides and documented credential content. Google create/update runtime metadata regressions are prepared for the next authoritative snapshot. This generated-output finding remains open until that fresh replacement is validated.
|
Fresh replacement: #5338, confirmed remote head 34bc46d. It applies the unmodified Google SDK 585 artifact from latest-main run 35666140726 after source fixes through #5337, with the handwritten Google regression tests. Coverage includes 7,951 commands and 1,455 groups with no removals or exclusions; docs and freshness checks pass. Explicit Google integration validation is running at https://github.com/thomhurst/ModularPipelines/actions/runs/35669471943. Acceptance findings remain open until runtime CI and review complete. |
|
Fresh replacement: #5341, generated unchanged from main 1be7211 by https://github.com/thomhurst/ModularPipelines/actions/runs/35676360503 after source fix #5340. Provenance and coverage pass: 7,951 commands, 1,455 groups, 24 additions, no removals/exclusions, all 54 reported omissions present. The Google library and documentation build; dedicated integration CI and reviews are pending in https://github.com/thomhurst/ModularPipelines/actions/runs/35678558349. This closed PR remains superseded. |
|
Fresh latest-main replacement: #5344, generated by run 35684123650 after source fixes through #5343. Artifact 10676728248 is unchanged and passes provenance, coverage, and documentation checks. The replacement contains 76 nested-choice runtime cases plus the earlier regression suites. Actual Google integration CI and reviews are pending; acceptance issues remain open until those pass. |
|
Fresh latest-main replacement: #5347, generated by run 35690354313 after source fixes through #5346. Artifact 10678711892 is unchanged and passes provenance, coverage, and documentation checks. Compared with rejected #5344, only eight embedded-credential SecretValue annotations and the fingerprint change; no masking annotation is removed. The replacement retains 76 nested-choice cases and adds runtime attribute checks for all eight credential fields. Explicit integration CI: https://github.com/thomhurst/ModularPipelines/actions/runs/35692851347. Acceptance remains pending actual Google test execution and reviews. |
This branch had an error being deployed
1 participant
Google Cloud SDK 585.0.0 adds 24 commands and exposes parser defects in nested resource groups, conditional requirements, list values, enum collisions, and secret classification. This refresh applies the unmodified artifact from generation run 35626984458, produced after the generator fixes in #5328, #5331, and #5332 merged to main.
The handwritten integration tests adapt to required resource constructors and exercise upload alternatives, resource arguments and service contracts, comma-delimited versus repeated options, command-specific enum values, provider requirements, resource-value flags, secret metadata, SQL group independence, Iceberg requirements, and optional Pub/Sub schema settings. Current CLI output remains authoritative; no compatibility shims or generated API baselines are added.
Replacement for closed #5315 and #5179; completes the runtime coverage from closed #4863, #4869, and #4956. Related acceptance issues: #5180, #5178, #5166, #5146, #4850, #4849, #4799, #4797, #4474, and #4331.
Validation: exact workflow patch verified unchanged; provenance matches main 1efed21; 7,951 commands (+24, no removals); command-tree SHA256 07958cfa2fad3f97aa3e098bfb3216ec2277041b474634f70e73a6f6efa21c11; handwritten tests formatted and git diff --check passed. Documentation build passed (331 documents). Explicit full integration CI: https://github.com/thomhurst/ModularPipelines/actions/runs/35631746332 (pending). The generation workflow compiled the generated library, then failed on the four known handwritten constructor calls tracked by #5178; this PR adapts those calls and validates the resulting integration. Local full Google compilation previously reached the mandated 2 GB guard; the expensive build and runtime tests run in CI.
Summary by CodeRabbit