Add username prefix restrictions to avoid unexpected failures - #1311
Add username prefix restrictions to avoid unexpected failures#1311rajivnathan wants to merge 2 commits into
Conversation
WalkthroughSetup now validates ChangesUsername Prefix Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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 `@setup/cmd/root.go`:
- Around line 144-155: Update the usernamePrefix validation in the user setup
flow around usersignup.TransformUsername to validate the longest username
userRoutine will generate, reserving the separator and numeric suffix; account
for the additional suffix digit when numberOfUsers exceeds 9,999. Ensure setup
uses the same effective length constraints as host-operator, and add boundary
coverage for 15- and 16-character prefixes with 9,999 and 10,000 users.
🪄 Autofix
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: c8cf42d9-9418-4009-a5d3-5f96a7def76a
📒 Files selected for processing (1)
setup/cmd/root.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. (2)
- GitHub Check: govulncheck
- 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:
setup/cmd/root.go
🔇 Additional comments (1)
setup/cmd/root.go (1)
14-14: LGTM!
| transformedUsername := usersignup.TransformUsername(usernamePrefix, []string{"openshift", "kube", "default", "redhat", "sandbox"}, []string{"admin"}) | ||
| if transformedUsername != usernamePrefix { | ||
| term.Fatalf(fmt.Errorf("username prefix '%s' would be transformed to '%s' by Dev Sandbox username restrictions: "+ | ||
| "must contain only lowercase alphanumeric characters or '-', "+ | ||
| "must not start or end with '-', "+ | ||
| "must not start with 'openshift', 'kube', 'default', 'redhat', or 'sandbox', "+ | ||
| "must not end with 'admin', "+ | ||
| "must not exceed 20 characters, "+ | ||
| "and must not be numeric-only", | ||
| usernamePrefix, transformedUsername), "invalid username value '%s'", usernamePrefix) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
module_dir="$(go list -m -f '{{.Dir}}' github.com/codeready-toolchain/toolchain-common)"
rg -n -C 4 'TransformUsername|20|admin' "$module_dir/pkg/usersignup"
rg -n -C 4 'usernamePrefix|fmt\.Sprintf\("%s-%04d"' --glob '*.go' .Repository: codeready-toolchain/toolchain-e2e
Length of output: 274
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- repository files ---'
fd -t f '(^go\.mod$|root\.go$|.*user.*signup.*|.*signup.*)' . | head -200
printf '%s\n' '--- relevant local references ---'
rg -n -C 8 'TransformUsername|usernamePrefix|fmt\.Sprintf\("%s-%04d"|users\.Create|curUserNum' setup go.mod go.sum 2>/dev/null || true
printf '%s\n' '--- dependency declaration ---'
cat go.mod | sed -n '1,180p'
printf '%s\n' '--- target sections ---'
sed -n '120,170p' setup/cmd/root.go
sed -n '430,480p' setup/cmd/root.goRepository: codeready-toolchain/toolchain-e2e
Length of output: 19780
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- username transformation implementation ---'
rg -n -C 12 'func TransformUsername|TransformUsername|Max.*Username|username.*20|20.*username|username.*length' . --glob '*.go' --glob '*.md' || true
printf '%s\n' '--- username consumers and tests ---'
rg -n -C 8 'TransformUsername|Create\(.*username|username.*restriction|numeric-only|must not exceed' . --glob '*.go' --glob '*.md' || trueRepository: codeready-toolchain/toolchain-common
Length of output: 14617
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- complete TransformUsername implementation ---'
sed -n '1,130p' pkg/usersignup/usersignup.go
printf '%s\n' '--- user creation path ---'
rg -n -C 12 'func Create|func Signup|UserSignup|TransformUsername' pkg --glob '*.go'Repository: codeready-toolchain/toolchain-common
Length of output: 50392
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- progress counter semantics ---'
sed -n '350,455p' setup/cmd/root.go
printf '%s\n' '--- users.Create implementation ---'
rg -n -C 10 'func Create|package users|users\.Create' setup . --glob '*.go' --glob '!vendor/**' | head -160
printf '%s\n' '--- generated-name boundary probe ---'
python3 - <<'PY'
for prefix_len in (14, 15, 16, 20):
prefix = "a" * prefix_len
for user_num in (1, 9999, 10000):
username = f"{prefix}-{user_num:04d}"
print(f"prefix={prefix_len:2d} user={user_num:5d} length={len(username):2d} username={username}")
PYRepository: codeready-toolchain/toolchain-e2e
Length of output: 15240
🏁 Script executed on selected repositories:
set -euo pipefail
printf '%s\n' '--- e2e user creation ---'
sed -n '18,95p' setup/users/create_users.go
printf '%s\n' '--- host-operator username handling ---'
rg -n -C 10 'TransformUsername|CompliantUsername|IdentityClaims|UserSignup.*Name|username.*MaxLength' . --glob '*.go' | head -260
Repositories: codeready-toolchain/toolchain-e2e, codeready-toolchain/host-operator
Length of output: 23522
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- host-operator files ---'
fd -t f -e go . | head -120
printf '%s\n' '--- compliant username generation ---'
rg -n -C 12 'TransformUsername|CompliantUsername|PreferredUsername|IdentityClaims' . --glob '*.go' | head -300Repository: codeready-toolchain/host-operator
Length of output: 29159
🏁 Script executed:
set -euo pipefail
rg -n -C 18 'usersignup\.TransformUsername|usersignup\.MaxLength|compliantUsername\s*[:=]|PreferredUsername' controllers pkg --glob '*.go' | head -260Repository: codeready-toolchain/host-operator
Length of output: 22479
🏁 Script executed:
set -euo pipefail
rg -n -C 14 'func ForSpace|ForSpace\(' setup testsupport --glob '*.go'Repository: codeready-toolchain/toolchain-e2e
Length of output: 29626
Validate the longest generated username.
userRoutine appends -%04d before calling users.Create. For a 16-character prefix, the generated username is 21 characters. host-operator truncates it to 20 characters, while setup waits for a Space using the untruncated name. The setup then waits for the wrong Space. Reserve the separator and numeric suffix when validating usernamePrefix, including the extra digit when numberOfUsers exceeds 9,999. Add boundary tests for 15/16-character prefixes and user counts of 9,999/10,000.
🤖 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 `@setup/cmd/root.go` around lines 144 - 155, Update the usernamePrefix
validation in the user setup flow around usersignup.TransformUsername to
validate the longest username userRoutine will generate, reserving the separator
and numeric suffix; account for the additional suffix digit when numberOfUsers
exceeds 9,999. Ensure setup uses the same effective length constraints as
host-operator, and add boundary coverage for 15- and 16-character prefixes with
9,999 and 10,000 users.
|
MatousJobanek
left a comment
MatousJobanek
left a comment
There was a problem hiding this comment.
I'm wondering, why not using TransformUsername when calling the ForSpace?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, 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 |
3 participants
Another approach to try to address #1308
Summary by CodeRabbit