CLI support for backingstore endpoint update#2014
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new ChangesBacking store update support
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CmdUpdate/RunUpdate
participant K8sAPI as Kubernetes API
User->>CLI: noobaa backingstore update <name> --endpoint
CLI->>K8sAPI: fetch BackingStore CR
K8sAPI-->>CLI: return BackingStore
CLI->>CLI: validate type against validBackingStores
CLI->>CLI: apply endpoint to spec sub-structure
CLI->>K8sAPI: KubeUpdate(BackingStore)
K8sAPI-->>CLI: update confirmation
CLI->>CLI: wait for readiness
CLI->>User: RunStatus output
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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. Comment |
Signed-off-by: Karthik P S <karthikperla2000@gmail.com>
9bee92d to
b09e359
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/backingstore/backingstore.go (1)
35-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared backing-store type constants. Keep only the three STS-only strings local; the six common backing-store names can use
nbv1.StoreType*to avoid drifting from the shared type values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/backingstore/backingstore.go` around lines 35 - 49, The backing-store type constants in validBackingStores should reuse the shared nbv1.StoreType values for the common non-STS names instead of duplicating local strings. Update the constants in backingstore.go so only the three STS-only values remain local, and change validBackingStores to reference nbv1.StoreTypeS3Compatible, nbv1.StoreTypeIBMCOS, nbv1.StoreTypeAWSS3, nbv1.StoreTypeGoogleCloudStorage, nbv1.StoreTypeAzureBlob, and nbv1.StoreTypePVPool alongside the local STS constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/backingstore/backingstore.go`:
- Around line 1016-1020: `RunUpdate` currently does nothing when `WaitReady(bs)`
returns false, so add an explicit failure or cancellation message in the
`RunUpdate` flow after the `WaitReady(bs)` check. Update the logging around the
existing `WaitReady(bs)` / `RunStatus(cmd, args)` path so users get clear
feedback for rejected or cancelled backing store readiness, using the existing
`RunUpdate` and `WaitReady` symbols to locate the branch.
---
Nitpick comments:
In `@pkg/backingstore/backingstore.go`:
- Around line 35-49: The backing-store type constants in validBackingStores
should reuse the shared nbv1.StoreType values for the common non-STS names
instead of duplicating local strings. Update the constants in backingstore.go so
only the three STS-only values remain local, and change validBackingStores to
reference nbv1.StoreTypeS3Compatible, nbv1.StoreTypeIBMCOS, nbv1.StoreTypeAWSS3,
nbv1.StoreTypeGoogleCloudStorage, nbv1.StoreTypeAzureBlob, and
nbv1.StoreTypePVPool alongside the local STS constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81a15727-6cdb-4bbd-b58d-d359dbe80a85
📒 Files selected for processing (1)
pkg/backingstore/backingstore.go
| if WaitReady(bs) { | ||
| log.Printf("") | ||
| log.Printf("") | ||
| RunStatus(cmd, args) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add user feedback when WaitReady returns false.
If WaitReady(bs) returns false (e.g., the backing store phase becomes Rejected or the context is cancelled), RunUpdate exits silently. The user sees "BackingStore Wait Ready:" with no subsequent indication of success or failure.
🔧 Proposed fix
if WaitReady(bs) {
log.Printf("")
log.Printf("")
RunStatus(cmd, args)
+} else {
+ log.Fatalf("❌ BackingStore %s did not become ready after update", bs.Name)
}📝 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.
| if WaitReady(bs) { | |
| log.Printf("") | |
| log.Printf("") | |
| RunStatus(cmd, args) | |
| } | |
| if WaitReady(bs) { | |
| log.Printf("") | |
| log.Printf("") | |
| RunStatus(cmd, args) | |
| } else { | |
| log.Fatalf("❌ BackingStore %s did not become ready after update", bs.Name) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/backingstore/backingstore.go` around lines 1016 - 1020, `RunUpdate`
currently does nothing when `WaitReady(bs)` returns false, so add an explicit
failure or cancellation message in the `RunUpdate` flow after the
`WaitReady(bs)` check. Update the logging around the existing `WaitReady(bs)` /
`RunStatus(cmd, args)` path so users get clear feedback for rejected or
cancelled backing store readiness, using the existing `RunUpdate` and
`WaitReady` symbols to locate the branch.
| log.Fatalf(`❌ Invalid backing store spec`) | ||
| } | ||
| bs.Spec.IBMCos.Endpoint = endpoint | ||
| default: |
There was a problem hiding this comment.
I think you can remove the default case, there is no code under it.
| Short: "Update a backing store", | ||
| Run: RunUpdate, | ||
| } | ||
| cmd.Flags().String("endpoint", "", "New endpoint URL") |
There was a problem hiding this comment.
To match the code style in the file, suggest writing it in multiple lines
| cmd.Flags().String("endpoint", "", "New endpoint URL") | |
| cmd.Flags().String( | |
| "endpoint", "", | |
| "New endpoint URL" | |
| ) |
| // args: <backingstore name> | ||
| // flags: <endpoint> |
There was a problem hiding this comment.
I don't think you need this comment
| const ( | ||
| S3Compatible = "s3-compatible" | ||
| IBMCOS = "ibm-cos" | ||
| AWSS3 = "aws-s3" | ||
| AWSSTSS3 = "aws-sts-s3" | ||
| GoogleCloudStorage = "google-cloud-storage" | ||
| GoogleCloudStorageSTS = "google-cloud-storage-sts" | ||
| AzureBlob = "azure-blob" | ||
| AzureSTSBlob = "azure-sts-blob" | ||
| PVPool = "pv-pool" | ||
| ) |
There was a problem hiding this comment.
Someone might want to reuse this for namespacestore as well.
2 participants
Describe the Problem
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
IBM COSorS3 compatible.nb backingstore update <name> --endpoint=<new-endpoint> -n <namespace>Dependent PR's: PR 1, PR 2
Summary by CodeRabbit
backingstore updateCLI subcommand with an--endpointflag.