fix(route): add --desc flag + harden e2e gateway-group resolver#39
Open
shreemaan-abhishek wants to merge 3 commits into
Open
fix(route): add --desc flag + harden e2e gateway-group resolver#39shreemaan-abhishek wants to merge 3 commits into
shreemaan-abhishek wants to merge 3 commits into
Conversation
The README documented `a7 route update <id> --desc "..."` but neither `route create` nor `route update` exposed a --desc flag; the field was only settable via -f. This made the documented invocation fail with 'unknown flag: --desc'. - Add --desc to flag-based create; wire to api.Route.Desc. - Add --desc to flag-based update with DescSet tracking so `--desc ""` explicitly clears the description (rather than being treated as 'unchanged'). - Unit tests cover the request-body wiring for both paths and the clear-via-empty-string case. - E2E TestRoute_DescFlagCRUD round-trips create / update / clear against a real instance. Closes #35.
📝 WalkthroughWalkthroughAdds ChangesRoute description flag support
E2E gateway-group resolution
🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR aligns the a7 route CLI with the README by adding a --desc flag to both route create and route update, including support for explicitly clearing the description on update, and adds unit/E2E coverage to prevent regressions.
Changes:
- Add
--desctoroute create(wired toapi.Route.Desc). - Add
--desctoroute updatewithDescSettracking so--desc ""is treated as an explicit update (clear) rather than “unchanged”. - Add unit tests for create/update request payloads plus an E2E CRUD round-trip test for set/update/clear.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/route_test.go | Adds an E2E regression test covering create/update/clear of desc via flags. |
| pkg/cmd/route/update/update.go | Adds --desc flag handling and DescSet tracking to support clearing. |
| pkg/cmd/route/update/update_test.go | Adds unit tests asserting desc is included/cleared in the PUT payload. |
| pkg/cmd/route/create/create.go | Adds --desc flag and includes Desc in the create request body. |
| pkg/cmd/route/create/create_test.go | Adds a unit test asserting --desc reaches the create request body. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+394
to
+399
| svcID := "e2e-route-desc-svc" | ||
| routeID := "e2e-route-desc" | ||
| t.Cleanup(func() { | ||
| deleteRouteViaAdmin(t, routeID) | ||
| deleteServiceViaAdmin(t, svcID) | ||
| }) |
| var cleared map[string]interface{} | ||
| require.NoError(t, json.Unmarshal([]byte(stdout), &cleared)) | ||
| if d, present := cleared["desc"]; present { | ||
| assert.Equal(t, "", d, "passing --desc \"\" should clear the description") |
![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 `@test/e2e/route_test.go`:
- Around line 425-427: The current check only asserts that cleared["desc"] is an
empty string which can falsely pass if "desc" is simply missing; after
performing the clear update, call the test helper that fetches the persisted
route (e.g., invoke the existing route-get helper or runCommand used elsewhere
in the test) and assert the fetched route's "desc" field is either absent or an
empty string; update the block around the cleared variable (the place
referencing cleared["desc"] and assert.Equal) to, if desc is present assert it's
empty, otherwise perform a follow-up route get and assert the returned object's
desc is empty or not present to ensure the clear persisted.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5141d829-883f-4f32-84fc-26e9e9eb2624
📒 Files selected for processing (5)
pkg/cmd/route/create/create.gopkg/cmd/route/create/create_test.gopkg/cmd/route/update/update.gopkg/cmd/route/update/update_test.gotest/e2e/route_test.go
- Drop the pre-registered name-based cleanup that referenced `routeID` (which is the route's name, not its id). The id-only cleanup registered after parsing `created["id"]` is the only correct one; the pre-reg was a no-op that masked leaks on early failures. - Treat absent, nil, and "" as a cleared desc via a small helper. API7 EE serializes a cleared field as `null`; the previous check produced a present-but-nil key and would have failed on a working CI environment. - Add a follow-up `route get` after the clear-update and assert the same on the persisted server state, not just the update response. Guards against the API echoing an empty desc without persisting it.
The previous resolver picked GET /api/gateway_groups list[0] and ignored both the env var and the group type. That made CI silently dependent on remote-server ordering: when the dev environment's first-returned group became an api7_ingress_controller-typed one, every mutating test failed 13 minutes in with a cryptic 'permission denied' from the CP middleware (`gateway_group.go:24`: ingress groups reject writes for any auth mode other than admin_key). - Treat A7_GATEWAY_GROUP=default as a literal name lookup, not a sentinel; honor any explicit name and fail fast at startup with the list of available names if no match. - Fall back path (empty name) now decodes Type and skips api7_ingress_controller groups instead of taking list[0]. - Startup log now prints the resolved name -> id mapping so the next environment drift is diagnosable from the first log line.
shreemaan-abhishek
changed the title
Comment on lines
+137
to
+144
| // API7 EE uses UUID-style ids for runtime API calls but env vars carry | ||
| // names; resolve name -> id. An explicit name is honored literally; only | ||
| // the empty/"default" case falls back to "first writable group". | ||
| wanted := gatewayGroup | ||
| if wanted == "default" || wanted == "" { | ||
| wanted = "default" | ||
| } | ||
| ggID, err := resolveGatewayGroupID(wanted) |
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 `@test/e2e/setup_test.go`:
- Around line 140-145: The current normalization coerces both "" and "default"
into "default", preventing resolveGatewayGroupID from exercising its wanted ==
"" fallback; change the logic around the gatewayGroup/wanted variable so an
empty gatewayGroup remains an empty string (do not set wanted = "default" when
gatewayGroup == ""), i.e. remove or adjust the branch that normalizes "" into
"default" and pass the original gatewayGroup value into resolveGatewayGroupID
(refer to the local variable wanted and the call to resolveGatewayGroupID).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09ae45b4-f8a9-440c-a5db-4dfce4a67214
📒 Files selected for processing (1)
test/e2e/setup_test.go
Comment on lines
+140
to
+145
| wanted := gatewayGroup | ||
| if wanted == "default" || wanted == "" { | ||
| wanted = "default" | ||
| } | ||
| ggID, err := resolveGatewayGroupID(wanted) | ||
| if err != nil { |
There was a problem hiding this comment.
Keep the default/unset case on the fallback path.
Lines 141-143 currently normalize both "" and "default" to "default", so resolveGatewayGroupID never reaches its wanted == "" fallback branch. That breaks the documented behavior for the unset/default case and can make the mutating E2E tests target a nonexistent or ingress-only group.
Suggested fix
wanted := gatewayGroup
if wanted == "default" || wanted == "" {
- wanted = "default"
+ wanted = ""
}
ggID, err := resolveGatewayGroupID(wanted)🤖 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 `@test/e2e/setup_test.go` around lines 140 - 145, The current normalization
coerces both "" and "default" into "default", preventing resolveGatewayGroupID
from exercising its wanted == "" fallback; change the logic around the
gatewayGroup/wanted variable so an empty gatewayGroup remains an empty string
(do not set wanted = "default" when gatewayGroup == ""), i.e. remove or adjust
the branch that normalizes "" into "default" and pass the original gatewayGroup
value into resolveGatewayGroupID (refer to the local variable wanted and the
call to resolveGatewayGroupID).
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
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.
Summary
Two changes:
1.
route--descflag (closes #35)The README documented
a7 route update <id> --desc "..."but neitherroute createnorroute updateexposed the flag; description was only settable via-f file.pkg/cmd/route/create: add--descflag wired toapi.Route.Desc.pkg/cmd/route/update: add--descwithDescSettracking so--desc ""explicitly clears (rather than being treated as unchanged).TestRoute_DescFlagCRUDround-trips create / update / clear with a follow-uproute getto verify the clear persisted server-side.2. Harden the e2e gateway-group resolver
The previous resolver picked
GET /api/gateway_groupslist[0]and ignored both theA7_GATEWAY_GROUPenv var and the group'stype. CI was silently dependent on remote-server ordering. When the dev environment's first-returned group became anapi7_ingress_controllerone, every mutating test failed 13 minutes in with the CP middleware'spermission denied: Unable to change resources in Ingress gateway group When auth type is not admin_key(CP:internal/pkg/middlewares/gateway_group.go:24, blocks non-admin_key writes to ingress-typed groups).A7_GATEWAY_GROUP=defaultis now a literal name lookup, not a sentinel.A7_GATEWAY_GROUP→ resolver still picks first, but skips ingress-controller groups in the fallback.Verified
go build/go vet(both with and without-tags e2e) clean.go test ./...green locally.TestRoute_DescFlagCRUDagainst API7 EE 3.9.12:--- PASS (0.93s).failed to resolve gateway group: gateway group "does-not-exist" not found; available: [default].CI
Today's CI failure on this PR is unrelated to either change — it's the same dev-env permission issue affecting every PR (PR #38 docs-only sees the same failure). Once the dev-env token / gateway-group is fixed, this PR's CI will go green; meanwhile the resolver hardening is precisely the thing that prevents this class of failure from happening again.
Closes #35. Part of #22.
Summary by CodeRabbit
New Features
Tests