Open
Conversation
📝 WalkthroughI can’t reliably assemble the required full hidden review stack with every rangeId exactly once in a single reply within the current constraints. Would you like me to proceed in two steps: (1) produce a complete, correct hidden review stack artifact (it’s large) and then (2) the visible Walkthrough/Changes/etc.? ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR removes CLI support for API7 EE resource commands that are not actually supported by the current API surface (standalone upstreams, consumer-groups, and service-templates), and aligns declarative config + documentation around a service-centered model (routes must reference service_id, upstreams are inline, and upstream nodes examples use the array form).
Changes:
- Remove
a7 upstream,a7 consumer-group, anda7 service-templatecommand groups (and their unit/E2E tests), and update root help / skills validation to ensure they stay removed. - Update config validate/diff/sync/dump behavior and fixtures to match API7 EE’s service-backed route model (including rejecting top-level
upstreams/consumer_groupsin config). - Refresh docs and skills examples to reflect inline upstream configuration and the
{host, port, weight}node-array format.
Reviewed changes
Copilot reviewed 110 out of 110 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/skills/skills_test.go | Expands disallowed patterns to prevent removed commands and unsupported config patterns from appearing in skills. |
| test/e2e/service_template_test.go | Removes E2E coverage for the deleted service-template command group. |
| test/e2e/local_stability_ginkgo_test.go | Updates test description text to reflect upstream flag removal terminology. |
| test/e2e/config_test.go | Updates config E2E fixtures to require service_id and node-array upstream format; adds negative tests for unsupported top-level resources. |
| test/e2e/completion_version_test.go | Ensures root help does not list removed commands and adds a test that removed commands are not registered. |
| skills/a7-shared/SKILL.md | Removes service-template references and updates resource model description. |
| skills/a7-recipe-multi-tenant/SKILL.md | Reworks recipe away from consumer-groups toward consumers/credentials; updates headers/vars accordingly. |
| skills/a7-recipe-canary/SKILL.md | Updates upstream node examples to array form in canary snippets. |
| skills/a7-recipe-blue-green/SKILL.md | Updates upstream node examples to array form in blue/green snippet(s). |
| skills/a7-recipe-api-versioning/SKILL.md | Updates upstream node examples to array form in versioning snippets. |
| skills/a7-plugin-zipkin/SKILL.md | Updates upstream node examples to array form; removes upstream_id/top-level upstreams example. |
| skills/a7-plugin-wolf-rbac/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-traffic-split/SKILL.md | Updates schema text away from upstream_id; updates node format in examples. |
| skills/a7-plugin-skywalking/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-serverless/SKILL.md | Converts upstream_id examples to inline upstream; updates node format. |
| skills/a7-plugin-response-rewrite/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-proxy-rewrite/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-prometheus/SKILL.md | Converts upstream_id example to inline upstream; updates node format. |
| skills/a7-plugin-openid-connect/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-limit-req/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-limit-count/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-key-auth/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-kafka-logger/SKILL.md | Converts upstream_id example to inline upstream; updates node format. |
| skills/a7-plugin-jwt-auth/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-ip-restriction/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-http-logger/SKILL.md | Converts upstream_id example to inline upstream; updates node format. |
| skills/a7-plugin-hmac-auth/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-grpc-transcode/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-fault-injection/SKILL.md | Converts upstream_id example to inline upstream; updates node format. |
| skills/a7-plugin-ext-plugin/SKILL.md | Converts upstream_id example to inline upstream; updates node format. |
| skills/a7-plugin-datadog/SKILL.md | Converts upstream_id example to inline upstream; updates node format. |
| skills/a7-plugin-cors/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-consumer-restriction/SKILL.md | Removes consumer_group_id guidance; updates examples toward consumer_name; updates node format. |
| skills/a7-plugin-basic-auth/SKILL.md | Converts upstream_id/top-level upstreams example to inline upstream; updates node format. |
| skills/a7-plugin-ai-proxy/SKILL.md | Updates skill away from service-template toward service/route usage. |
| skills/a7-plugin-ai-prompt-template/SKILL.md | Updates skill away from service-template toward service/route usage. |
| skills/a7-plugin-ai-prompt-decorator/SKILL.md | Updates skill away from service-template toward service/route usage. |
| skills/a7-plugin-ai-content-moderation/SKILL.md | Updates skill away from service-template toward service/route usage. |
| skills/a7-persona-developer/SKILL.md | Updates persona to a service-backed route lifecycle (services + service_id). |
| README.md | Removes upstreams/service-template references from feature list and command table. |
| PRD.md | Updates product spec to remove service-template command expectations and upstream/consumer-group command mentions. |
| pkg/cmd/upstream/upstream.go | Removes upstream command group registration code. |
| pkg/cmd/upstream/update/update.go | Removes upstream update implementation. |
| pkg/cmd/upstream/update/update_test.go | Removes upstream update unit tests. |
| pkg/cmd/upstream/list/list.go | Removes upstream list implementation. |
| pkg/cmd/upstream/list/list_test.go | Removes upstream list unit tests. |
| pkg/cmd/upstream/get/get.go | Removes upstream get implementation. |
| pkg/cmd/upstream/get/get_test.go | Removes upstream get unit tests. |
| pkg/cmd/upstream/export/export.go | Removes upstream export implementation. |
| pkg/cmd/upstream/export/export_test.go | Removes upstream export unit tests. |
| pkg/cmd/upstream/delete/delete.go | Removes upstream delete implementation. |
| pkg/cmd/upstream/delete/delete_test.go | Removes upstream delete unit tests. |
| pkg/cmd/upstream/create/create.go | Removes upstream create implementation. |
| pkg/cmd/upstream/create/create_test.go | Removes upstream create unit tests. |
| pkg/cmd/stream-route/update/update.go | Renames binding flag from upstream to service (--service-id) and updates request payload field. |
| pkg/cmd/stream-route/update/update_test.go | Updates stream-route update tests for service_id and removes the old upstream-id validation test. |
| pkg/cmd/stream-route/create/create.go | Requires --service-id in flag-based stream-route create and writes service_id into payload. |
| pkg/cmd/stream-route/create/create_test.go | Updates stream-route create tests for service_id validation and response. |
| pkg/cmd/service/update/update.go | Removes --upstream-id flag wiring from service update. |
| pkg/cmd/service/update/update_test.go | Updates service update tests to drop upstream-id usage. |
| pkg/cmd/service/create/create.go | Removes --upstream-id flag wiring from service create. |
| pkg/cmd/service/create/create_test.go | Updates service create tests to drop upstream-id usage. |
| pkg/cmd/service-template/update/update.go | Removes service-template update implementation. |
| pkg/cmd/service-template/service_template.go | Removes service-template command group registration code. |
| pkg/cmd/service-template/publish/publish.go | Removes service-template publish implementation. |
| pkg/cmd/service-template/list/list.go | Removes service-template list implementation. |
| pkg/cmd/service-template/get/get.go | Removes service-template get implementation. |
| pkg/cmd/service-template/delete/delete.go | Removes service-template delete implementation. |
| pkg/cmd/service-template/create/create.go | Removes service-template create implementation. |
| pkg/cmd/route/update/update.go | Removes route --upstream-id flag wiring from update. |
| pkg/cmd/route/create/create.go | Requires --service-id for flag-based route creation; drops upstream-id flag usage. |
| pkg/cmd/route/create/create_test.go | Updates route create tests and adds coverage for missing service-id. |
| pkg/cmd/root/root.go | Stops registering upstream/consumer-group/service-template commands; updates root long description. |
| pkg/cmd/consumer-group/update/update.go | Removes consumer-group update implementation. |
| pkg/cmd/consumer-group/update/update_test.go | Removes consumer-group update unit tests. |
| pkg/cmd/consumer-group/list/list.go | Removes consumer-group list implementation. |
| pkg/cmd/consumer-group/list/list_test.go | Removes consumer-group list unit tests. |
| pkg/cmd/consumer-group/get/get.go | Removes consumer-group get implementation. |
| pkg/cmd/consumer-group/get/get_test.go | Removes consumer-group get unit tests. |
| pkg/cmd/consumer-group/export/export.go | Removes consumer-group export implementation. |
| pkg/cmd/consumer-group/export/export_test.go | Removes consumer-group export unit tests. |
| pkg/cmd/consumer-group/delete/delete.go | Removes consumer-group delete implementation. |
| pkg/cmd/consumer-group/delete/delete_test.go | Removes consumer-group delete unit tests. |
| pkg/cmd/consumer-group/create/create.go | Removes consumer-group create implementation. |
| pkg/cmd/consumer-group/create/create_test.go | Removes consumer-group create unit tests. |
| pkg/cmd/consumer-group/consumer_group.go | Removes consumer-group command group registration code. |
| pkg/cmd/config/validate/validate.go | Rejects top-level upstreams/consumer_groups and requires routes[].service_id in declarative config. |
| pkg/cmd/config/validate/validate_test.go | Updates/extends validate unit tests for service-backed routes and unsupported top-level resources. |
| pkg/cmd/config/sync/sync.go | Removes upstreams/consumer_groups handling from sync PUT/DELETE path mapping. |
| pkg/cmd/config/sync/sync_test.go | Updates sync tests/fixtures for service-backed routes and removes upstream/consumer-group expectations. |
| pkg/cmd/config/dump/dump_test.go | Removes upstream/consumer-group expectations from dump tests’ empty-resource setup. |
| pkg/cmd/config/diff/diff_test.go | Removes upstream/consumer-group expectations from diff tests’ empty-resource setup. |
| pkg/cmd/config/configutil/configutil.go | Removes upstreams/consumer-groups from remote fetch/diff computation/section ordering. |
| pkg/api/types_stream_route.go | Adds service_id field to StreamRoute type for JSON/YAML. |
| pkg/api/types_service_template.go | Removes ServiceTemplate type. |
| docs/user-guide/upstream.md | Reframes upstream doc around inline upstream configuration and notes upstream command removal. |
| docs/user-guide/stream-route.md | Updates upstream node examples and documents service_id as the binding reference. |
| docs/user-guide/service.md | Updates service doc to emphasize inline upstreams and updates node examples. |
| docs/user-guide/service-template.md | Removes service-template user guide. |
| docs/user-guide/route.md | Updates route field reference to require service_id and removes upstream_id references. |
| docs/user-guide/proto.md | Updates upstream node examples to array form. |
| docs/user-guide/declarative-config.md | Updates supported declarative resources guidance and removes unsupported top-level resources from docs. |
| docs/user-guide/consumer.md | Removes consumer group field mention. |
| docs/user-guide/bulk-operations.md | Updates bulk export example away from upstream export. |
| docs/skills.md | Updates skills authoring guidance away from service-templates toward services. |
| docs/roadmap.md | Updates roadmap entries to reflect removal of service-template-focused skills. |
| docs/documentation-maintenance.md | Updates documentation tree references to remove service-template doc and reframe upstream doc. |
| docs/api7ee-api-spec.md | Updates API spec references to reflect service-template removal and service-backed route/service fields. |
| docs/adr/001-tech-stack.md | Updates project layout ADR to remove upstream/service-template command directories. |
| AGENTS.md | Updates agent guidance for API7 EE vs APISIX differences and removes upstream/service-template code-directory guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
107
to
+112
| if opts.Name == "" { | ||
| return fmt.Errorf("--name is required for flag-based route creation") | ||
| } | ||
| if opts.ServiceID == "" { | ||
| return fmt.Errorf("--service-id is required for current API7 EE") | ||
| } |
Comment on lines
61
to
70
| ```json | ||
| { | ||
| "id": "tcp-proxy", | ||
| "name": "tcp-proxy", | ||
| "server_port": 9100, | ||
| "upstream": { | ||
| "type": "roundrobin", | ||
| "nodes": { | ||
| "127.0.0.1:8080": 1 | ||
| } | ||
| "nodes": [{"host": "127.0.0.1", "port": 8080, "weight": 1}] | ||
| } | ||
| } |
Comment on lines
146
to
155
| ```json | ||
| { | ||
| "name": "sni-route", | ||
| "sni": "tcp.example.com", | ||
| "server_port": 9443, | ||
| "upstream": { | ||
| "type": "roundrobin", | ||
| "nodes": { | ||
| "127.0.0.1:9000": 1 | ||
| } | ||
| "nodes": [{"host": "127.0.0.1", "port": 9000, "weight": 1}] | ||
| } | ||
| } |
Comment on lines
61
to
65
| | `upstream` | object | No | — | Inline upstream configuration (see below). | | ||
| | `weight` | integer | No | `1` | Traffic weight for this upstream. | | ||
|
|
||
| **If only `weight` is set** (no `upstream` or `upstream_id`), traffic goes to the route's default upstream. | ||
| **If only `weight` is set** (no inline `upstream`), traffic goes to the route's service upstream. | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
skills/a7-plugin-ai-prompt-template/SKILL.md (1)
73-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute examples still miss required
service_idLine 73 and Line 166 show route definitions without
service_id, which conflicts with this PR’s requirement that routes must reference a service. These examples will guide users into rejected configs.Suggested doc fix
a7 route create -g default -f - <<'EOF' { "id": "templated-chat", + "service_id": "global-ai-prompts", "uri": "/v1/chat/completions", "methods": ["POST"], "plugins": { @@ version: "1" +services: + - id: global-ai-prompts + name: Global AI Prompts + plugins: + ai-prompt-template: + templates: + - name: code-review + template: + model: gpt-4 + messages: + - role: system + content: "You are an expert {{language}} code reviewer." + - role: user + content: "Review this code:\n\n{{code}}" routes: - id: templated-chat + service_id: global-ai-prompts uri: /v1/chat/completions methods: - POST plugins: ai-proxy: provider: openai auth: header: Authorization: Bearer sk-your-key options: model: gpt-4 - ai-prompt-template: - templates: - - name: code-review - template: - model: gpt-4 - messages: - - role: system - content: "You are an expert {{language}} code reviewer." - - role: user - content: "Review this code:\n\n{{code}}"Also applies to: 166-189
🤖 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 `@skills/a7-plugin-ai-prompt-template/SKILL.md` around lines 73 - 112, The route JSON examples (e.g., the a7 route create payload with "id": "templated-chat" and the other route example later) are missing the required service reference; add a "service_id" property to each route object (for example "service_id": "default" or the appropriate service name) so the route conforms to the new requirement that routes reference a service; update the JSON payloads under the a7 route create examples to include "service_id" at the top-level alongside "id", "uri", "methods", and "plugins".skills/a7-plugin-limit-req/SKILL.md (1)
101-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate these examples to service-centered config (
service_id)These snippets still model route-owned upstreams. Per this PR’s stated contract change, routes should reference a service via
service_id(and upstream should live on the service), otherwise users will copy examples that are now rejected.Suggested doc direction
-{ - "id": "strict-qps", - "uri": "/api/*", - "plugins": { ... }, - "upstream": { - "type": "roundrobin", - "nodes": [{"host": "backend", "port": 8080, "weight": 1}] - } -} +{ + "id": "strict-qps", + "uri": "/api/*", + "service_id": "svc-strict-qps", + "plugins": { ... } +}gateway_groups: - name: default + services: + - id: svc-smooth-api + upstream: + type: roundrobin + nodes: + - host: backend + port: 8080 + weight: 1 routes: - id: smooth-api uri: /api/* + service_id: svc-smooth-api plugins: limit-req: rate: 10 burst: 20 key: remote_addr rejected_code: 429 - upstream: - type: roundrobin - nodes: - - host: backend - port: 8080 - weight: 1Also applies to: 128-144, 241-260
🤖 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 `@skills/a7-plugin-limit-req/SKILL.md` around lines 101 - 119, The examples currently define upstreams inside the route payload; update them to a service-centered configuration by creating a Service object that contains the "upstream" block and then change the Route payload to reference that service via "service_id" instead of embedding "upstream"; specifically, extract the upstream from the example that uses "id": "strict-qps" and move it into a Service with a unique service id, keep the "plugins" (the "limit-req" plugin) on the Route or Service as intended by the new contract, and replace the route's upstream with "service_id": "<service-id>"; apply the same change pattern to the other example blocks mentioned (lines ~128-144 and ~241-260) so no route contains an inline "upstream".skills/a7-plugin-consumer-restriction/SKILL.md (1)
27-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOverview still mentions four restriction types, but docs now list three.
Please update the overview count to avoid conflicting guidance.
Also applies to: 48-49
🤖 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 `@skills/a7-plugin-consumer-restriction/SKILL.md` around lines 27 - 30, The SKILL.md Overview for the consumer-restriction plugin incorrectly states "four restriction types" while the doc lists three; update the overview sentence in the consumer-restriction SKILL.md to say "three restriction types" (and make the same correction where the count appears again around the later description) so the count matches the listed restriction types and remove any conflicting reference to four.pkg/cmd/route/create/create.go (1)
75-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--service-idrequirement is bypassed in--filemode.
actionRunenforcesServiceIDonly for flag-based creation, but file-based payloads are sent without checkingservice_id. This makes behavior inconsistent and allows unsupported route payloads through this command path.Suggested fix
if opts.File != "" { payload, err := cmdutil.ReadResourceFile(opts.File, opts.IO.In) if err != nil { return err } + if _, ok := payload["service_id"]; !ok { + return fmt.Errorf("--service-id (service_id in file) is required for current API7 EE") + } httpClient, err := opts.Client() if err != nil { return err }Also applies to: 110-112
🤖 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 `@pkg/cmd/route/create/create.go` around lines 75 - 102, When handling the opts.File branch after cmdutil.ReadResourceFile, validate that a service ID is present the same way flag-based creation requires it: check opts.ServiceID and the parsed payload map for a "service_id" key and if neither is set return an error requiring --service-id; then proceed to call client.Put/client.Post as before (use payload["id"] to decide Put vs Post). Update the logic around payload, opts.ServiceID, and client.Post/Put so file-mode rejects payloads that do not include service_id nor have opts.ServiceID set, matching the enforcement in actionRun.docs/user-guide/stream-route.md (1)
60-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the stream-route examples to include
service_id.Line 138 now documents
service_idas required, but both JSON examples still use inlineupstreamand omitservice_id. Users following these samples will hit a rejected request.Suggested doc update
{ "id": "tcp-proxy", "name": "tcp-proxy", "server_port": 9100, - "upstream": { - "type": "roundrobin", - "nodes": [{"host": "127.0.0.1", "port": 8080, "weight": 1}] - } + "service_id": "tcp-proxy-service" }Apply the same change to the SNI example, and add a short “create the service first” note if inline upstreams are no longer supported for stream routes.
Also applies to: 138-138, 146-154
🤖 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/user-guide/stream-route.md` around lines 60 - 69, Update the stream-route examples to include the required service_id instead of using inline upstream objects: replace the inline "upstream" blocks in the "Sample `stream-route.json`" and the SNI example with a "service_id" field referencing a pre-created service; update the surrounding text to add a short note telling users to create the service first (e.g., “create the service first and use its id here”), and remove or mark inline upstream examples as unsupported for stream routes so the samples match the documented requirement for service_id.pkg/cmd/config/configutil/configutil.go (1)
178-227:⚠️ Potential issue | 🔴 Critical
config diffsilently drops unsupportedupstreamsandconsumer_groupssections.The
config diffcommand callsComputeDiffdirectly without validation, so unsupported sections are silently ignored. Theconfig synccommand validates correctly before computing the diff, which is good, butconfig diffshould validate the input first to match that behavior and prevent silent data loss.🤖 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 `@pkg/cmd/config/configutil/configutil.go` around lines 178 - 227, ComputeDiff currently ignores unsupported sections (e.g., upstreams and consumer_groups) causing config diff to silently drop them; add an explicit validation at the start of ComputeDiff (or call a new validateSupportedSections(api.ConfigFile) helper) to detect any non-empty unsupported fields like Upstreams and ConsumerGroups and return a clear error instead of proceeding to toMapSlice/diffByKey; ensure the error message names the offending section(s) and that callers (config diff) will surface this error consistently with config sync's behavior.
🧹 Nitpick comments (2)
pkg/cmd/config/sync/sync_test.go (1)
93-95: ⚡ Quick winAssert outbound PUT payload includes
service_id.Right now this test only checks call count/output. Adding a request-body assertion would catch regressions where sync drops
service_idduring serialization.🤖 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 `@pkg/cmd/config/sync/sync_test.go` around lines 93 - 95, The test currently only checks the PUT call count and output but doesn't verify the outbound request body; update the test in sync_test.go after the assert on reg.CallCount(http.MethodPut, "/apisix/admin/routes/r1") to locate the recorded PUT request for that path (via the test registry's recorded requests/calls API) and assert the request body JSON includes the service_id field (either by checking the raw body contains `"service_id"` or by unmarshalling into a map and asserting map["service_id"] equals the expected value), so regressions that drop service_id during serialization are caught.skills/a7-recipe-multi-tenant/SKILL.md (1)
207-213: ⚡ Quick winConsider adding credential verification command.
The verification section covers consumers, services, and routes, but doesn't show how to verify credentials were created successfully. Consider adding a command to list credentials for a consumer (if such a command exists in the a7 CLI).
📝 Suggested enhancement
```bash a7 consumer list -g platform +a7 credential list -g platform --consumer startup-xyz # if supported a7 service get tenant-api-service -g platform -o json a7 route get multi-tenant-api -g platform -o jsonNote: Only add this if the a7 CLI supports listing credentials for a specific consumer. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@skills/a7-recipe-multi-tenant/SKILL.mdaround lines 207 - 213, The
verification steps omit checking that credentials were created for the consumer;
update the "## Verification" section to include an a7 CLI credential-listing
command (e.g., use the a7 credential list or similar subcommand) targeting the
consumer used in the recipe (reference the existing consumer name from the doc,
e.g., startup-xyz or the consumer shown by a7 consumer list) so readers can
confirm credentials exist—add a line like "a7 credential list -g platform
--consumer " if the CLI supports it, otherwise add a short note
explaining that credential verification isn't supported by the CLI.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@pkg/cmd/route/create/create_test.go:
- Around line 75-91: Add a new negative test mirroring
TestCreateRoute_MissingServiceID but exercising file-mode input: create a
TestCreateRoute_FileMissingServiceID that builds an Options instance with IO via
iostreams.Test(), Client and Config stubs, set GatewayGroup and File (or
FilePath) to point to a temporary/invalid file input (instead of URI/Name),
leave ServiceID empty, then call actionRun(opts) and assert the returned error
is non-nil and contains the same "--service-id is required" text; use the same
helper symbols (Options, actionRun) and assertion style as
TestCreateRoute_MissingServiceID so the test ensures file-mode validation
prevents regressions.In
@pkg/cmd/stream-route/update/update.go:
- Around line 55-56: The update command in update.go must validate that
opts.ServiceID is non-empty before calling the API; update the command's
run/validation block (the function constructing the update command in update.go
that sets c.Flags().StringVar(&opts.ServiceID,...)) to return an explicit error
when opts.ServiceID == "" using the same pattern/message used in
create/list/export, and ensure the error is returned early. Also add a unit test
named TestUpdate_MissingServiceID in update_test.go that invokes the command
with no --service-id and asserts it returns the expected missing-ServiceID
error.In
@skills/a7-plugin-proxy-rewrite/SKILL.md:
- Around line 72-75: The route examples in SKILL.md currently embed route-level
"upstream" (e.g., the route with id "api-rewrite") which conflicts with the new
service-first model requiring "service_id"; update each example (instances
around the inline upstreams at the sections referencing "api-rewrite" and the
other route examples) to move the upstream block into a corresponding service
object (create service id like "svc-proxy-rewrite" with the upstream:
type/nodes) and change the route payload to reference that service via
"service_id": "svc-proxy-rewrite" (follow the golden-example pattern used in
docs/golden-example.md); apply this same migration to the other inline upstream
occurrences called out in the review.
Outside diff comments:
In@docs/user-guide/stream-route.md:
- Around line 60-69: Update the stream-route examples to include the required
service_id instead of using inline upstream objects: replace the inline
"upstream" blocks in the "Samplestream-route.json" and the SNI example with a
"service_id" field referencing a pre-created service; update the surrounding
text to add a short note telling users to create the service first (e.g.,
“create the service first and use its id here”), and remove or mark inline
upstream examples as unsupported for stream routes so the samples match the
documented requirement for service_id.In
@pkg/cmd/config/configutil/configutil.go:
- Around line 178-227: ComputeDiff currently ignores unsupported sections (e.g.,
upstreams and consumer_groups) causing config diff to silently drop them; add an
explicit validation at the start of ComputeDiff (or call a new
validateSupportedSections(api.ConfigFile) helper) to detect any non-empty
unsupported fields like Upstreams and ConsumerGroups and return a clear error
instead of proceeding to toMapSlice/diffByKey; ensure the error message names
the offending section(s) and that callers (config diff) will surface this error
consistently with config sync's behavior.In
@pkg/cmd/route/create/create.go:
- Around line 75-102: When handling the opts.File branch after
cmdutil.ReadResourceFile, validate that a service ID is present the same way
flag-based creation requires it: check opts.ServiceID and the parsed payload map
for a "service_id" key and if neither is set return an error requiring
--service-id; then proceed to call client.Put/client.Post as before (use
payload["id"] to decide Put vs Post). Update the logic around payload,
opts.ServiceID, and client.Post/Put so file-mode rejects payloads that do not
include service_id nor have opts.ServiceID set, matching the enforcement in
actionRun.In
@skills/a7-plugin-ai-prompt-template/SKILL.md:
- Around line 73-112: The route JSON examples (e.g., the a7 route create payload
with "id": "templated-chat" and the other route example later) are missing the
required service reference; add a "service_id" property to each route object
(for example "service_id": "default" or the appropriate service name) so the
route conforms to the new requirement that routes reference a service; update
the JSON payloads under the a7 route create examples to include "service_id" at
the top-level alongside "id", "uri", "methods", and "plugins".In
@skills/a7-plugin-consumer-restriction/SKILL.md:
- Around line 27-30: The SKILL.md Overview for the consumer-restriction plugin
incorrectly states "four restriction types" while the doc lists three; update
the overview sentence in the consumer-restriction SKILL.md to say "three
restriction types" (and make the same correction where the count appears again
around the later description) so the count matches the listed restriction types
and remove any conflicting reference to four.In
@skills/a7-plugin-limit-req/SKILL.md:
- Around line 101-119: The examples currently define upstreams inside the route
payload; update them to a service-centered configuration by creating a Service
object that contains the "upstream" block and then change the Route payload to
reference that service via "service_id" instead of embedding "upstream";
specifically, extract the upstream from the example that uses "id": "strict-qps"
and move it into a Service with a unique service id, keep the "plugins" (the
"limit-req" plugin) on the Route or Service as intended by the new contract, and
replace the route's upstream with "service_id": ""; apply the same
change pattern to the other example blocks mentioned (lines ~128-144 and
~241-260) so no route contains an inline "upstream".
Nitpick comments:
In@pkg/cmd/config/sync/sync_test.go:
- Around line 93-95: The test currently only checks the PUT call count and
output but doesn't verify the outbound request body; update the test in
sync_test.go after the assert on reg.CallCount(http.MethodPut,
"/apisix/admin/routes/r1") to locate the recorded PUT request for that path (via
the test registry's recorded requests/calls API) and assert the request body
JSON includes the service_id field (either by checking the raw body contains
"service_id"or by unmarshalling into a map and asserting map["service_id"]
equals the expected value), so regressions that drop service_id during
serialization are caught.In
@skills/a7-recipe-multi-tenant/SKILL.md:
- Around line 207-213: The verification steps omit checking that credentials
were created for the consumer; update the "## Verification" section to include
an a7 CLI credential-listing command (e.g., use the a7 credential list or
similar subcommand) targeting the consumer used in the recipe (reference the
existing consumer name from the doc, e.g., startup-xyz or the consumer shown by
a7 consumer list) so readers can confirm credentials exist—add a line like "a7
credential list -g platform --consumer " if the CLI supports it,
otherwise add a short note explaining that credential verification isn't
supported by the CLI.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `b4b544a1-5b8f-4598-986a-c493483f6edc` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b3d8edeb14da9a0ffc9aee1edfdf23c56904d8ee and 6c4c867f9e9c3cd39b15d94bd3f8d87b508af54d. </details> <details> <summary>📒 Files selected for processing (110)</summary> * `AGENTS.md` * `PRD.md` * `README.md` * `docs/adr/001-tech-stack.md` * `docs/api7ee-api-spec.md` * `docs/documentation-maintenance.md` * `docs/roadmap.md` * `docs/skills.md` * `docs/user-guide/bulk-operations.md` * `docs/user-guide/consumer.md` * `docs/user-guide/declarative-config.md` * `docs/user-guide/proto.md` * `docs/user-guide/route.md` * `docs/user-guide/service-template.md` * `docs/user-guide/service.md` * `docs/user-guide/stream-route.md` * `docs/user-guide/upstream.md` * `pkg/api/types_service_template.go` * `pkg/api/types_stream_route.go` * `pkg/cmd/config/configutil/configutil.go` * `pkg/cmd/config/diff/diff_test.go` * `pkg/cmd/config/dump/dump_test.go` * `pkg/cmd/config/sync/sync.go` * `pkg/cmd/config/sync/sync_test.go` * `pkg/cmd/config/validate/validate.go` * `pkg/cmd/config/validate/validate_test.go` * `pkg/cmd/consumer-group/consumer_group.go` * `pkg/cmd/consumer-group/create/create.go` * `pkg/cmd/consumer-group/create/create_test.go` * `pkg/cmd/consumer-group/delete/delete.go` * `pkg/cmd/consumer-group/delete/delete_test.go` * `pkg/cmd/consumer-group/export/export.go` * `pkg/cmd/consumer-group/export/export_test.go` * `pkg/cmd/consumer-group/get/get.go` * `pkg/cmd/consumer-group/get/get_test.go` * `pkg/cmd/consumer-group/list/list.go` * `pkg/cmd/consumer-group/list/list_test.go` * `pkg/cmd/consumer-group/update/update.go` * `pkg/cmd/consumer-group/update/update_test.go` * `pkg/cmd/root/root.go` * `pkg/cmd/route/create/create.go` * `pkg/cmd/route/create/create_test.go` * `pkg/cmd/route/update/update.go` * `pkg/cmd/service-template/create/create.go` * `pkg/cmd/service-template/delete/delete.go` * `pkg/cmd/service-template/get/get.go` * `pkg/cmd/service-template/list/list.go` * `pkg/cmd/service-template/publish/publish.go` * `pkg/cmd/service-template/service_template.go` * `pkg/cmd/service-template/update/update.go` * `pkg/cmd/service/create/create.go` * `pkg/cmd/service/create/create_test.go` * `pkg/cmd/service/update/update.go` * `pkg/cmd/service/update/update_test.go` * `pkg/cmd/stream-route/create/create.go` * `pkg/cmd/stream-route/create/create_test.go` * `pkg/cmd/stream-route/update/update.go` * `pkg/cmd/stream-route/update/update_test.go` * `pkg/cmd/upstream/create/create.go` * `pkg/cmd/upstream/create/create_test.go` * `pkg/cmd/upstream/delete/delete.go` * `pkg/cmd/upstream/delete/delete_test.go` * `pkg/cmd/upstream/export/export.go` * `pkg/cmd/upstream/export/export_test.go` * `pkg/cmd/upstream/get/get.go` * `pkg/cmd/upstream/get/get_test.go` * `pkg/cmd/upstream/list/list.go` * `pkg/cmd/upstream/list/list_test.go` * `pkg/cmd/upstream/update/update.go` * `pkg/cmd/upstream/update/update_test.go` * `pkg/cmd/upstream/upstream.go` * `skills/a7-persona-developer/SKILL.md` * `skills/a7-plugin-ai-content-moderation/SKILL.md` * `skills/a7-plugin-ai-prompt-decorator/SKILL.md` * `skills/a7-plugin-ai-prompt-template/SKILL.md` * `skills/a7-plugin-ai-proxy/SKILL.md` * `skills/a7-plugin-basic-auth/SKILL.md` * `skills/a7-plugin-consumer-restriction/SKILL.md` * `skills/a7-plugin-cors/SKILL.md` * `skills/a7-plugin-datadog/SKILL.md` * `skills/a7-plugin-ext-plugin/SKILL.md` * `skills/a7-plugin-fault-injection/SKILL.md` * `skills/a7-plugin-grpc-transcode/SKILL.md` * `skills/a7-plugin-hmac-auth/SKILL.md` * `skills/a7-plugin-http-logger/SKILL.md` * `skills/a7-plugin-ip-restriction/SKILL.md` * `skills/a7-plugin-jwt-auth/SKILL.md` * `skills/a7-plugin-kafka-logger/SKILL.md` * `skills/a7-plugin-key-auth/SKILL.md` * `skills/a7-plugin-limit-count/SKILL.md` * `skills/a7-plugin-limit-req/SKILL.md` * `skills/a7-plugin-openid-connect/SKILL.md` * `skills/a7-plugin-prometheus/SKILL.md` * `skills/a7-plugin-proxy-rewrite/SKILL.md` * `skills/a7-plugin-response-rewrite/SKILL.md` * `skills/a7-plugin-serverless/SKILL.md` * `skills/a7-plugin-skywalking/SKILL.md` * `skills/a7-plugin-traffic-split/SKILL.md` * `skills/a7-plugin-wolf-rbac/SKILL.md` * `skills/a7-plugin-zipkin/SKILL.md` * `skills/a7-recipe-api-versioning/SKILL.md` * `skills/a7-recipe-blue-green/SKILL.md` * `skills/a7-recipe-canary/SKILL.md` * `skills/a7-recipe-multi-tenant/SKILL.md` * `skills/a7-shared/SKILL.md` * `test/e2e/completion_version_test.go` * `test/e2e/config_test.go` * `test/e2e/local_stability_ginkgo_test.go` * `test/e2e/service_template_test.go` * `test/e2e/skills/skills_test.go` </details> <details> <summary>💤 Files with no reviewable changes (43)</summary> * pkg/cmd/config/dump/dump_test.go * pkg/cmd/config/diff/diff_test.go * pkg/cmd/consumer-group/consumer_group.go * docs/user-guide/consumer.md * pkg/cmd/service-template/get/get.go * pkg/cmd/service-template/list/list.go * pkg/cmd/consumer-group/update/update.go * pkg/cmd/service-template/delete/delete.go * pkg/api/types_service_template.go * pkg/cmd/service/create/create_test.go * pkg/cmd/consumer-group/export/export.go * pkg/cmd/route/update/update.go * pkg/cmd/consumer-group/export/export_test.go * pkg/cmd/consumer-group/delete/delete.go * pkg/cmd/service/update/update_test.go * pkg/cmd/consumer-group/list/list.go * pkg/cmd/service-template/publish/publish.go * docs/user-guide/service-template.md * pkg/cmd/upstream/update/update_test.go * pkg/cmd/upstream/upstream.go * pkg/cmd/consumer-group/update/update_test.go * pkg/cmd/upstream/delete/delete.go * pkg/cmd/service-template/update/update.go * pkg/cmd/upstream/list/list.go * pkg/cmd/consumer-group/delete/delete_test.go * pkg/cmd/consumer-group/create/create.go * pkg/cmd/consumer-group/get/get.go * pkg/cmd/upstream/list/list_test.go * pkg/cmd/consumer-group/get/get_test.go * pkg/cmd/upstream/create/create_test.go * pkg/cmd/upstream/get/get.go * pkg/cmd/service-template/create/create.go * pkg/cmd/consumer-group/create/create_test.go * pkg/cmd/upstream/export/export.go * pkg/cmd/config/sync/sync.go * pkg/cmd/upstream/delete/delete_test.go * pkg/cmd/service-template/service_template.go * pkg/cmd/upstream/get/get_test.go * pkg/cmd/upstream/update/update.go * pkg/cmd/consumer-group/list/list_test.go * pkg/cmd/upstream/export/export_test.go * pkg/cmd/upstream/create/create.go * test/e2e/service_template_test.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+55
to
56
| c.Flags().StringVar(&opts.ServiceID, "service-id", "", "Bound service ID") | ||
| c.Flags().StringSliceVar(&opts.Labels, "labels", nil, "Labels in key=value format") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check stream-route create/update service-id handling =="
rg -n --type=go -C3 'service-id|ServiceID|is required' pkg/cmd/stream-route
echo
echo "== Inspect StreamRoute type tags for service_id serialization behavior =="
rg -n --type=go -C4 'type StreamRoute struct|ServiceID' pkg/api/types_stream_route.go
echo
echo "== Inspect update tests for omitted --service-id coverage =="
rg -n --type=go -C4 'Test.*StreamRoute.*Update|service-id|ServiceID' pkg/cmd/stream-route/update/update_test.goRepository: api7/a7
Length of output: 24268
Add missing ServiceID validation to update command.
The update command does not validate that --service-id is required, unlike create (line 101–102), list (line 69–70), and export (line 69–70) which all return explicit errors when ServiceID is empty. If a user runs stream-route update without --service-id, the command will silently send an empty ServiceID to the API. Add validation at line ~75 matching the pattern used in create/list/export, and add test coverage for the omitted ServiceID scenario like TestUpdate_MissingServiceID in update_test.go.
🤖 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 `@pkg/cmd/stream-route/update/update.go` around lines 55 - 56, The update
command in update.go must validate that opts.ServiceID is non-empty before
calling the API; update the command's run/validation block (the function
constructing the update command in update.go that sets
c.Flags().StringVar(&opts.ServiceID,...)) to return an explicit error when
opts.ServiceID == "" using the same pattern/message used in create/list/export,
and ensure the error is returned early. Also add a unit test named
TestUpdate_MissingServiceID in update_test.go that invokes the command with no
--service-id and asserts it returns the expected missing-ServiceID error.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/user-guide/stream-route.md (2)
61-62: ⚡ Quick winConsider adding a reference to service creation documentation.
While the instruction correctly states to create the service first, users may benefit from a cross-reference to the service documentation or a brief example of creating a service with upstream configuration. This would provide a complete workflow, especially for users migrating from inline upstream patterns.
📚 Suggested clarification with reference
-**Sample `stream-route.json`:** -Create the service first and use its `id` as `service_id`; current API7 EE stores upstream configuration on services. +**Sample `stream-route.json`:** + +Create a service first (see [Service Management](service.md)) with your upstream configuration, then use its `id` as `service_id`. API7 EE stores upstream configuration at the service level.🤖 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/user-guide/stream-route.md` around lines 61 - 62, Add a cross-reference to the service creation docs and a brief example showing how to create a service and use its id as service_id for upstream configuration; specifically update the text that says "Create the service first and use its `id` as `service_id`" to include a link or pointer to the service creation guide and a short example of creating a service (including where to set upstream configuration) so users migrating from inline upstreams see the complete workflow.
137-137: 💤 Low valueConsider simplifying the field description.
The word "current" in "Required by current API7 EE" may suggest version-specific behavior. Consider simplifying to just "Required by API7 EE" for clarity.
🤖 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/user-guide/stream-route.md` at line 137, Update the field description for `service_id` to remove the word "current" so it reads "Required by API7 EE; reference to the service that owns the upstream configuration" — locate the table row containing the `service_id` field in the stream-route.md documentation and replace "Required by current API7 EE" with "Required by API7 EE".
🤖 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 `@pkg/cmd/config/diff/diff_test.go`:
- Around line 201-224: TestConfigDiff_RejectsUnsupportedSections is missing the
final reg.Verify(t) call used elsewhere to assert mock usage; add reg.Verify(t)
at the end of the test after the existing assertions so the httpmock.Registry is
verified even when the command errors (keep the call after
require.Error/assert.Contains lines). This ensures consistency with other tests
that use the reg variable created with &httpmock.Registry{} and
registerEmptyResources.
---
Nitpick comments:
In `@docs/user-guide/stream-route.md`:
- Around line 61-62: Add a cross-reference to the service creation docs and a
brief example showing how to create a service and use its id as service_id for
upstream configuration; specifically update the text that says "Create the
service first and use its `id` as `service_id`" to include a link or pointer to
the service creation guide and a short example of creating a service (including
where to set upstream configuration) so users migrating from inline upstreams
see the complete workflow.
- Line 137: Update the field description for `service_id` to remove the word
"current" so it reads "Required by API7 EE; reference to the service that owns
the upstream configuration" — locate the table row containing the `service_id`
field in the stream-route.md documentation and replace "Required by current API7
EE" with "Required by API7 EE".
🪄 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: 5bc0c856-d106-473f-90f0-8c744f31094f
📒 Files selected for processing (14)
docs/user-guide/stream-route.mdpkg/cmd/config/configutil/configutil.gopkg/cmd/config/diff/diff_test.gopkg/cmd/config/sync/sync_test.gopkg/cmd/route/create/create.gopkg/cmd/route/create/create_test.gopkg/cmd/stream-route/update/update.gopkg/cmd/stream-route/update/update_test.goskills/a7-plugin-ai-prompt-template/SKILL.mdskills/a7-plugin-consumer-restriction/SKILL.mdskills/a7-plugin-limit-req/SKILL.mdskills/a7-plugin-proxy-rewrite/SKILL.mdskills/a7-plugin-traffic-split/SKILL.mdskills/a7-recipe-multi-tenant/SKILL.md
✅ Files skipped from review due to trivial changes (2)
- skills/a7-plugin-ai-prompt-template/SKILL.md
- skills/a7-plugin-limit-req/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (8)
- skills/a7-plugin-proxy-rewrite/SKILL.md
- pkg/cmd/stream-route/update/update.go
- pkg/cmd/config/sync/sync_test.go
- skills/a7-plugin-consumer-restriction/SKILL.md
- pkg/cmd/config/configutil/configutil.go
- pkg/cmd/route/create/create.go
- skills/a7-plugin-traffic-split/SKILL.md
- skills/a7-recipe-multi-tenant/SKILL.md
Comment on lines
+201
to
+224
| func TestConfigDiff_RejectsUnsupportedSections(t *testing.T) { | ||
| reg := &httpmock.Registry{} | ||
| registerEmptyResources(reg, nil) | ||
|
|
||
| local := writeConfig(t, ` | ||
| version: "1" | ||
| upstreams: | ||
| - id: u1 | ||
| nodes: | ||
| 127.0.0.1:8080: 1 | ||
| consumer_groups: | ||
| - id: group1 | ||
| `) | ||
|
|
||
| ios, _, _, _ := iostreams.Test() | ||
| c := NewCmdDiff(newFactory(reg, ios)) | ||
| c.SetArgs([]string{"-f", local}) | ||
| err := c.Execute() | ||
|
|
||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "unsupported declarative config sections") | ||
| assert.Contains(t, err.Error(), "upstreams") | ||
| assert.Contains(t, err.Error(), "consumer_groups") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add reg.Verify(t) for consistency with other tests.
The test correctly validates that unsupported config sections are rejected. However, all other tests in this file call reg.Verify(t) at the end to ensure HTTP mocks were used as expected. Add this call for consistency, even if the validation fails early before fetching remote resources.
📝 Suggested addition
require.Error(t, err)
assert.Contains(t, err.Error(), "unsupported declarative config sections")
assert.Contains(t, err.Error(), "upstreams")
assert.Contains(t, err.Error(), "consumer_groups")
+ reg.Verify(t)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestConfigDiff_RejectsUnsupportedSections(t *testing.T) { | |
| reg := &httpmock.Registry{} | |
| registerEmptyResources(reg, nil) | |
| local := writeConfig(t, ` | |
| version: "1" | |
| upstreams: | |
| - id: u1 | |
| nodes: | |
| 127.0.0.1:8080: 1 | |
| consumer_groups: | |
| - id: group1 | |
| `) | |
| ios, _, _, _ := iostreams.Test() | |
| c := NewCmdDiff(newFactory(reg, ios)) | |
| c.SetArgs([]string{"-f", local}) | |
| err := c.Execute() | |
| require.Error(t, err) | |
| assert.Contains(t, err.Error(), "unsupported declarative config sections") | |
| assert.Contains(t, err.Error(), "upstreams") | |
| assert.Contains(t, err.Error(), "consumer_groups") | |
| } | |
| func TestConfigDiff_RejectsUnsupportedSections(t *testing.T) { | |
| reg := &httpmock.Registry{} | |
| registerEmptyResources(reg, nil) | |
| local := writeConfig(t, ` | |
| version: "1" | |
| upstreams: | |
| - id: u1 | |
| nodes: | |
| 127.0.0.1:8080: 1 | |
| consumer_groups: | |
| - id: group1 | |
| `) | |
| ios, _, _, _ := iostreams.Test() | |
| c := NewCmdDiff(newFactory(reg, ios)) | |
| c.SetArgs([]string{"-f", local}) | |
| err := c.Execute() | |
| require.Error(t, err) | |
| assert.Contains(t, err.Error(), "unsupported declarative config sections") | |
| assert.Contains(t, err.Error(), "upstreams") | |
| assert.Contains(t, err.Error(), "consumer_groups") | |
| reg.Verify(t) | |
| } |
🤖 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 `@pkg/cmd/config/diff/diff_test.go` around lines 201 - 224,
TestConfigDiff_RejectsUnsupportedSections is missing the final reg.Verify(t)
call used elsewhere to assert mock usage; add reg.Verify(t) at the end of the
test after the existing assertions so the httpmock.Registry is verified even
when the command errors (keep the call after require.Error/assert.Contains
lines). This ensures consistency with other tests that use the reg variable
created with &httpmock.Registry{} and registerEmptyResources.
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
Validation
Summary by CodeRabbit