fix(stack): add missing stack metadata update flag #83
Conversation
Co-authored-by: salaun.clement <[email protected]>
|
Cursor Agent can help with this pull request. Just |
WalkthroughIntroduces a new --metadata flag to the stack update command, parses user-supplied metadata, merges it with the protected metadata (which is enforced/overridden), and sends the merged map in UpdateStackRequest. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as stack update (cmd)
participant F as fctl (flag & metadata utils)
participant API as UpdateStack API
U->>CLI: run "stack update --metadata k=v ..."
CLI->>F: GetStringSlice(metadataFlag)
F-->>CLI: []string{"k=v", ...}
CLI->>F: ParseMetadata(strings)
F-->>CLI: map{ k: v, ... } or error
alt parse error
CLI-->>U: fail with "parsing metadata"
else success
note over CLI: Create metadata map<br/>Merge user entries<br/>Override ProtectedStackMetadata
CLI->>API: UpdateStackRequest(metadata=merged)
API-->>CLI: response
CLI-->>U: success
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
altitude
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/stack/update.go (1)
62-67: Parsing path is fine once the default is fixed; consider trimming/ignoring empties defensively.Optional: filter out blank entries before parsing to be resilient to odd env/quoting cases.
Example:
- userMetadata, err := fctl.ParseMetadata(fctl.GetStringSlice(cmd, metadataFlag)) + rawMD := fctl.GetStringSlice(cmd, metadataFlag) + filtered := make([]string, 0, len(rawMD)) + for _, s := range rawMD { + if strings.TrimSpace(s) != "" { + filtered = append(filtered, s) + } + } + userMetadata, err := fctl.ParseMetadata(filtered)Note: add
stringsimport if you adopt this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cmd/stack/update.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/stack/update.go (5)
pkg/command.go (1)
WithStringSliceFlag(160-164)pkg/metadata.go (1)
ParseMetadata(13-23)pkg/flags.go (2)
GetStringSlice(43-53)GetBool(26-33)pkg/approval.go (1)
ProtectedStackMetadata(25-25)pkg/ptr.go (1)
BoolPointerToString(12-17)
🪛 GitHub Actions: Default
cmd/stack/update.go
[error] 68-79: CI cleanliness check failed due to uncommitted changes. The script ran 'git status --porcelain' and detected changes in cmd/stack/update.go, causing the step to exit with code 1.
🔇 Additional comments (2)
cmd/stack/update.go (2)
13-15: LGTM on adding metadataFlag.Restores parity between help and functionality.
1-1: CI cleanliness: run formatting/codegen and commitCI found a dirty working tree (affected: cmd/stack/update.go). Sandbox verification failed with "telemetry sidecar: os.Executable: readlink /proc/self/exe: no such file or directory" and "go: go.mod file not found"; cannot auto-verify here.
Run locally and push fixes:
- git status --porcelain
- git diff -- cmd/stack/update.go
- gofmt -s -w .
- go mod tidy
- if [ -f Makefile ]; then make fmt && make generate; fi
- git add cmd/stack/update.go
- git commit -m "chore: format/codegen" && git push
| }), | ||
| fctl.WithBoolFlag(unprotectFlag, false, "Unprotect stacks (no confirmation on write commands)"), | ||
| fctl.WithStringFlag(nameFlag, "", "Name of the stack"), | ||
| fctl.WithStringSliceFlag(metadataFlag, []string{""}, "Metadata to use"), |
There was a problem hiding this comment.
Bug: default []string{""} makes ParseMetadata fail when flag is omitted.
With the current default, GetStringSlice returns a single empty string, and ParseMetadata errors with “malformed metadata: ”. Use an empty slice as default.
Apply this diff:
- fctl.WithStringSliceFlag(metadataFlag, []string{""}, "Metadata to use"),
+ fctl.WithStringSliceFlag(metadataFlag, []string{}, "Metadata key=value. Repeat the flag to add multiple entries"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fctl.WithStringSliceFlag(metadataFlag, []string{""}, "Metadata to use"), | |
| fctl.WithStringSliceFlag(metadataFlag, []string{}, "Metadata key=value. Repeat the flag to add multiple entries"), |
🤖 Prompt for AI Agents
In cmd/stack/update.go around line 50, the flag default is set to []string{""}
which causes GetStringSlice to return a single empty string and ParseMetadata to
fail; change the flag default to an empty slice (i.e., []string{}) so omitted
flag yields no entries, then run tests/CLI to verify ParseMetadata no longer
errors when the flag is omitted.
| // Merge user metadata with protected metadata | ||
| protected := !fctl.GetBool(cmd, unprotectFlag) | ||
| metadata := map[string]string{ | ||
| fctl.ProtectedStackMetadata: fctl.BoolPointerToString(&protected), | ||
| metadata := make(map[string]string) | ||
|
|
||
| // Add user metadata first | ||
| for k, v := range userMetadata { | ||
| metadata[k] = v | ||
| } | ||
|
|
||
| // Add/override with protected metadata | ||
| metadata[fctl.ProtectedStackMetadata] = fctl.BoolPointerToString(&protected) | ||
|
|
There was a problem hiding this comment.
❓ Verification inconclusive
Risk: updating name with no --metadata will wipe existing metadata keys.
You always send a fresh map containing only user pairs plus the protected key. If the stack already has other metadata, those keys are lost on update. Preserve existing metadata and still enforce the protected key.
Apply this minimal merge and gate the request payload:
- // Merge user metadata with protected metadata
- protected := !fctl.GetBool(cmd, unprotectFlag)
- metadata := make(map[string]string)
-
- // Add user metadata first
- for k, v := range userMetadata {
- metadata[k] = v
- }
-
- // Add/override with protected metadata
- metadata[fctl.ProtectedStackMetadata] = fctl.BoolPointerToString(&protected)
+ // Prepare metadata (user-supplied now, merge existing later)
+ protected := !fctl.GetBool(cmd, unprotectFlag)
+ metadata := make(map[string]string, len(userMetadata)+1)
+ for k, v := range userMetadata {
+ metadata[k] = v
+ }
+ // Enforce protected flag; prefer direct bool->string if available.
+ metadata[fctl.ProtectedStackMetadata] = fctl.BoolToString(protected)
@@
- name := fctl.GetString(cmd, nameFlag)
+ // Preserve existing metadata keys not provided by the user
+ if stack.Data != nil && stack.Data.Metadata != nil {
+ for k, v := range stack.Data.Metadata {
+ if _, exists := metadata[k]; !exists {
+ metadata[k] = v
+ }
+ }
+ }
+ name := fctl.GetString(cmd, nameFlag)
@@
- req := membershipclient.UpdateStackRequest{
- Name: name,
- Metadata: pointer.For(metadata),
- }
+ // If neither --metadata nor --unprotect was provided, avoid sending Metadata to prevent unintended changes.
+ var metaPtr *map[string]string
+ if len(userMetadata) > 0 || cmd.Flags().Changed(unprotectFlag) {
+ metaPtr = pointer.For(metadata)
+ }
+ req := membershipclient.UpdateStackRequest{
+ Name: name,
+ Metadata: metaPtr,
+ }If BoolToString isn’t available in your pkg, keep BoolPointerToString(&protected).
Don't overwrite existing stack metadata — merge current metadata and only send Metadata when the user changed it.
File: cmd/stack/update.go Lines: 68-79
The code builds a fresh metadata map (userMetadata + protected key) and will erase any existing metadata on update; merge stack.Data.Metadata (preserving keys not provided by the user) and only include Metadata in UpdateStackRequest when userMetadata is non-empty or the unprotect flag was changed.
- // Merge user metadata with protected metadata
- protected := !fctl.GetBool(cmd, unprotectFlag)
- metadata := make(map[string]string)
-
- // Add user metadata first
- for k, v := range userMetadata {
- metadata[k] = v
- }
-
- // Add/override with protected metadata
- metadata[fctl.ProtectedStackMetadata] = fctl.BoolPointerToString(&protected)
+ // Prepare metadata (user-supplied now, merge existing later)
+ protected := !fctl.GetBool(cmd, unprotectFlag)
+ metadata := make(map[string]string, len(userMetadata)+1)
+ for k, v := range userMetadata {
+ metadata[k] = v
+ }
+ // Enforce protected flag; prefer direct bool->string if available.
+ metadata[fctl.ProtectedStackMetadata] = fctl.BoolToString(protected)
@@
- name := fctl.GetString(cmd, nameFlag)
+ // Preserve existing metadata keys not provided by the user
+ if stack.Data != nil && stack.Data.Metadata != nil {
+ for k, v := range stack.Data.Metadata {
+ if _, exists := metadata[k]; !exists {
+ metadata[k] = v
+ }
+ }
+ }
+ name := fctl.GetString(cmd, nameFlag)
@@
- req := membershipclient.UpdateStackRequest{
- Name: name,
- Metadata: pointer.For(metadata),
- }
+ // If neither --metadata nor --unprotect was provided, avoid sending Metadata to prevent unintended changes.
+ var metaPtr *map[string]string
+ if len(userMetadata) > 0 || cmd.Flags().Changed(unprotectFlag) {
+ metaPtr = pointer.For(metadata)
+ }
+ req := membershipclient.UpdateStackRequest{
+ Name: name,
+ Metadata: metaPtr,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Merge user metadata with protected metadata | |
| protected := !fctl.GetBool(cmd, unprotectFlag) | |
| metadata := map[string]string{ | |
| fctl.ProtectedStackMetadata: fctl.BoolPointerToString(&protected), | |
| metadata := make(map[string]string) | |
| // Add user metadata first | |
| for k, v := range userMetadata { | |
| metadata[k] = v | |
| } | |
| // Add/override with protected metadata | |
| metadata[fctl.ProtectedStackMetadata] = fctl.BoolPointerToString(&protected) | |
| // Prepare metadata (user-supplied now, merge existing later) | |
| protected := !fctl.GetBool(cmd, unprotectFlag) | |
| metadata := make(map[string]string, len(userMetadata)+1) | |
| for k, v := range userMetadata { | |
| metadata[k] = v | |
| } | |
| // Enforce protected flag; prefer direct bool->string if available. | |
| metadata[fctl.ProtectedStackMetadata] = fctl.BoolToString(protected) | |
| // Preserve existing metadata keys not provided by the user | |
| if stack.Data != nil && stack.Data.Metadata != nil { | |
| for k, v := range stack.Data.Metadata { | |
| if _, exists := metadata[k]; !exists { | |
| metadata[k] = v | |
| } | |
| } | |
| } | |
| name := fctl.GetString(cmd, nameFlag) | |
| // If neither --metadata nor --unprotect was provided, avoid sending Metadata to prevent unintended changes. | |
| var metaPtr *map[string]string | |
| if len(userMetadata) > 0 || cmd.Flags().Changed(unprotectFlag) { | |
| metaPtr = pointer.For(metadata) | |
| } | |
| req := membershipclient.UpdateStackRequest{ | |
| Name: name, | |
| Metadata: metaPtr, | |
| } |
🧰 Tools
🪛 GitHub Actions: Default
[error] 68-79: CI cleanliness check failed due to uncommitted changes. The script ran 'git status --porcelain' and detected changes in cmd/stack/update.go, causing the step to exit with code 1.
🤖 Prompt for AI Agents
In cmd/stack/update.go around lines 68-79, the current code creates a fresh
metadata map which wipes existing stack metadata; instead, fetch and clone the
current stack.Data.Metadata into your working metadata map first, then overlay
userMetadata and the protected key so keys not provided by the user are
preserved. Only set the UpdateStackRequest.Metadata field if the user provided
any metadata changes (userMetadata is non-empty) or if the protected/unprotect
flag changed compared to the original stack.Data.Metadata value (compare
original protected value vs intended protected); otherwise omit Metadata from
the request to avoid erasing existing values.
|
Resolve coderabbit comment |
Add
--metadataflag tofctl stack updateto enable updating stack metadata as described in the help text.The
fctl stack update --helpoutput indicated that stack metadata could be updated, but the--metadataflag was not implemented, leading to a discrepancy between documentation and functionality. This PR resolves that by adding the flag and its processing logic.Slack Thread