feat: handle usersignup rejected state#1267
Open
mfrancisc wants to merge 5 commits into
Open
Conversation
Add setStatusRejected status updater and wire it into the reconcile loop so users with the Rejected state are labelled and their status is set to Complete/Rejected instead of falling through to provisioning. Co-authored-by: Cursor <cursoragent@cursor.com>
Cover the reconcile path where a UserSignup has the Rejected state set, asserting the state label is updated to "rejected" and the Complete condition is set to True with reason Rejected. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📜 Recent 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)
🧰 Additional context used🔀 Multi-repo context codeready-toolchain/registration-service, codeready-toolchain/toolchain-e2e, codeready-toolchain/api, codeready-toolchain/toolchain-common, codeready-toolchain/member-operatorcodeready-toolchain/registration-service
codeready-toolchain/toolchain-e2e
codeready-toolchain/api
codeready-toolchain/toolchain-common
codeready-toolchain/member-operator
Overall conclusion
WalkthroughThis PR implements a Rejected state for UserSignup by adding a status updater method, inserting a Reconcile branch that sets the Rejected label and status and returns early, adding a controller test verifying no provisioning occurs for rejected users, and updating go.mod dependency versions. ChangesUser signup rejected state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/usersignup/usersignup_controller.go (1)
170-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRejected-state check is ordered too late in reconcile flow.
At Line 170,
checkIfMurAlreadyExists(...)can return early before the new rejected branch (Line 185), so rejected signups with an existing MUR can bypass rejected handling and continue the non-rejected path.Proposed fix
- if exists, err := r.checkIfMurAlreadyExists(ctx, config, userSignup, banned); err != nil || exists { - return reconcile.Result{}, err - } - // If there is no MasterUserRecord created, yet the UserSignup is Banned, simply set the status // and return if banned { // if the UserSignup doesn't have the state=banned label set, then update it if err := r.setStateLabel(ctx, config, userSignup, toolchainv1alpha1.UserSignupStateLabelValueBanned); err != nil { return reconcile.Result{}, err } return reconcile.Result{}, r.updateStatus(ctx, userSignup, r.setStatusBanned) } // Check if the user has been rejected if states.Rejected(userSignup) { if err := r.setStateLabel(ctx, config, userSignup, toolchainv1alpha1.UserSignupStateLabelValueRejected); err != nil { return reconcile.Result{}, err } return reconcile.Result{}, r.updateStatus(ctx, userSignup, r.setStatusRejected) } + + if exists, err := r.checkIfMurAlreadyExists(ctx, config, userSignup, banned); err != nil || exists { + return reconcile.Result{}, err + }Also applies to: 185-191
🤖 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 `@controllers/usersignup/usersignup_controller.go` around lines 170 - 172, The current reconcile flow calls checkIfMurAlreadyExists(ctx, config, userSignup, banned) before handling the "rejected" branch, which allows rejected userSignups with existing MURs to bypass rejected handling; fix by moving the rejected-state check so that the code inspects userSignup's rejected state (the branch currently at lines ~185-191) and executes the rejected handling path before calling checkIfMurAlreadyExists, or alternatively adjust the conditional around checkIfMurAlreadyExists to skip/return if userSignup.IsRejected() so checkIfMurAlreadyExists never short-circuits a rejected signup; reference checkIfMurAlreadyExists, userSignup (the rejected branch currently after it), and the reconcile return path to locate and reorder/update the logic.
🤖 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 160-163: Remove the local path replace directives from go.mod that
reference github.com/codeready-toolchain/api and
github.com/codeready-toolchain/toolchain-common; instead revert to canonical
module versions (or move those local overrides into a developer-only go.work) so
module resolution is deterministic in CI. Locate the replace block containing
"github.com/codeready-toolchain/api => ../api" and
"github.com/codeready-toolchain/toolchain-common => ../toolchain-common" and
delete those lines (or relocate them to a go.work used only for local
development).
---
Outside diff comments:
In `@controllers/usersignup/usersignup_controller.go`:
- Around line 170-172: The current reconcile flow calls
checkIfMurAlreadyExists(ctx, config, userSignup, banned) before handling the
"rejected" branch, which allows rejected userSignups with existing MURs to
bypass rejected handling; fix by moving the rejected-state check so that the
code inspects userSignup's rejected state (the branch currently at lines
~185-191) and executes the rejected handling path before calling
checkIfMurAlreadyExists, or alternatively adjust the conditional around
checkIfMurAlreadyExists to skip/return if userSignup.IsRejected() so
checkIfMurAlreadyExists never short-circuits a rejected signup; reference
checkIfMurAlreadyExists, userSignup (the rejected branch currently after it),
and the reconcile return path to locate and reorder/update the logic.
🪄 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: 81e07af2-48dd-457d-b66b-1eb516e5b3c7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
controllers/usersignup/status_updater.gocontrollers/usersignup/usersignup_controller.gocontrollers/usersignup/usersignup_controller_test.gogo.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). (2)
- 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:
controllers/usersignup/usersignup_controller.gocontrollers/usersignup/usersignup_controller_test.gogo.modcontrollers/usersignup/status_updater.go
🔀 Multi-repo context codeready-toolchain/api, codeready-toolchain/toolchain-common, codeready-toolchain/registration-service, codeready-toolchain/member-operator, codeready-toolchain/toolchain-e2e
Linked repositories findings
-
[::codeready-toolchain/api::]
- Contains UserSignupComplete condition type (api/v1alpha1/usersignup_types.go:12-13). The host-operator PR introduces a new status reason constant (UserSignupUserRejectedReason) which is not present in this file — API repo likely needs an update to add that reason. [::codeready-toolchain/api::api/v1alpha1/usersignup_types.go:12-13]
-
[::codeready-toolchain/toolchain-common::]
- Tests/helpers reference UserSignupComplete (pkg/test/usersignup/usersignup.go lines ~95,110,123,164,176). I did not find an existing SetRejected helper or a 'rejected' state constant in the scanned locations; the host-operator PR calls states.SetRejected, so toolchain-common must provide the new state constant/helper (PR
#531referenced). [::codeready-toolchain/toolchain-common::pkg/test/usersignup/usersignup.go:95,110,123,164,176]
- Tests/helpers reference UserSignupComplete (pkg/test/usersignup/usersignup.go lines ~95,110,123,164,176). I did not find an existing SetRejected helper or a 'rejected' state constant in the scanned locations; the host-operator PR calls states.SetRejected, so toolchain-common must provide the new state constant/helper (PR
-
[::codeready-toolchain/registration-service::]
- Many locations read the UserSignupComplete condition (tests and service code), e.g. pkg/signup/service/signup_service.go lines referencing condition.FindConditionByType(..., UserSignupComplete). If host-operator sets the Complete condition with reason Rejected, registration-service behavior that checks Complete may be affected; consider whether registration-service expects particular reasons. [::codeready-toolchain/registration-service::pkg/signup/service/signup_service.go:330,442 and test/fake/conditions.go:11]
-
[::codeready-toolchain/member-operator::]
- No matches for rejected-state/status changes found in the scan; no direct impact observed.
-
[::codeready-toolchain/toolchain-e2e::]
- Tests wait on UserSignupComplete condition in many helpers (testsupport/wait/conditions.go). Changing Complete reason to Rejected should be visible to e2e checks that inspect condition reason/status; review tests that assert specific reasons if any. [::codeready-toolchain/toolchain-e2e::testsupport/wait/conditions.go:multiple]
Summary / action items:
- codeready-toolchain/api: add the new status reason constant (UserSignupUserRejectedReason) so host-operator builds against API types.
- codeready-toolchain/toolchain-common: add 'rejected' state constant and states.SetRejected helper used by host-operator.
- Review registration-service and e2e tests for any logic that inspects the UserSignup Complete condition reason (they already inspect Complete); ensure they accept the new Rejected reason where appropriate.
🔇 Additional comments (1)
controllers/usersignup/status_updater.go (1)
5-5: LGTM!Also applies to: 186-198
MatousJobanek
approved these changes
May 29, 2026
Contributor
|
btw, it would be nice to update also e2e tests |
metlos
approved these changes
Jun 1, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, 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 |
coderabbitai
Bot
removed
dependencies
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`:
- Line 67: The go.mod lists an indirect dependency
"github.com/go-bindata/go-bindata v3.1.2+incompatible" that likely isn't
required; run "go mod why github.com/go-bindata/go-bindata" and search the repo
for any imports referencing "github.com/go-bindata/go-bindata" (or a /v3
variant) to confirm usage, then either remove the stale indirect entry by
running "go mod tidy" (if unused) or update imports and the go.mod to the
canonical "/v3" module if the v3 API is actually required; ensure the final
change updates the go.mod entry accordingly and re-runs tests/build.
🪄 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: 8735911f-2110-4ca3-ad95-d267c10fb0fc
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.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). (2)
- 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.mod
🔀 Multi-repo context codeready-toolchain/api, codeready-toolchain/toolchain-common, codeready-toolchain/registration-service, codeready-toolchain/toolchain-e2e, codeready-toolchain/member-operator
codeready-toolchain/api
- Defines the new rejected state/value and reason:
- api/v1alpha1/usersignup_types.go:80 — UserSignupStateLabelValueRejected = "rejected" [::codeready-toolchain/api::api/v1alpha1/usersignup_types.go:80]
- api/v1alpha1/usersignup_types.go:171-173 — UserSignupStateRejected = UserSignupState("rejected") [::codeready-toolchain/api::api/v1alpha1/usersignup_types.go:171-173]
- api/v1alpha1/usersignup_types.go:110 — UserSignupUserRejectedReason = "Rejected" [::codeready-toolchain/api::api/v1alpha1/usersignup_types.go:110]
- Confirms UserSignupComplete ConditionType exists (Complete) [::codeready-toolchain/api::api/v1alpha1/usersignup_types.go:12-13]
codeready-toolchain/toolchain-common
- Provides state helpers for rejected:
- pkg/states/state_manager.go:52-57 — Rejected(...) and SetRejected(...) implementations that operate on UserSignupStateRejected [::codeready-toolchain/toolchain-common::pkg/states/state_manager.go:52-57]
- Test helpers reference UserSignupComplete condition in multiple locations (tests/usersignup helper) [::codeready-toolchain/toolchain-common::pkg/test/usersignup/usersignup.go:95,110,123,164,176]
codeready-toolchain/registration-service
- Reads/checks UserSignupComplete condition in service logic:
- pkg/signup/service/signup_service.go:330,442,456-458 — finds/uses UserSignupComplete and checks its Status before proceeding (uses checkUserSignupCompleted flag) [::codeready-toolchain/registration-service::pkg/signup/service/signup_service.go:330,442,456-458]
- Tests and test fakes reference UserSignupComplete:
- test/fake/conditions.go:11 — builds condition of type UserSignupComplete [::codeready-toolchain/registration-service::test/fake/conditions.go:11]
- Also uses a "Rejected" metric label in proxy metrics/tests:
- pkg/proxy/metrics/proxy_metrics.go:8 — MetricLabelRejected = "Rejected" and multiple test files reference rejected headers/behaviour [::codeready-toolchain/registration-service::pkg/proxy/metrics/proxy_metrics.go:8]
codeready-toolchain/toolchain-e2e
- Many e2e helpers wait on UserSignupComplete condition (testsupport/wait/conditions.go multiple lines) — tests may observe the Complete condition and its Reason/Status [::codeready-toolchain/toolchain-e2e::testsupport/wait/conditions.go:multiple]
codeready-toolchain/member-operator
- No matches found for rejected/state changes or UserSignupComplete in search results (no stdout) [::codeready-toolchain/member-operator::]
Conclusion: The API and toolchain-common already contain the 'rejected' state and UserSignupUserRejectedReason and a SetRejected helper; registration-service and e2e reference UserSignupComplete (and registration-service checks condition Status). These repositories are relevant consumers of the status change introduced in the PR.
🔇 Additional comments (2)
go.mod (2)
161-164: Local replace directives are already flagged.This concern has been raised in a previous review.
5-6: LGTM!
Contributor
Author
|
coderabbitai
Bot
added
dependencies
|
coderabbitai
Bot
removed
dependencies
coderabbitai
Bot
removed
the
test
Contributor
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add setStatusRejected status updater and wire it into the reconcile loop so users with the Rejected state are labelled and their status is set to Complete/Rejected instead of falling through to provisioning.
related PRs:
Summary by CodeRabbit
New Features
Tests