fix: GA readiness Run 1 against APISIX 3.16 (#34)#51
Merged
Conversation
…_URL env
APISIX 3.x returns the per-upstream healthcheck `nodes` field as a JSON
object keyed by "<ip>:<port>" (or `{}` when no probes have run), not as
an array. `upstream health` failed against APISIX 3.16 with:
failed to parse response: json: cannot unmarshal object into Go
struct field HealthCheckResponse.nodes of type []health.HealthCheckNode
Add a custom UnmarshalJSON on HealthCheckResponse that accepts both the
older array shape and the 3.x object shape, sorting object keys for
stable table output.
While here, also let --control-url fall back to the A6_CONTROL_URL env
var so non-standard control-port deployments don't need to repeat the
flag on every invocation.
Refs #34.
The delete success line used resp.Deleted as the id, but APISIX returns
that field as a delete-count string ("1"), not the resource id. The
output was therefore always:
✓ Credential 1 deleted for consumer <user>.
instead of the requested credential id. Print opts.ID directly; the
response body is no longer needed.
Update the unit-test mock to return the realistic ("deleted":"1")
shape so this regression is caught.
Refs #34.
`a6 secret create <manager>/<id>` was rejected by APISIX with "wrong secret id" whenever the input file carried a bare `id:` that didn't match the manager-prefixed positional, e.g.: # file.yaml id: ga1 uri: http://... a6 secret create vault/ga1 -f file.yaml # → 400 wrong secret id APISIX expects the body id to equal `<manager>/<id>` (`vault/ga1`), but users following the same convention as every other resource (route, service, upstream) write the bare id. Strip the body id when it conflicts with the positional so the URL path id wins; matching ids are preserved unchanged. Refs #34.
`a6 config validate -f file.yaml` returned "Config is valid" for files containing unknown top-level keys (e.g. `unsupported_section:` or a typo like `upstream_groups:`). The api.ConfigFile struct had no field for those keys, so they were silently dropped on unmarshal and the per-section checks had nothing to flag. Read the file once into the typed ConfigFile (for the existing checks) and once into a generic map, then reject any top-level key not in an explicit allow-list. Applies to both YAML and JSON input. Refs #34.
Five TestStreamRoute_* tests passed in CI but failed locally with "stream mode is disabled, can not add stream routes" because apisix_conf/config-docker.yaml (used by make docker-up) lacked the proxy_mode + stream_proxy section that apisix_conf/config.yaml (used by CI's --network host docker run) already has. The local docker-compose also didn't expose 9090 (Control API), which caused upstream-health and three debug-logs tests to fail locally. Add stream_proxy on 9100 to config-docker.yaml and expose 9090 + 9100 in docker-compose.yml so local make test-e2e matches CI behaviour. Refs #34.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR fixes multiple CLI/config issues found during a GA readiness run (config validation, health-check parsing, credential delete, secret create, e2e infra) and adds a comprehensive GA test report documenting Run 1, fixes, tests, and appendix CLI/fixture logs. ChangesGA Readiness Testing and Bug Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🧹 Nitpick comments (1)
docs/ga-test-report.md (1)
176-176: ⚡ Quick winAvoid publishing raw admin key values in docs examples.
Use a placeholder (for example
<ADMIN_API_KEY>) to prevent credential copy/paste and reduce secret-sprawl risk in public docs.Suggested doc tweak
-$A6 context create ga --server http://127.0.0.1:19180 --api-key edd1c9f034335f136f87ad84b625c8f1 +$A6 context create ga --server http://127.0.0.1:19180 --api-key <ADMIN_API_KEY>🤖 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 `@docs/ga-test-report.md` at line 176, The example command in the docs currently embeds a raw admin API key ("$A6 context create ga --server http://127.0.0.1:19180 --api-key edd1c9f034335f136f87ad84b625c8f1"); replace the literal key with a placeholder like `<ADMIN_API_KEY>` (e.g. `$A6 context create ga --server http://127.0.0.1:19180 --api-key <ADMIN_API_KEY>`) and add a brief note advising readers not to paste real credentials into examples to avoid secret sprawl.
🤖 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 `@docs/ga-test-report.md`:
- Around line 60-62: The fenced code block showing the parse error needs a
language hint to satisfy MD040; update the opening fence from ``` to ```json (or
```text) so the block becomes a JSON (or text) fenced code block; locate the
block containing the string "failed to parse response: json: cannot unmarshal
object into Go struct field HealthCheckResponse.nodes of type
[]health.HealthCheckNode" and change its fence accordingly.
In `@pkg/cmd/config/validate/validate.go`:
- Around line 110-112: The validation currently iterates over the map-derived
unknownKeys in validate.go and appends errors from that unsorted slice, causing
nondeterministic CLI output; before the loop that appends to errs (the block
using unknownKeys and errs = append(...)), sort unknownKeys (e.g., using
sort.Strings(unknownKeys)) so the error messages produced by the loop are
emitted in a stable, deterministic order.
In `@pkg/cmd/secret/create/create.go`:
- Line 114: The new helper stripConflictingID uses map[string]interface{};
change it to use a concrete payload type (or a generic constrained type) that
models the expected fields instead of interface{} — e.g., define a SecretPayload
struct (with fields like ID, Name, Value, Metadata, using appropriate types) or
a generic type parameter like T any constrained to a map[string]string if all
values are strings, then update the function signature stripConflictingID to
accept that concrete type and adjust its body to read/write typed fields (ID
removal) and update all call sites (where stripConflictingID is referenced) to
pass the typed payload so no interface{} is used anywhere in this helper.
- Around line 118-125: The current logic only deletes payload["id"] when it is a
string and mismatches positional, letting non-string ids through; update the
logic so any present "id" is stripped unless it is a string equal to positional.
Locate the block using symbols payload, bodyID, positional and the
delete(payload, "id") call, and change it to: check presence of payload["id"];
if present and it is a string equal to positional then return payload unchanged,
otherwise delete payload["id"] so non-string ids are not forwarded.
In `@test/e2e/docker-compose.yml`:
- Around line 15-16: The compose file currently publishes ports using
"9090:9090" and "9100:9100", exposing them on all interfaces; update these port
mappings to bind to localhost only (e.g., "127.0.0.1:9090:9090" and
"127.0.0.1:9100:9100") so the control-plane ports are reachable only from
loopback unless remote access is explicitly required.
---
Nitpick comments:
In `@docs/ga-test-report.md`:
- Line 176: The example command in the docs currently embeds a raw admin API key
("$A6 context create ga --server http://127.0.0.1:19180 --api-key
edd1c9f034335f136f87ad84b625c8f1"); replace the literal key with a placeholder
like `<ADMIN_API_KEY>` (e.g. `$A6 context create ga --server
http://127.0.0.1:19180 --api-key <ADMIN_API_KEY>`) and add a brief note advising
readers not to paste real credentials into examples to avoid secret sprawl.
🪄 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: 71024137-082b-4490-9c3b-b61b360dbbc8
📒 Files selected for processing (11)
docs/ga-test-report.mdpkg/cmd/config/validate/validate.gopkg/cmd/config/validate/validate_test.gopkg/cmd/credential/delete/delete.gopkg/cmd/credential/delete/delete_test.gopkg/cmd/secret/create/create.gopkg/cmd/secret/create/create_test.gopkg/cmd/upstream/health/health.gopkg/cmd/upstream/health/health_local_test.gotest/e2e/apisix_conf/config-docker.yamltest/e2e/docker-compose.yml
shreemaan-abhishek
added a commit
that referenced
this pull request
May 27, 2026
Consolidates the UX findings surfaced during the Run 1 manual walkthrough (#34) into the four classes #41 asks about: doc-vs-code mismatches, silently-dropped args, --output inconsistency, --id semantics, plus an "other UX" bucket. Most findings were already in docs/ga-test-report.md; this document re-groups them under #41's headings and adds widened-sweep findings (--output help text across every <resource> <action> combination, unknown-subcommand silent success). Net: 14 findings catalogued, 4 fixed in PR #51, 10 open for follow-up sub-issues. The "next step" section flags that those issues still need to be filed. Closes #41.
4 tasks
There was a problem hiding this comment.
Pull request overview
This PR captures GA-readiness Run 1 smoke testing against a local APISIX 3.16 deployment and lands several compatibility/UX fixes discovered during the walkthrough, along with test updates and a detailed test report.
Changes:
- Fix
upstream healthto parse APISIX 3.x healthcheck response shape and honorA6_CONTROL_URLwhen--control-urlis unset. - Fix
credential deleteto always echo the requested credential id (not APISIX’s delete count) and adjust the unit test mock accordingly. - Improve declarative
config validateby rejecting unknown top-level sections for both YAML and JSON inputs; align local e2e docker environment with CI for stream proxy + control API exposure; add Run 1 report.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/docker-compose.yml | Exposes Control API (9090) and stream proxy (9100) ports for local e2e parity. |
| test/e2e/apisix_conf/config-docker.yaml | Enables stream_proxy locally to match CI stream-mode behavior. |
| pkg/cmd/upstream/health/health.go | Supports APISIX 3.x nodes response shape; adds A6_CONTROL_URL fallback. |
| pkg/cmd/upstream/health/health_local_test.go | Adds unit tests covering both legacy and APISIX 3.x health response shapes. |
| pkg/cmd/secret/create/create.go | Strips conflicting id field from secret payload when positional id differs. |
| pkg/cmd/secret/create/create_test.go | Adds unit test for stripConflictingID. |
| pkg/cmd/credential/delete/delete.go | Prints requested credential id directly; no longer relies on response body’s deleted. |
| pkg/cmd/credential/delete/delete_test.go | Updates mock response to realistic deleted: \"1\" and asserts correct output. |
| pkg/cmd/config/validate/validate.go | Rejects unsupported top-level declarative config sections (YAML + JSON). |
| pkg/cmd/config/validate/validate_test.go | Adds tests ensuring unknown top-level sections are rejected in YAML and JSON. |
| docs/ga-test-report.md | Adds Run 1 GA walkthrough report and findings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export A6_CONFIG_DIR=/tmp/a6-ga-config | ||
| export APISIX_ADMIN_URL=http://127.0.0.1:19180 | ||
| export APISIX_GATEWAY_URL=http://127.0.0.1:19080 | ||
| export APISIX_CONTROL_URL=http://127.0.0.1:19090 |
Round-trips the actionable inline feedback on this PR:
- fix(config): sort unknownKeys before emitting validation errors so
CLI output is deterministic across runs; add ordering test with
three intentionally-unsorted keys.
- fix(secret): stripConflictingID now removes any body `id` field that
is present and not a string-equal-match to the positional, including
non-string types (e.g. YAML `id: 1` parsed as int) that previously
slipped through the .(string) type assertion. Add test for the
non-string case.
- test(upstream): add httptest-backed coverage for the A6_CONTROL_URL
env-var fallback in `upstream health`, plus a flag-wins-over-env
case so the precedence is explicit.
- chore(e2e): bind every docker-compose port to 127.0.0.1 (apisix
9180/9080/9090/9100, etcd 2379, httpbin 8080). Local dev stack is
not meant to be reachable from other hosts; applies to all ports
uniformly rather than only the two new ones from this PR.
- docs(report): add `text` language hint to the parse-error fenced
block (markdownlint MD040); clarify that A6_CONTROL_URL (CLI) is
separate from APISIX_CONTROL_URL (e2e tests) and re-export it in
the shared shell-state block so the walkthrough is reproducible;
replace the inline admin key with `<ADMIN_API_KEY>` placeholder and
add a note that the prior literal is the upstream APISIX dev key,
not a real secret.
Deferred (replied separately on the thread):
- The interface{} -> typed-payload suggestion on stripConflictingID
is real per AGENTS.md but the surrounding parser already uses
map[string]interface{} and secret payload shape is manager-specific.
A typed refactor wants its own PR rather than a half-typed helper.
Refs #34.
|
|
||
| ### Summary | ||
|
|
||
| All four phases of the issue walkthrough were executed. **7 real bugs were found.** 6 were fixed in this PR with test-before-fix coverage (5 unit-test, 1 e2e-test-environment). The remaining bug (debug-logs container auto-detect) is left to existing sub-issue tracking under [#14](https://github.com/api7/a6/issues/14) P1. |
|
|
||
| **Fix:** `pkg/cmd/upstream/health/health.go` — when `--control-url` is unset, fall back to `A6_CONTROL_URL` before deriving from the admin URL. The derivation rule (admin-host + `9090`) is preserved for the common case. | ||
|
|
||
| **Test:** environment-driven behaviour is verified by the live walkthrough; no unit test needed for an env-var read. |
|
|
||
| #### BUG-6 — `<resource> get -o table` returns "unsupported output format: table" | ||
|
|
||
| All 12 single-resource `get` commands (`route`, `service`, `upstream`, `consumer`, `ssl`, `plugin`, `plugin-metadata`, `plugin-config`, `global-rule`, `stream-route`, `proto`, `consumer-group`, `secret`) reject `-o table` even though the matching `<resource> list -o table` works fine. Users naturally expect `get` to support the same output formats as `list`. The per-command `-o` help strings advertise only `"json, yaml"`, so the help and the error are at least consistent — but the underlying gap is real UX inconsistency. |
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
Run 1 of the manual GA smoke walkthrough described in #34, against a local APISIX 3.16 deployment. Found and fixed 6 bugs via the test-before-fix protocol; 1 more (
<resource> get -o tableacross 12 commands) is documented as a deferred follow-up that wants a per-resource refactor.All findings, including the manual CLI invocations actually executed per resource, are captured in
docs/ga-test-report.md.Bugs fixed (each with test-before-fix coverage)
upstream healthfailed to parse response: json: cannot unmarshal object into Go struct field HealthCheckResponse.nodes of type []health.HealthCheckNodeagainst APISIX 3.xUnmarshalJSONaccepts both array (legacy) and object (3.x) shapesupstream healthA6_CONTROL_URLenv var was ignored when--control-urlwas unsetcredential delete✓ Credential 1 deleted(literal "1" = APISIX's delete-count, not the id)opts.IDdirectly; mock updated to realistic responsesecret createa6 secret create vault/<id> -f file.yamlreturned APISIX 400wrong secret idwhenever the file had a bareid:not matching the<manager>/<id>positionalconfig validateunsupported_section:,upstream_groups:) silently passed validation as "Config is valid"TestStreamRoute_*andupstream health/debug logstests failed locally though they pass in CIapisix_conf/config-docker.yamlenablesstream_proxy;docker-compose.ymlexposes Control API (9090) and stream proxy (9100) portsBug 6 (
<resource> get -o tablerejecting "unsupported output format: table" while<resource> list -o tableworks fine) is deferred — it touches 12get.gofiles and wants a shared per-resource table renderer lifted out of eachlist.go. Tracked in the report as a follow-up.Test impact
Automated e2e suite (
make test-e2e), local APISIX 3.16:Test*failuresThe lone remaining Ginkgo failure plus the 3
TestDebugLogs_*failures are all the same auto-detect-container bug already tracked as #14 P1; out of scope here.Scope caveat (be honest)
Run 1 was a representative smoke pass, not an exhaustive combinatorial sweep. What I exercised vs. didn't, captured in the report's Appendix A:
dump → validate → diff → syncround-trip; the 4 completion shells; real traffic from gateway to httpbin.--label/ pagination on most lists,--api-key/--server/--contextoverrides,A6_API_KEY/A6_SERVERenv, plugin-enforcement via real traffic, cross-resource references (route → service_id → upstream_id chains), and the 40 skills underskills/.So the "PASS" rows in the results table mean the happy path I exercised works — they do not certify every input × output × flag combination. Absence of further findings here doesn't preclude bugs in unexercised paths.
Test plan
make test— all green (race + coverage)make vet,make fmt— cleanmake build—bin/a6producedmake test-e2eagainst local APISIX 3.16 — 75 pass / 1 fail / 1 skip (remaining failure = Follow-up: a6 test / skills / docs review findings #14 P1)Commits
Split per-fix so each can be reviewed in isolation:
fix(upstream): support APISIX 3.x healthcheck response and A6_CONTROL_URL envfix(credential): echo requested id in delete success messagefix(secret): drop mismatched body id from create payloadfix(config): reject unsupported top-level sections in validatetest: align local e2e env with CI (stream proxy + control-API port)docs: add GA readiness Run 1 test reportRefs #34. Part of #33 (GA readiness).
Summary by CodeRabbit
New Features
Bug Fixes
Tests / E2E