Add PhoneLookupMode to ToolchainConfig CRD#1268
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds two phone lookup fields to the ToolchainConfig CRD under spec.host.registrationService.verification and updates go.mod with a replace directive for the API dependency. ChangesPhone Lookup Configuration Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 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
dependencies
![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
🤖 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 `@config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml`:
- Around line 359-368: The CRD schema documents a default of "log" for
phoneLookupMode but doesn't set it in the schema; update the OpenAPI schema for
the phoneLookupMode property (phoneLookupMode under the CRD spec properties) to
include default: "log" so the CRD enforces the documented default value and
omitted fields resolve to "log" at validation time.
In `@go.mod`:
- Line 160: Remove the committed replace directive that swaps
github.com/codeready-toolchain/api to the fork github.com/fbm3307/toolchainapi
in go.mod; instead restore the upstream module import
(github.com/codeready-toolchain/api) and its proper version, and rely on the
upstream tag/ref once the API change is merged—specifically delete the replace
line shown in go.mod and update any version requirements to the upstream release
rather than using the forked module.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d8f0bbbd-55ee-43be-b4fb-54470ca83188
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yamlgo.mod
📜 Review details
⏰ 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). (4)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
- GitHub Check: govulncheck
- GitHub Check: GolangCI Lint
- GitHub Check: test
🧰 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.modconfig/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml
🪛 GitHub Actions: ci-check-gomod / 0_go.mod replacements.txt
go.mod
[error] 1-1: Forbidden module replacement detected in go.mod. The script checks go list -m all | grep -E "${REGEX}.*\s*=>" for protected module replacements and fails with: "the above replacement(s) are not allowed in go.mod".
[error] 1-1: Disallowed replace directive found: "github.com/codeready-toolchain/api v0.0.0-20260415142422-12ff40f3bdb6 => github.com/fbm3307/toolchainapi v0.0.0-20260604075613-b313112960df".
🪛 GitHub Actions: ci-check-gomod / go.mod replacements
go.mod
[error] 1-1: CI step failed: replacement(s) are not allowed in go.mod. The log shows: 'github.com/codeready-toolchain/api ... => github.com/fbm3307/toolchainapi ...' followed by 'the above replacement(s) are not allowed in go.mod' and 'Process completed with exit code 1.'
🔀 Multi-repo context codeready-toolchain/registration-service, codeready-toolchain/api, codeready-toolchain/toolchain-common, codeready-toolchain/toolchain-e2e
Linked repositories findings
-
Registration service consumes ToolchainConfig verification settings (Twilio usage and verification flow):
- pkg/configuration/configuration.go — loads ToolchainConfig and returns RegistrationServiceConfig (reads verification settings) [::codeready-toolchain/registration-service::pkg/configuration/configuration.go]
- pkg/verification/... — Twilio sender and verification service implementations/tests reference Twilio config and verification flows (multiple files; see twilio_sender.go, verification_service.go, tests) [::codeready-toolchain/registration-service::pkg/verification/sender/twilio_sender.go] [::codeready-toolchain/registration-service::pkg/verification/service/verification_service.go] [::codeready-toolchain/registration-service::pkg/verification/service/verification_service_test.go]
-
API repo defines the ToolchainConfig types/schema used by consumers:
- api/v1alpha1/toolchainconfig_types.go — ToolchainConfigSpec/verification-related types and comments (place to add new phoneLookupMode field) [::codeready-toolchain/api::api/v1alpha1/toolchainconfig_types.go]
- api/v1alpha1/zz_generated.openapi.go, zz_generated.deepcopy.go, docs — generated artifacts and docs for ToolchainConfig schema (ensure generation updated when CRD/types change) [::codeready-toolchain/api::api/v1alpha1/zz_generated.openapi.go] [::codeready-toolchain/api::api/v1alpha1/zz_generated.deepcopy.go] [::codeready-toolchain/api::api/v1alpha1/docs/apiref.adoc]
-
Shared test helpers and utilities set/read verification fields on ToolchainConfig:
- toolchain-common: pkg/test/config/toolchainconfig.go — helpers that set RegistrationService.Verification.Secret Twilio fields and other ToolchainConfig test options (may need extension to set phoneLookupMode in tests) [::codeready-toolchain/toolchain-common::pkg/test/config/toolchainconfig.go]
- toolchain-common: many test helpers that construct and assert ToolchainConfig objects (assertions/loaders) [::codeready-toolchain/toolchain-common::pkg/test/config/toolchainconfig_assertion.go]
-
E2E tests and deployment fixtures update/read ToolchainConfig:
- toolchain-e2e: tests frequently UpdateToolchainConfig/WaitForToolchainConfig and include deploy/toolchainconfig.yaml files with Twilio secrets — these test flows may need the new phoneLookupMode field added/handled (files: test/e2e/, deploy/host-operator/) [::codeready-toolchain/toolchain-e2e::test/e2e/parallel/registration_service_test.go] [::codeready-toolchain/toolchain-e2e::deploy/host-operator/e2e-tests/toolchainconfig.yaml]
-
go.mod replace in the PR (host-operator) points to a fork of api to pick up phone-lookup annotations — consumers that vendor or import api must ensure generated code/docs are in sync. I did not find go.mod files in the listed repos in the sandbox, but the API types are the authoritative place for the new field and generated artifacts must be updated there [::codeready-toolchain/api::api/v1alpha1/toolchainconfig_types.go]
Summary implication:
- No direct references to phoneLookupMode were found in these repos, but the registration-service reads verification config from ToolchainConfig and will pick up the new field once the api types/CRD are updated and code regenerated.
- Test helpers (toolchain-common) and e2e fixtures should be updated to set/expect the new phoneLookupMode where applicable to avoid test regressions.
- Ensure api repo has updated types + generated deepcopy/openapi/docs; propagate those generated artifacts to consumers (host-operator/go.mod replace already attempts this).
- Dispatch updated CRD YAML with phoneLookupMode field (disabled/log/enabled) - Update go.mod to reference toolchainapi with phone-lookup annotations Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
coderabbitai
Bot
removed
dependencies
There was a problem hiding this comment.
♻️ Duplicate comments (1)
go.mod (1)
160-160:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the fork
replacebefore merge — CI is currently blocked.Line 160 introduces a protected-module replacement that
ci-check-gomodalready rejects, so this PR cannot merge with this line present.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 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 `@go.mod` at line 160, Remove the temporary module replacement that CI rejects: delete the replace directive "replace github.com/codeready-toolchain/api => github.com/fbm3307/toolchainapi v0.0.0-20260604080936-f3460ab84acc" from go.mod, restore the dependency to the upstream module, then run go mod tidy to update go.sum and ensure the module graph is clean before pushing; keep the forked replace only in a personal branch if needed for testing.
🤖 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.
Duplicate comments:
In `@go.mod`:
- Line 160: Remove the temporary module replacement that CI rejects: delete the
replace directive "replace github.com/codeready-toolchain/api =>
github.com/fbm3307/toolchainapi v0.0.0-20260604080936-f3460ab84acc" from go.mod,
restore the dependency to the upstream module, then run go mod tidy to update
go.sum and ensure the module graph is clean before pushing; keep the forked
replace only in a personal branch if needed for testing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4d63dc8d-a0ce-458f-8288-f9a40a756cf3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yamlgo.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml
📜 Review details
🧰 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.mod
🪛 GitHub Actions: ci-check-gomod / go.mod replacements
go.mod
[error] 1-1: CI check failed: disallowed module replacement detected in go.mod for protected modules. The script ran 'go list -m all | grep --color=never -E "${REGEX}.\s=>"' and found replacement(s): 'github.com/codeready-toolchain/api v0.0.0-20260415142422-12ff40f3bdb6 => github.com/fbm3307/toolchainapi v0.0.0-20260604080936-f3460ab84acc'.
🔀 Multi-repo context codeready-toolchain/registration-service, codeready-toolchain/api, codeready-toolchain/toolchain-common, codeready-toolchain/toolchain-e2e, codeready-toolchain/member-operator
Linked repositories findings
registration-service
- Reads/uses verification config from ToolchainConfig:
- pkg/configuration/configuration.go — loads ToolchainConfig and maps RegistrationService verification config (e.g., Twilio fields, message template) [::codeready-toolchain/registration-service::pkg/configuration/configuration.go:55-99]
- pkg/configuration/configuration.go — accessor methods for verification (TwilioAccountSID, TwilioAuthToken, TwilioFromNumber, TwilioSenderConfigs) [::codeready-toolchain/registration-service::pkg/configuration/configuration.go:219-259]
- Verification logic and Twilio sender use ToolchainConfig verification settings:
- pkg/verification/service/verification_service.go — verification flow, annotations, captcha interaction (many refs) [::codeready-toolchain/registration-service::pkg/verification/service/verification_service.go:40-120, 152-176]
- pkg/verification/sender/twilio_sender.go — Twilio sender reads sender configs from config.TwilioSenderConfigs (used to send SMS) [::codeready-toolchain/registration-service::pkg/verification/sender/twilio_sender.go:13-51]
- Tests exercise verification behavior and construct ToolchainConfig in test helpers:
- pkg/verification/service/verification_service_test.go, pkg/verification/sender/twilio_sender_test.go (many usages of ToolchainConfig/Twilio secrets) [::codeready-toolchain/registration-service::pkg/verification/service/verification_service_test.go] [::codeready-toolchain/registration-service::pkg/verification/sender/twilio_sender_test.go]
Implication: registration-service will pick up the new spec.host.registrationService.verification.phoneLookupMode once API types/CRD are updated; code may need to read/act on that field if behavior changes (no current references found).
api
- ToolchainConfig types and generated artifacts present:
- api/v1alpha1/toolchainconfig_types.go — ToolchainConfigSpec includes RegistrationService and Verification config (place to add phoneLookupMode) [::codeready-toolchain/api::api/v1alpha1/toolchainconfig_types.go:28-61, 231-333]
- zz_generated.openapi.go, zz_generated.deepcopy.go — generated OpenAPI and deepcopy code for ToolchainConfig present and referenced (must be regenerated when types change) [::codeready-toolchain/api::api/v1alpha1/zz_generated.openapi.go] [::codeready-toolchain/api::api/v1alpha1/zz_generated.deepcopy.go]
- docs/apiref.adoc — ToolchainConfig documentation includes verification config (needs update to document new enum/default) [::codeready-toolchain/api::api/v1alpha1/docs/apiref.adoc:2230-2292, 3502-3512]
Implication: API repo is the authoritative source for the new field and its generated artifacts (deepcopy/openapi/docs) must be updated and consumers (host-operator via go.mod replace) must depend on that updated code.
toolchain-common
- Test helpers construct and assert ToolchainConfig values (including RegistrationService.Verification secrets and options):
- pkg/test/config/toolchainconfig.go — many ToolchainConfigOption builders that set Verification.Secret.Twilio* and other verification options; will need new option(s) for phoneLookupMode to simplify tests [::codeready-toolchain/toolchain-common::pkg/test/config/toolchainconfig.go:520-542, 26-36]
- pkg/test/config/toolchainconfig_assertion.go — assertions/loaders for ToolchainConfig used in tests [::codeready-toolchain/toolchain-common::pkg/test/config/toolchainconfig_assertion.go:14-63]
Implication: update test helpers to support setting/expecting phoneLookupMode in tests and fixtures.
toolchain-e2e
- E2E fixtures, helpers and tests update/read ToolchainConfig:
- deploy/host-operator/*/toolchainconfig.yaml — multiple example/e2e/dev/ui fixtures that contain registrationService.verification entries; these should be updated to include phoneLookupMode where required [::codeready-toolchain/toolchain-e2e::deploy/host-operator/e2e-tests/toolchainconfig.yaml] [::codeready-toolchain/toolchain-e2e::deploy/host-operator/dev/toolchainconfig.yaml]
- testsupport/wait/host.go & testsupport/wait/conditions.go — helpers to Get/Wait/Update ToolchainConfig and criteria that check verification enabled/auto-approval; may need new wait/criterion for phoneLookupMode if tests assert on it [::codeready-toolchain/toolchain-e2e::testsupport/wait/host.go:1611-1733] [::codeready-toolchain/toolchain-e2e::testsupport/wait/conditions.go:416-420]
- many e2e tests call hostAwait.UpdateToolchainConfig(...) and may rely on verification settings — those updates/fixtures might need extension to set phoneLookupMode [::codeready-toolchain/toolchain-e2e::test/e2e/* and test/e2e/parallel/* references]
Implication: update e2e fixtures and test helper expectations to include the new field to avoid test regressions.
member-operator
- No matches/reference to phone/verification or ToolchainConfig in search results; likely unaffected directly by this change [::codeready-toolchain/member-operator::(no relevant matches)]
Overall summary: The PR adds a new enum field to the ToolchainConfig CRD. Relevant consumers are registration-service (reads verification config), toolchain-common (test builders/assertions), toolchain-e2e (fixtures/tests), and api (types + generated artifacts). Actions to consider: ensure api types/docs/generated files are updated (and consumers point to updated module), update registration-service if it must act on phoneLookupMode, and update test helpers & e2e fixtures to set/expect the new field.
Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
coderabbitai
Bot
added
dependencies
|
/test e2e |
- Update CRD with phoneLookupExcludedCountries field - Bump toolchainapi replace to pick up annotation reduction and new PhoneLookupExcludedCountries config Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
coderabbitai
Bot
removed
dependencies
Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
coderabbitai
Bot
added
dependencies
The CRD-only changes in this PR don't require a forked API module. Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Simplify PhoneLookupExcludedCountries description - Change x-kubernetes-list-type from atomic to set Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fbm3307, MatousJobanek, metlos 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 |
3 participants
related Prs:
API: codeready-toolchain/api#509
Registration-Service: codeready-toolchain/registration-service#595
toolchain-Common: codeready-toolchain/toolchain-common#532
toolchain-e2e: codeready-toolchain/toolchain-e2e#1285
Summary by CodeRabbit
New Features
Chores