use account-verifier response types from api#599
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔀 Multi-repo context codeready-toolchain/api, codeready-toolchain/toolchain-commonLinked Repositories Research FindingsBased on my exploration of the cross-repository impact, here's what I found: codeready-toolchain/apiAccountVerifierResponse definition:
The annotation key is defined as codeready-toolchain/toolchain-commonTest configuration support:
State management:
codeready-toolchain/host-operatorNo direct consumption of account-verifier results: The usersignup controller and cleanup controller handle rejected/verification-required states but do not depend on the specific result string values. codeready-toolchain/toolchain-e2eNo direct AccountVerifier references: Impact AssessmentThe string value changes ("reject" → "rejected", "phone-verification" → "phone_verification") are locally contained to the registration-service ↔ API boundary. The host-operator and E2E tests operate at the UserSignup state abstraction level and do not directly depend on these string constants, reducing the risk of breaking changes in dependent repositories. 🔇 Additional comments (1)
WalkthroughUpdates the ChangesAccount-verifier type migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
refactoring
![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
🤖 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 `@go.mod`:
- Around line 22-23: Remove the replace directive in go.mod that redirects
github.com/codeready-toolchain/api to the personal fork
github.com/matousjobanek/api. Once your API changes are merged into the official
upstream repository, update the go.mod to depend on the official version with an
appropriate version pin. If you still need to test against a fork locally during
development, manage this through local-only configuration (such as
.git/info/attributes or local development setup) rather than committing fork
overrides to the repository.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 72029767-daa6-409c-8d55-6434972f72f2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modpkg/signup/service/signup_service.gopkg/signup/service/signup_service_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
go.modpkg/signup/service/signup_service_test.gopkg/signup/service/signup_service.go
🔇 Additional comments (4)
go.mod (1)
9-9: LGTM!pkg/signup/service/signup_service.go (2)
69-69: LGTM!Also applies to: 148-148, 279-310, 392-402
157-160: 🔒 Security & PrivacyHandle legacy verifier result values to avoid fail-open behavior during rollout.
Lines 157-160 and 335-336 only directly compare against enum constants. If the external verifier emits legacy string values during version skew, rejected users may bypass blocking and phone verification may not be enforced.
Suggested compatibility patch
@@ - if accountVerifierResp.Result == toolchainv1alpha1.AccountVerifierResultPhoneVerification { + if isAccountVerifierPhoneVerification(accountVerifierResp) { states.SetVerificationRequired(userSignup, true) } @@ func isAccountVerifierRejected(resp *toolchainv1alpha1.AccountVerifierResponse) bool { - return resp != nil && resp.Result == toolchainv1alpha1.AccountVerifierResultRejected + if resp == nil { + return false + } + return resp.Result == toolchainv1alpha1.AccountVerifierResultRejected || string(resp.Result) == "reject" } + +func isAccountVerifierPhoneVerification(resp *toolchainv1alpha1.AccountVerifierResponse) bool { + if resp == nil { + return false + } + return resp.Result == toolchainv1alpha1.AccountVerifierResultPhoneVerification || string(resp.Result) == "phone-verification" +}pkg/signup/service/signup_service_test.go (1)
1542-1591: LGTM!Also applies to: 1640-1679, 1764-1816
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, metlos, MikelAlejoBR, rajivnathan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
MatousJobanek
merged commit e8ab074
into
codeready-toolchain:master
|
4 participants
related PR codeready-toolchain/api#511
Summary by CodeRabbit