fix: make account verifier flag enum#1266
Conversation
WalkthroughThe ToolchainConfig CRD schema is updated to replace the ChangesAccount Verifier Configuration Schema
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
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)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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)
config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml (1)
162-171: 💤 Low valueConsider adding an explicit schema default for
accountVerifierMode.The description states it "Defaults to 'log'" but no
default:directive is set. Consumers must handle nil/empty values. Adding an explicit default would be clearer:accountVerifierMode: default: log ...If the default is intentionally applied in Go code rather than the CRD schema, this is acceptable—just verify the consumer handles the unset case.
🤖 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 `@config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml` around lines 162 - 171, The CRD schema for accountVerifierMode lacks an explicit default even though the description says it defaults to "log"; update the accountVerifierMode schema to include default: log so consumers get the expected value from the CRD (i.e., add a default field alongside enum/type for the accountVerifierMode schema), or if you prefer applying the default in code, ensure the reconciler/Go code that reads accountVerifierMode (where you handle registration config) explicitly sets "log" when the field is nil/empty.
🤖 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 162-171: The CRD added registrationService.accountVerifierMode but
the API type and controller logic aren’t wired: update the API by replacing
AccountVerifierEnabled *bool with a new AccountVerifierMode (string or typed
enum) on the ToolchainConfig RegistrationService spec and regenerate
deepcopy/OpenAPI/docs, then change the registration-service code to read
ToolchainConfig.Spec.RegistrationService.AccountVerifierMode (default to "log"
when empty) and enforce the enum in the verifyAccount() path (short-circuit and
skip verifier call when mode == "disabled", call but only log response when mode
== "log", and enforce when mode == "enabled"); also stop gating solely on
accountVerifierURL, update any helper methods that reference
AccountVerifierEnabled, and extend/adjust unit/integration tests to cover all
three modes and the default behavior.
---
Nitpick comments:
In `@config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml`:
- Around line 162-171: The CRD schema for accountVerifierMode lacks an explicit
default even though the description says it defaults to "log"; update the
accountVerifierMode schema to include default: log so consumers get the expected
value from the CRD (i.e., add a default field alongside enum/type for the
accountVerifierMode schema), or if you prefer applying the default in code,
ensure the reconciler/Go code that reads accountVerifierMode (where you handle
registration config) explicitly sets "log" when the field is nil/empty.
🪄 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: d98a28f5-32f6-4b7e-b74e-b4e202d2a99d
📒 Files selected for processing (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:
config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml
🔀 Multi-repo context codeready-toolchain/registration-service, codeready-toolchain/api, codeready-toolchain/member-operator, codeready-toolchain/toolchain-common, codeready-toolchain/toolchain-e2e
codeready-toolchain/registration-service
- pkg/signup/service/signup_service.go:246-286 — contains HTTP client code calling an external account-verifier endpoint and types accountVerifierRequest, accountVerifierResponse, accountVerifierReason. This code uses the account-verifier concept and may rely on configuration controlled by ToolchainConfig.accountVerifier* fields. [::codeready-toolchain/registration-service::]
codeready-toolchain/api
- api/v1alpha1/toolchainconfig_types.go:263-267 — defines ToolchainConfig fields:
- AccountVerifierURL *string
json:"accountVerifierURL,omitempty" - AccountVerifierEnabled *bool
json:"accountVerifierEnabled,omitempty"(old boolean field still present here)
- AccountVerifierURL *string
- api/v1alpha1/zz_generated.openapi.go:2654-2661 — OpenAPI schema includes "accountVerifierEnabled".
- api/v1alpha1/docs/apiref.adoc:2140-2142 — docs reference
accountVerifierEnabledand describe behavior.
These indicate the api repo still contains the old boolean field and generated artifacts that will need updating to match the new enum CRD (accountVerifierMode). [::codeready-toolchain/api::]
codeready-toolchain/member-operator
- Repository clone failed during search (network/clone error). Could not scan for references to accountVerifier*; recommend re-running searches or checking member-operator for ToolchainConfig usage and mapping of registrationService config. [::codeready-toolchain/member-operator::]
codeready-toolchain/toolchain-common
- No matches for accountVerifier* found. [::codeready-toolchain/toolchain-common::]
codeready-toolchain/toolchain-e2e
- No matches for accountVerifier* found. [::codeready-toolchain/toolchain-e2e::]
Summary / impact
- registration-service is a direct consumer of account-verifier functionality; ensure it reads the new ToolchainConfig.registrationService.accountVerifierMode (enum) instead of the removed boolean.
- api repo still defines/generated docs for accountVerifierEnabled — must be updated to accountVerifierMode and regenerated.
- member-operator could not be scanned due to clone error; verify there are no references or CR usage that assume the boolean field.
| accountVerifierMode: | ||
| description: |- | ||
| AccountVerifierMode controls how the registration service handles responses from the account-verifier service. | ||
| Valid values are "disabled" (do not call the verifier), "log" (call but only log the response), | ||
| and "enabled" (call and enforce the response). Defaults to "log". | ||
| enum: | ||
| - disabled | ||
| - log | ||
| - enabled | ||
| type: string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if registration-service reads the new field or still references the old one
rg -n "accountVerifier" --type goRepository: codeready-toolchain/host-operator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check api repo for the field definitions
rg -n "AccountVerifier" --type goRepository: codeready-toolchain/host-operator
Length of output: 59
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "accountVerifier(Enabled|Mode)" . || true
rg -n "accountVerifierURL" . || trueRepository: codeready-toolchain/registration-service
Length of output: 66
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "accountVerifier(Enabled|Mode)" . || trueRepository: codeready-toolchain/api
Length of output: 481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '140,220p' config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yamlRepository: codeready-toolchain/host-operator
Length of output: 4428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== matches: AccountVerifierEnabled =="
rg -n "AccountVerifierEnabled" --type go . || true
echo "== matches: accountVerifierEnabled =="
rg -n "accountVerifierEnabled" . || true
echo "== matches: AccountVerifierMode =="
rg -n "AccountVerifierMode" --type go . || true
echo "== matches: accountVerifierMode =="
rg -n "accountVerifierMode" . || true
echo "== matches: accountVerifier (any) =="
rg -n "accountVerifier" --type go . || trueRepository: codeready-toolchain/registration-service
Length of output: 1075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml"
echo "== accountVerifierEnabled occurrences =="
rg -n "accountVerifierEnabled" "$FILE" || true
echo "== accountVerifierMode occurrences + nearby default =="
rg -n -C 2 "accountVerifierMode" "$FILE" || trueRepository: codeready-toolchain/host-operator
Length of output: 534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml"
rg -n "accountVerifierEnabled" "$FILE" || true
rg -n -C 3 "accountVerifierMode" "$FILE" || true
rg -n -C 3 "default:.*log|default: \"log\"|default: log" "$FILE" || trueRepository: codeready-toolchain/host-operator
Length of output: 607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "AccountVerifierURL|accountVerifierURL" . --type go || true
rg -n "AccountVerifierEnabled|accountVerifierEnabled" . --type go || true
rg -n "AccountVerifierMode|accountVerifierMode" . --type go || true
rg -n "verifierURL" pkg/signup/service/signup_service.go || true
sed -n '200,340p' pkg/signup/service/signup_service.goRepository: codeready-toolchain/registration-service
Length of output: 6169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' pkg/configuration/configuration.go
rg -n "accountVerifierMode|AccountVerifierMode|accountVerifierEnabled|AccountVerifierEnabled" pkg/configuration -S || true
rg -n "RegistrationServiceConfig" pkg/configuration/configuration.go || trueRepository: codeready-toolchain/registration-service
Length of output: 9459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml"
sed -n '150,200p' "$FILE"Repository: codeready-toolchain/host-operator
Length of output: 2748
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "accountVerifierMode|AccountVerifierMode" . --type go || true
rg -n "accountVerifierEnabled|AccountVerifierEnabled" . --type go || trueRepository: codeready-toolchain/api
Length of output: 809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "accountVerifierMode|AccountVerifierMode|accountVerifierEnabled|AccountVerifierEnabled" . || trueRepository: codeready-toolchain/member-operator
Length of output: 61
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "accountVerifierMode|AccountVerifierMode|accountVerifierEnabled|AccountVerifierEnabled" . || trueRepository: codeready-toolchain/toolchain-common
Length of output: 331
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "accountVerifierMode|AccountVerifierMode|accountVerifierEnabled|AccountVerifierEnabled" . || trueRepository: codeready-toolchain/toolchain-e2e
Length of output: 59
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,330p' api/v1alpha1/toolchainconfig_types.go
rg -n "AccountVerifierURL|accountVerifierURL|AccountVerifierMode|accountVerifierMode" api/v1alpha1/toolchainconfig_types.go || trueRepository: codeready-toolchain/api
Length of output: 5119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "AccountVerifierMode|accountVerifierMode" pkg/test/config toolchain-common pkg --type go || true
rg -n "AccountVerifierEnabled|accountVerifierEnabled" pkg/test/config -S --type go || trueRepository: codeready-toolchain/toolchain-common
Length of output: 402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "AccountVerifierURL|accountVerifierURL|AccountVerifierEnabled|AccountVerifierMode|accountVerifierMode" pkg --type go || trueRepository: codeready-toolchain/registration-service
Length of output: 827
Fix accountVerifierMode wiring: CRD adds it, but controllers don’t read/enforce it
config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yamladdsregistrationService.accountVerifierMode(enum) but provides no CRDdefault:and the “Defaults to 'log'” text won’t be honored unless the controller applies it.codeready-toolchain/registration-serviceonly gates onregistrationService.accountVerifierURL(verifyAccount()always calls the verifier when the URL is set; there’s no use ofaccountVerifierModeoraccountVerifierEnabled), so"disabled"/"enabled"behavior is currently non-functional.codeready-toolchain/apistill definesAccountVerifierEnabled *bool(and generated OpenAPI/docs referenceaccountVerifierEnabled), so the newaccountVerifierModefield won’t be present in the Go types/deserialization.
Update the API types (add AccountVerifierMode, regenerate deepcopy/openapi/docs, remove the boolean), then update registration-service to implement the enum semantics and default to "log" when unset, and adjust/extend tests accordingly.
🤖 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 `@config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml` around
lines 162 - 171, The CRD added registrationService.accountVerifierMode but the
API type and controller logic aren’t wired: update the API by replacing
AccountVerifierEnabled *bool with a new AccountVerifierMode (string or typed
enum) on the ToolchainConfig RegistrationService spec and regenerate
deepcopy/OpenAPI/docs, then change the registration-service code to read
ToolchainConfig.Spec.RegistrationService.AccountVerifierMode (default to "log"
when empty) and enforce the enum in the verifyAccount() path (short-circuit and
skip verifier call when mode == "disabled", call but only log response when mode
== "log", and enforce when mode == "enabled"); also stop gating solely on
accountVerifierURL, update any helper methods that reference
AccountVerifierEnabled, and extend/adjust unit/integration tests to cover all
three modes and the default behavior.
There was a problem hiding this comment.
I still need to update the registration-service PR, will do after merging this one.
There was a problem hiding this comment.
@mfrancisc, sounds good — this CRD change is a reasonable first step. Would you like me to open a GitHub issue to track the remaining follow-up work (updating codeready-toolchain/api to replace AccountVerifierEnabled *bool with AccountVerifierMode, regenerating deepcopy/OpenAPI/docs, and updating registration-service to read and enforce the enum)? That way nothing slips through the cracks.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: metlos, mfrancisc 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 |
|
/retest |
2 participants
generated by codeready-toolchain/api#507
Summary by CodeRabbit