Add validate account before creating - #102
Conversation
WalkthroughAdds a package-level error in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)pkg/bsql/user_syncer.go (1)
⏰ 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). (1)
🔇 Additional comments (3)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/bsql/provisioning.go (1)
16-17: Add documentation comment for exported error variable.Per Go conventions and the coding guidelines, exported identifiers should have documentation comments that are complete sentences ending with periods.
+// ErrUnableFindResourceProvisioning is returned when the validation query does not find a matching resource. var ErrUnableFindResourceProvisioning = errors.New("unable to find resource for account provisioning")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/bsql/provisioning.go(2 hunks)pkg/bsql/user_syncer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/bsql/provisioning.gopkg/bsql/user_syncer.go
🧬 Code graph analysis (1)
pkg/bsql/user_syncer.go (1)
pkg/bsql/provisioning.go (1)
ErrUnableFindResourceProvisioning(16-16)
⏰ 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). (1)
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (1)
pkg/bsql/provisioning.go (1)
184-186: LGTM!Using the sentinel error here enables callers to distinguish "resource not found" from other validation failures using
errors.Is().
| } | ||
|
|
||
| if previousAccountResource != nil { | ||
| logger.Info("account resource is already created", zap.String("resource_id", previousAccountResource.GetId().GetResource())) |
There was a problem hiding this comment.
If it's already created, don't we want to update the password based on the credential options?
ggreer
left a comment
There was a problem hiding this comment.
Coderabbit has a good point about that error message, and our real fix should involve returning a new CreateAccountResponse with an AlreadyExistsResult, but this should help for now.
|
|
||
| return &v2.CreateAccountResponse_SuccessResult{ | ||
| Resource: previousAccountResource, | ||
| }, plaintextDataList, nil, nil |
There was a problem hiding this comment.
Oh yeah, definitely remove the plaintextDataList from the return value, because those credentials are not the ones for the existing account.
Description
Useful links:
Summary by CodeRabbit
Bug Fixes
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.