OCM-4245 | Add validation retry for GCP service account file prompts #948
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: miguelhbrito The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughGCP CCS authentication flow was refactored: prompting for the service account file now uses a PromptFilePath callback that resolves and validates the path and returns a plain string; the helper to resolve relative paths was exported as ResolveRelativePath; constructGCPCredentials signature changed from an arguments.FilePath to string. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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)
cmd/ocm/create/cluster/cmd.go (1)
1616-1631: Consider deferring credential construction until after validation succeeds.The validator function constructs GCP credentials (
constructGCPCredentialsat line 1626) as a side effect during validation. If the user provides multiple invalid file paths,args.ccs.GCPmay be modified during each validation attempt.While this achieves the retry behavior and
json.Unmarshaltypically preserves state on failure, it's generally better practice to separate validation from state modification. Consider validating the file path and JSON structure without modifyingargs.ccs, then constructing credentials only after the prompt completes successfully.Alternative approach
case c.AuthenticationKey: err = arguments.PromptFilePath(fs, "service-account-file", true, func(val interface{}) error { gcpServiceAccountFile, ok := val.(string) if !ok { return fmt.Errorf("invalid file path") } gcpServiceAccountFile, err := arguments.ResolveRelativePath(gcpServiceAccountFile) if err != nil { return fmt.Errorf("invalid file path: %v", err) } - if err := constructGCPCredentials(gcpServiceAccountFile, &args.ccs); err != nil { - return err - } + // Validate file exists and is valid JSON without modifying state + jsonFile, err := os.Open(gcpServiceAccountFile) + if err != nil { + return err + } + defer jsonFile.Close() + var tempGCP c.GCPCredentials + byteValue, _ := io.ReadAll(jsonFile) + if err := json.Unmarshal(byteValue, &tempGCP); err != nil { + return err + } return nil }, ) if err != nil { return err } + // Construct credentials after validation succeeds + flag := fs.Lookup("service-account-file") + if err := constructGCPCredentials(flag.Value.String(), &args.ccs); err != nil { + return err + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
cmd/ocm/create/cluster/cmd.gopkg/arguments/interactive.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/ocm/create/cluster/cmd.go (2)
pkg/arguments/interactive.go (2)
PromptFilePath(257-299)ResolveRelativePath(303-315)pkg/cluster/cluster.go (1)
CCS(117-121)
pkg/arguments/interactive.go (1)
cmd/ocm/create/machinepool/helpers.go (1)
FlagSet(14-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Red Hat Konflux / ocm-cli-on-pull-request
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (macos-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint
🔇 Additional comments (3)
pkg/arguments/interactive.go (2)
257-262: LGTM! Well-designed extensibility.The variadic validators parameter allows callers to inject custom validation logic while maintaining backward compatibility.
303-315: LGTM! Appropriate export for external usage.The function is correctly exported to support its use in
cmd/ocm/create/cluster/cmd.gofor path resolution within the validator callback.cmd/ocm/create/cluster/cmd.go (1)
1879-1892: LGTM! Cleaner function signature.The change from
arguments.FilePathtostringsimplifies the function signature and makes it more flexible for use in the validator callback.
miguelhbrito
force-pushed
the
pop-out-message-for-invalid-service-account-file
branch
from
99c90aa to
392d285
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/arguments/interactive.go (1)
304-316: Consider handling standalone tilde.The function handles
~/pathbut not~alone (just the home directory). If a user enters~expecting it to resolve to their home directory, it will be returned unchanged.🔎 Proposed fix to handle standalone tilde
func ResolveRelativePath(path string) (string, error) { - if !strings.Contains(path, "~") { + if path != "~" && !strings.HasPrefix(path, "~/") { return path, nil } usr, err := user.Current() if err != nil { return "", err } - if strings.HasPrefix(path, "~/") { + if path == "~" { + path = usr.HomeDir + } else if strings.HasPrefix(path, "~/") { path = strings.Replace(path, "~", usr.HomeDir, 1) } return path, nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
cmd/ocm/create/cluster/cmd.gopkg/arguments/interactive.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ocm/create/cluster/cmd.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Red Hat Konflux / ocm-cli-on-pull-request
- GitHub Check: Test (macos-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint
🔇 Additional comments (2)
pkg/arguments/interactive.go (2)
257-262: Good extensibility for custom validators.The variadic
validatorsparameter cleanly extends the function's capabilities while maintaining backward compatibility with existing callers that don't need additional validation.
285-296: Error handling is now correct.The
survey.AskOneerror is properly checked and returned at lines 289-291 before callingResolveRelativePath. This addresses the previous review concern about the error being overwritten.
1 participant
mipereir@mipereir-thinkpadp1gen4i:~/myprojects/ocm-cli$ ./ocm create cluster -i
? Cluster name: mipereir-pr-test
? Subscription type: standard (Annual: Fixed capacity subscription from Red Hat)
? Cloud provider: gcp
? CCS: Yes
? Authentication type: Service account
X Sorry, your reply was invalid: open aaa: no such file or directory
? Service account file: [? for help, tab for suggestions]
mipereir@mipereir-thinkpadp1gen4i:~/myprojects/ocm-cli$ ./ocm create cluster -i
? Cluster name: mipereir-pr-test
? Subscription type: standard (Annual: Fixed capacity subscription from Red Hat)
? Cloud provider: gcp
? CCS: Yes
? Authentication type: Service account
X Sorry, your reply was invalid: unexpected end of JSON input
? Service account file: [? for help, tab for suggestions] /home/mipereir/.gcp/aaa.json
mipereir@mipereir-thinkpadp1gen4i:~/myprojects/ocm-cli$ ./ocm create cluster -i
? Cluster name: mipereir-pr-test
? Subscription type: standard (Annual: Fixed capacity subscription from Red Hat)
? Cloud provider: gcp
? CCS: Yes
? Authentication type: Service account
? Service account file: /home/mipereir/.gcp/osd-ccs-admin.json
? Region: [Use arrows to move, type to filter, ? for more help]