ROSAENG-60614 | feat: allow partial component route updates#3403
ROSAENG-60614 | feat: allow partial component route updates#3403reedcort wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
dco-signoff: yes
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: reedcort The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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:
📝 WalkthroughWalkthroughIngress component-route parsing now accepts partial route sets instead of requiring every allowed route. The CLI help text documents HCP route constraints and the required Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cdbcf23 to
7ce958b
Compare
openshift-ci
Bot
removed
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/edit/ingress/cmd.go (1)
62-69: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep the ingress error text consistent or update the e2e assertions
tests/e2e/test_rosacli_ingress.gostill expects"Expected exactly one command line parameter containing the id of the ingress"in two places, so the lowercase change incmd/edit/ingress/cmd.gowill break those checks unless they’re updated too.🤖 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 `@cmd/edit/ingress/cmd.go` around lines 62 - 69, Keep the validation error text in the ingress command’s Args function consistent with the existing e2e assertions, or update both assertions in test_rosacli_ingress.go to match the new lowercase wording; ensure all occurrences use the same exact message.Source: Learnings
🧹 Nitpick comments (1)
cmd/edit/ingress/flags.go (1)
96-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider mentioning partial-update support in flag help.
The updated help text lists available keys per cluster type but doesn't explicitly note that any subset of keys can be supplied and unspecified routes are left unchanged, which is the core behavior change of this PR.
🤖 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 `@cmd/edit/ingress/flags.go` around lines 96 - 99, Update the component routes flag help text in the flag definition near the ingress configuration to state that any subset of supported route keys may be supplied and unspecified routes remain unchanged. Preserve the existing key list and hostname/tlsSecretRef format guidance.
🤖 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.
Outside diff comments:
In `@cmd/edit/ingress/cmd.go`:
- Around line 62-69: Keep the validation error text in the ingress command’s
Args function consistent with the existing e2e assertions, or update both
assertions in test_rosacli_ingress.go to match the new lowercase wording; ensure
all occurrences use the same exact message.
---
Nitpick comments:
In `@cmd/edit/ingress/flags.go`:
- Around line 96-99: Update the component routes flag help text in the flag
definition near the ingress configuration to state that any subset of supported
route keys may be supplied and unspecified routes remain unchanged. Preserve the
existing key list and hostname/tlsSecretRef format guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 52dd1458-85cf-4d6f-8ce4-725f016b194a
📒 Files selected for processing (3)
cmd/edit/ingress/cmd.gocmd/edit/ingress/cmd_test.gocmd/edit/ingress/flags.go
|
@reedcort thanks for submitting the PR. How is expected to delete an existing route, for example, I added console + download and now I want only console. What would be the flow? @coderabbitai pinging you in case this already exists in the code, check. Also check if we have other commands working in a similar way (partial updates/keeps things unchanged). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Relevant ingress files ==='
fd -t f . cmd/edit/ingress | sort
echo '=== Ingress source outline ==='
ast-grep outline cmd/edit/ingress --items all 2>/dev/null || true
echo '=== Component-route symbols and request/update construction ==='
rg -n -C 5 --glob '*.go' \
'parseComponentRoutes|component-routes|ComponentRoutes|componentRoutes|TlsSecretRef|Hostname|Ingress' \
cmd/edit/ingress cmd 2>/dev/null || true
echo '=== Edit commands that explicitly clear/reset/remove fields ==='
rg -n -i -C 4 --glob '*.go' \
'(clear|unset|remove|delete|reset|nil).*flag|flag.*(clear|unset|remove|delete|reset)|Set[A-Za-z0-9_]*\(\s*(nil|""|false|0)\s*\)' \
cmd/edit 2>/dev/null || true
echo '=== Partial-update mechanisms and PATCH/update calls across edit commands ==='
rg -n -C 4 --glob '*.go' \
'\.(Patch|Update|Edit)\(|New.*Patch|patch\.|Set[A-Za-z0-9_]+\(' \
cmd/edit 2>/dev/null || true
echo '=== PR metadata and changed files ==='
gh pr view 3403 --repo openshift/rosa --json files,body,url 2>/dev/null || trueLength of output: 174516 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== OCM UpdateIngress wrapper and its tests ==='
rg -n -C 8 --glob '*.go' \
'func \(.*\) UpdateIngress\b|UpdateIngress\(|ComponentRoutes\(' \
pkg internal cmd 2>/dev/null || true
echo '=== Ingress command request tests (method, payload, and component routes) ==='
rg -n -C 10 --glob '*test.go' \
'UpdateIngress|ComponentRoutes|component-routes|MethodPatch|MethodPost|MethodPut' \
cmd/edit/ingress pkg 2>/dev/null || true
echo '=== OCM SDK version and locally cached ingress service implementation ==='
rg -n 'openshift-online/ocm-sdk-go' go.mod go.sum 2>/dev/null || true
sdk_dir="$(go env GOPATH 2>/dev/null)/pkg/mod/github.com/openshift-online"
if [ -d "$sdk_dir" ]; then
fd -t f . "$sdk_dir" | rg 'clustersmgmt.*/v1/.+(ingress|component_route)|ingress.*\.go|component_route.*\.go' | head -80
fi
echo '=== Focused comparable partial-update command code ==='
for f in cmd/edit/service/cmd.go cmd/edit/cluster/cmd.go; do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all 2>/dev/null || true
fi
done
sed -n '80,120p' cmd/edit/service/cmd.go 2>/dev/null || true
sed -n '790,830p' cmd/edit/cluster/cmd.go 2>/dev/null || true
echo '=== Explicit command-level removal interfaces in edit commands ==='
rg -n -i -C 3 --glob '*.go' \
'(remove-|delete-|clear-|unset-|disable-).*(flag|option)|flag.*(remove|delete|clear|unset|disable)' \
cmd/edit 2>/dev/null || trueLength of output: 56753
rosa edit ingress -c <cluster> \
--component-routes 'console: hostname=console.example.com;tlsSecretRef=console-tls' \
<ingress-id>With existing There is currently no This partial/unchanged behavior matches other edit flows, notably So I’d recommend documenting this limitation or tracking route removal separately if the underlying OCM API supports a deletion representation. |
|
@amandahla Assuming both console and download routes are set to remove a single route you just need to pass it with empty values.
This clears downloads while leaving console unchanged.` |
|
@coderabbitai do we have a e2e test for component routes? Should we update something? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Candidate test directories/files ==='
fd -t f -i '(e2e|integration|acceptance|functional|test)' . \
| rg -i '(^|/)(e2e|integration|acceptance|functional|test)(/|_|\.|$)|ingress|route' \
| sort \
| head -300
echo '=== All component-route and ingress edit test references ==='
rg -n -i -C 5 --glob '*_test.go' --glob '*.feature' --glob '*.yaml' --glob '*.yml' \
'(component-routes|component routes|ComponentRoutes|edit ingress|UpdateIngress|TlsSecretRef)' \
. 2>/dev/null || true
echo '=== Test framework/dependency clues ==='
rg -n -i -C 3 --glob 'go.mod' --glob '*.go' --glob '*.feature' \
'(ginkgo|gomega|e2e|integration|acceptance|cucumber|godog)' \
. 2>/dev/null | head -400 || true
echo '=== Ingress command test layout and focused contents ==='
ast-grep outline cmd/edit/ingress --items all 2>/dev/null || true
sed -n '1,280p' cmd/edit/ingress/cmd_test.go 2>/dev/null || true
echo '=== Test-related CI/workflow entries ==='
rg -n -i -C 3 --glob '.github/workflows/*' --glob 'Makefile' --glob 'makefile' \
'(e2e|integration|acceptance|functional)' \
. 2>/dev/null || trueLength of output: 57742 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== E2E ingress test structure ==='
ast-grep outline tests/e2e/test_rosacli_ingress.go --items all 2>/dev/null || true
echo '=== E2E ingress test: default and edit contexts ==='
sed -n '1,540p' tests/e2e/test_rosacli_ingress.go
echo '=== CLI ingress helper definitions and use sites ==='
rg -n -C 5 --glob '*.go' \
'(type .*Ingress|func \(.*\) (Edit|Describe|List|Create|Delete).*Ingress|EditIngress\(|DescribeIngress\()' \
tests 2>/dev/null || true
echo '=== Component-routes references specifically in executable E2E tests ==='
rg -n -i -C 5 --glob '*.go' \
'(component-routes|ComponentRoutes|TlsSecretRef)' \
tests/e2e tests/utils/exec 2>/dev/null || true
echo '=== Ingress feature labels and E2E configuration guards ==='
rg -n -C 4 --glob '*.go' \
'(Ingress|Hypershift|HCP|PrivateLink|STS)' \
tests/ci/labels tests/e2e/e2e_pre_check.go tests/e2e/e2e_setup_test.go tests/utils/config 2>/dev/null || trueLength of output: 51932 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== E2E ingress reflected model and describe parsing ==='
sed -n '52,112p' tests/utils/exec/rosacli/ingress_service.go
echo '=== Canonical component-routes describe output and assertions ==='
ast-grep outline cmd/describe/ingress/cmd_test.go --items all 2>/dev/null || true
sed -n '15,145p' cmd/describe/ingress/cmd_test.go
echo '=== Other E2E helpers that parse nested YAML maps ==='
rg -n -C 6 --glob '*.go' \
'(YamlToMap\(\)|map\[string\].*yaml|yaml:".*Routes|yaml:".*Config)' \
tests/utils/exec tests/e2e 2>/dev/null | head -280 || trueLength of output: 32267
I recommend adding, at minimum, a command/request-level test in this PR that verifies a partial invocation—for example, only An E2E test would also be valuable, but needs a small amount of test infrastructure first:
So: no existing E2E test; add the sparse-payload test now, and add the E2E scenario when the test environment can safely supply and restore valid route values. |
@reedcort Could you paste a short before/after from a live cluster into the PR description (or a comment)? Something like
That would make the OCM merge behavior easy for reviewers to verify alongside the unit tests. Appreciate it! |
|
@coderabbitai create an issue referencing this PR Details from Cursor: rosa edit ingress -c <cluster> \
--component-routes 'downloads: hostname=;tlsSecretRef=' <ingress-id>
That is hard to discover, easy to mistype, and fragile in shells because the DSL uses ; (command separator) unless the whole value is quoted.
A clearer follow-up would be something like:
rosa edit ingress <ingress-id> -c <cluster> \
--component-route name=console,hostname=console.example.com,tlsSecretRef=console-tls
rosa edit ingress <ingress-id> -c <cluster> \
--remove-component-route downloads
Why this is better UX than empty keys:
Intent is explicit (remove vs update), so omit ≠ delete stays unambiguous.
Users don’t need to remember hostname / tlsSecretRef just to clear a route.
Avoids shell breakage from unquoted ; / trailing = in the clear path.
Matches other ROSA edit patterns where inverse actions are explicit (e.g. boolean toggles), rather than “pass empty values”. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openshift/rosa"
pr_number="3403"
pr_url="https://github.com/${repo}/pull/${pr_number}"
comment_url="$(
gh api --paginate "repos/${repo}/issues/${pr_number}/comments" \
--jq '.[] | select(.user.login == "amandahla" and (.body | contains("create an issue referencing this PR"))) | .html_url' \
| tail -n 1
)"
if [ -z "$comment_url" ]; then
comment_url="${pr_url}`#issuecomment-unavailable`"
fi
body="$(cat <<EOF
## Summary
Follow up on the component-route UX introduced in #${pr_number}. Evaluate a clearer, explicit interface for updating and removing individual ingress component routes.
## Current behavior and problem
\`rosa edit ingress --component-routes\` supports partial updates: routes omitted from an invocation remain unchanged. Removing a route currently relies on supplying the existing DSL with empty values, for example:
\`\`\`bash
rosa edit ingress -c <cluster> \\
--component-routes 'downloads: hostname=;tlsSecretRef=' <ingress-id>
\`\`\`
This is difficult to discover, requires users to know the internal route fields, and can be easy to mistype or misquote because the DSL uses semicolons.
## Requested evaluation
1. Evaluate an explicit removal option, for example:
\`\`\`bash
rosa edit ingress <ingress-id> -c <cluster> \\
--remove-component-route downloads
\`\`\`
2. Evaluate more standard component-route update representations, such as:
- repeatable key-value input, for example \`--component-route name=console,hostname=console.example.com,tlsSecretRef=console-tls\`;
- a JSON or YAML payload alternative, if consistent with existing ROSA CLI conventions.
The final interface should preserve the distinction between:
- omitting a route: leave it unchanged;
- updating a route: set the supplied values;
- removing a route: explicit deletion/clear operation.
## Affected areas
- \`rosa edit ingress\` command and its flag parsing under \`cmd/edit/ingress\`
- CLI help text and parser/request tests
- Any applicable ingress E2E coverage or documentation
## Acceptance criteria
- [ ] The removal flow is explicit and documented; it does not require users to provide empty \`hostname\` and \`tlsSecretRef\` values.
- [ ] Partial update semantics remain unchanged: routes not named by update or removal options are preserved.
- [ ] The chosen update input format is evaluated for consistency, discoverability, shell safety, validation, and backward compatibility.
- [ ] Unit/request tests cover updating one route, removing one route while preserving another, invalid route names, and duplicate/conflicting input.
- [ ] CLI help documents update versus removal behavior and examples.
## References
- Originating pull request: ${pr_url}
- Originating discussion: ${comment_url}
- Requested by: `@amandahla`
EOF
)"
gh issue create \
--repo "$repo" \
--title "Improve ingress component-route update and removal UX" \
--body "$body" \
--assignee "amandahla"Length of output: 3150
It is assigned to you and includes the requested evaluation of:
|
@jerichokeyne wdyt? should we add one? |
7ce958b to
38aebc7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/edit/ingress/cmd_test.go (1)
98-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify the values produced by sparse route parsing.
The new tests prove only that route keys are present, not that the builders contain the supplied values. Assert each returned route’s
Hostname()andTlsSecretRef(); add a focused case for clearing a route with empty values.
cmd/edit/ingress/cmd_test.go#L98-L105: build and validate the partialconsoleroute.cmd/edit/ingress/cmd_test.go#L119-L127: build and validate the partialoauthanddownloadsroutes.🤖 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 `@cmd/edit/ingress/cmd_test.go` around lines 98 - 105, Strengthen the sparse route parsing tests in cmd/edit/ingress/cmd_test.go at lines 98-105 by asserting the returned console route’s Hostname() and TlsSecretRef() match the supplied values, and add a focused empty-values case that verifies clearing the route. At lines 119-127, assert the partial oauth and downloads routes’ Hostname() and TlsSecretRef() values as well.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@cmd/edit/ingress/cmd_test.go`:
- Around line 98-105: Strengthen the sparse route parsing tests in
cmd/edit/ingress/cmd_test.go at lines 98-105 by asserting the returned console
route’s Hostname() and TlsSecretRef() match the supplied values, and add a
focused empty-values case that verifies clearing the route. At lines 119-127,
assert the partial oauth and downloads routes’ Hostname() and TlsSecretRef()
values as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8282d7cd-0218-49b8-931d-5cc3a553d9b9
📒 Files selected for processing (2)
cmd/edit/ingress/cmd_test.gocmd/edit/ingress/flags.go
| @@ -107,13 +109,6 @@ func parseComponentRoutesForAllowed(input string, allowedRoutes []string) (map[s | |||
| result := map[string]*cmv1.ComponentRouteBuilder{} | |||
| input = strings.TrimSpace(input) | |||
| components := strings.Split(input, ",") | |||
There was a problem hiding this comment.
Now that componentRoutes can be sparse, this full-map DeepEqual no longer matches the CLI contract. A console-only update against an ingress that already has unchanged console plus existing downloads will still PATCH and report success, because the omitted key makes the maps unequal.
There was a problem hiding this comment.
Addressed — added sparseComponentRoutesEqual to fix no-op detection for partial updates.
Verified on live cluster:
# Set console
$ rosa edit ingress --component-routes 'console: hostname=console.custom.example.com;tlsSecretRef=console-tls-secret' j3t8
INFO: Updated ingress 'j3t8'
# Same console again — no-op detected
$ rosa edit ingress --component-routes 'console: hostname=console.custom.example.com;tlsSecretRef=console-tls-secret' j3t8
WARN: No need to update ingress as there are no changes
# Different console value — update sent
$ rosa edit ingress --component-routes 'console: hostname=console2.custom.example.com;tlsSecretRef=console-tls-secret' j3t8
INFO: Updated ingress 'j3t8'
|
The PR description now includes both cases this thread asked for: a console-only update that leaves downloads unchanged and a downloads clear with empty values that leaves console unchanged. That gives reviewers the backend-merge evidence needed for the behavior change. |
|
I wouldn't block this PR on a new live HCP E2E, but I would block on adding one focused command/runtime PATCH assertion now. The current diff only updates parser tests, so we still need a test that proves omitted routes are not serialized and empty values do serialize as a clear. |
6f0413c to
400cf5f
Compare
|
@reedcort: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Now that |
|
Given the new serialization tests plus the live-cluster proof in the PR description, I don't think this PR needs a new HCP E2E before merge. A follow-up E2E is still worthwhile once |
Remove the count check in parseComponentRoutesForAllowed so users can update a single component route without providing all routes. The OCM API supports partial updates and requiring the full set was overly restrictive. Name validation and duplicate detection are preserved. Signed-off-by: Cortney Reed <creed@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
400cf5f to
6492237
Compare
3 participants
PR Summary
Allow partial component route updates via
rosa edit ingress --component-routes— users can now set a single route (e.g. only console) without providing all routes.Detailed Description of the Issue
The previous change (#3376) followed the Classic CLI framework which required all routes to be set together (classic: oauth+console+downloads, HCP: console+downloads). The OCM API supports partial updates — setting only console leaves downloads unchanged — but the CLI rejected partial input with "the expected amount of component routes is N, but M have been supplied". After review feedback, the decision is to allow partial updates to match the API flexibility.
Related Issues and PRs
Type of Change
Previous Behavior
rosa edit ingress --component-routesrequired all routes for the platform — classic required oauth+console+downloads (3), HCP required console+downloads (2). Passing fewer routes returned:Behavior After This Change
Users can pass any subset of valid routes. Name validation and duplicate detection are preserved.
Invalid names and duplicates are still rejected:
How to Test (Step-by-Step)
Preconditions
hypershift-component-routesfeature toggle enabledopenshift-configon the hosted clusterTest Steps
rosa edit ingress -c <cluster> --component-routes 'console: hostname=console.example.com;tlsSecretRef=console-tls' <ingress-id>rosa edit ingress -c <cluster> --component-routes 'downloads: hostname=downloads.example.com;tlsSecretRef=downloads-tls' <ingress-id>rosa edit ingress -c <cluster> --component-routes 'oauth: hostname=x;tlsSecretRef=y' <ingress-id>rosa edit ingress -c <cluster> --component-routes 'console: hostname=a;tlsSecretRef=b, console: hostname=c;tlsSecretRef=d' <ingress-id>Expected Results
Proof of the Fix
Live cluster — partial updates and clearing
1. Set console only:
API state:
{"console": {"hostname": "console.custom.example.com", "tls_secret_ref": "console-tls-secret"}}2. Add downloads (console unchanged):
API state:
{"console": {...}, "downloads": {"hostname": "downloads.custom.example.com", "tls_secret_ref": "downloads-tls-secret"}}3. Clear downloads only (console unchanged):
API state:
{"console": {"hostname": "console.custom.example.com", "tls_secret_ref": "console-tls-secret"}}Breaking Changes
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit
component-routesCLI flag help text to clarify HCP-supported components and the requiredhostname+tlsSecretRefpair per route.