fix: resolve CLI bugs found during GA validation#31
Open
shreemaan-abhishek wants to merge 6 commits into
Open
Conversation
Flag-based ssl update silently dropped partial updates. Require --cert/--key so updates do not lose fields.
Pass the decoded map to the exporter instead of json.RawMessage so -o yaml no longer renders a byte array.
plugin get -o yaml emitted JSON. Honor any --output value, not just json.
stream-route create had no --name flag. Add the Name field and validation.
config validate silently accepted a top-level service_templates section. Reject it on the validate and diff/sync paths.
📝 WalkthroughWalkthroughThis PR executes a comprehensive GA readiness test plan for API7 EE 3.9.12. The test plan and detailed report document systematic testing across four phases, identify five bugs in stream-route, ssl update, plugin, and config validation commands, and implement fixes with full E2E and unit test coverage. ChangesGA Readiness Testing & Bug Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/e2e/ssl_test.go (1)
176-181: ⚡ Quick winStrengthen the failed-update assertion to prevent false positives.
After the rejected update, you assert old SNI exists, but not that the new SNI is absent. Adding that check makes the regression stricter.
Suggested test hardening
snis := requireJSONArray(t, ssl["snis"], "ssl.snis") assert.Contains(t, snis, "require-cert.example.com") + assert.NotContains(t, snis, "require-cert-updated.example.com")🤖 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/ssl_test.go` around lines 176 - 181, The test currently checks that the original SNI ("require-cert.example.com") remains after a rejected update but doesn't assert the new (rejected) SNI is absent; update the assertion block around the ssl lookup (using runA7JSON, requireJSONArray and the snis variable) to also assert that the rejected/new SNI value is not present (e.g., add an assert.NotContains(t, snis, "<new-sni-host>") after the existing assert.Contains) so the test fails if the rejected update accidentally added the new SNI.
🤖 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-plan.md`:
- Around line 26-30: The fenced code block containing the environment variables
(A7_ADMIN_URL, A7_TOKEN, A7_GATEWAY_GROUP) is missing a language tag which
triggers MD040; update the opening fence to include a language (e.g., "text") so
it becomes ```text to satisfy markdown linting while keeping the block content
unchanged.
In `@pkg/cmd/config/configutil/configutil.go`:
- Around line 245-247: The validator currently only rejects service_templates
when cfg.ServiceTemplates has items; change the check to also reject an
explicitly set empty section by testing for a non-nil slice instead of length.
Replace the condition using len(cfg.ServiceTemplates) with a check like
cfg.ServiceTemplates != nil (or cfg.ServiceTemplates != nil ||
len(cfg.ServiceTemplates) > 0) so that an explicit service_templates: [] is
treated as unsupported and appended to the unsupported list (referencing
cfg.ServiceTemplates and the unsupported variable in configutil.go).
In `@pkg/cmd/config/validate/validate.go`:
- Around line 106-108: The current check only rejects non-empty slices
(len(cfg.ServiceTemplates) > 0) and therefore allows an explicitly-present empty
section; change the condition to detect presence instead of length by checking
for nil (if cfg.ServiceTemplates != nil) and append the same error to errs.
Update the validate logic that references cfg.ServiceTemplates and errs in
validate.go so any explicitly-provided service_templates (including []) is
rejected.
In `@pkg/cmd/ssl/update/update_test.go`:
- Around line 90-92: The test currently serializes the SSL payload (body) into
the error using "%#v", which can expose cert/key material; update the assertion
in update_test.go so that when the check on body.Cert and body.Key fails you
return a non-sensitive error message (e.g., "unexpected cert/key in payload" or
include only boolean context), removing any serialization of body, Cert, or Key
values; ensure the failing message references only that the cert/key did not
match expected flags and does not print the payload contents.
---
Nitpick comments:
In `@test/e2e/ssl_test.go`:
- Around line 176-181: The test currently checks that the original SNI
("require-cert.example.com") remains after a rejected update but doesn't assert
the new (rejected) SNI is absent; update the assertion block around the ssl
lookup (using runA7JSON, requireJSONArray and the snis variable) to also assert
that the rejected/new SNI value is not present (e.g., add an
assert.NotContains(t, snis, "<new-sni-host>") after the existing
assert.Contains) so the test fails if the rejected update accidentally added the
new SNI.
🪄 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: 893f658c-30f9-424c-840f-6a38143ea3ea
📒 Files selected for processing (17)
docs/ga-test-plan.mddocs/ga-test-report.mdpkg/api/types_config.gopkg/api/types_stream_route.gopkg/cmd/config/configutil/configutil.gopkg/cmd/config/validate/validate.gopkg/cmd/plugin-metadata/get/get.gopkg/cmd/plugin/get/get.gopkg/cmd/ssl/update/update.gopkg/cmd/ssl/update/update_test.gopkg/cmd/stream-route/create/create.gopkg/cmd/stream-route/create/create_test.gotest/e2e/config_test.gotest/e2e/plugin_metadata_test.gotest/e2e/plugin_test.gotest/e2e/ssl_test.gotest/e2e/stream_route_test.go
Comment on lines
+26
to
+30
| ``` | ||
| A7_ADMIN_URL = https://localhost:7443 # control-plane HTTPS port — confirm | ||
| A7_TOKEN = a7ee-<your-access-token> # access token from the dashboard UI / API | ||
| A7_GATEWAY_GROUP = default # resolved to a UUID at runtime | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block to satisfy markdown linting.
The block starting at Line 26 is missing a fence language (MD040).
Suggested fix
-```
+```text
A7_ADMIN_URL = https://localhost:7443 # control-plane HTTPS port — confirm
A7_TOKEN = a7ee-<your-access-token> # access token from the dashboard UI / API
A7_GATEWAY_GROUP = default # resolved to a UUID at runtime</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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-plan.md` around lines 26 - 30, The fenced code block containing
the environment variables (A7_ADMIN_URL, A7_TOKEN, A7_GATEWAY_GROUP) is missing
a language tag which triggers MD040; update the opening fence to include a
language (e.g., "text") so it becomes ```text to satisfy markdown linting while
keeping the block content unchanged.
Comment on lines
+245
to
+247
| if len(cfg.ServiceTemplates) > 0 { | ||
| unsupported = append(unsupported, "service_templates") | ||
| } |
There was a problem hiding this comment.
Reject explicit empty service_templates sections as well.
Current validation only rejects non-empty values; an explicit service_templates: [] can still pass even though the section is unsupported.
Suggested fix
- if len(cfg.ServiceTemplates) > 0 {
+ if cfg.ServiceTemplates != nil {
unsupported = append(unsupported, "service_templates")
}📝 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
| if len(cfg.ServiceTemplates) > 0 { | |
| unsupported = append(unsupported, "service_templates") | |
| } | |
| if cfg.ServiceTemplates != nil { | |
| unsupported = append(unsupported, "service_templates") | |
| } |
🤖 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 245 - 247, The
validator currently only rejects service_templates when cfg.ServiceTemplates has
items; change the check to also reject an explicitly set empty section by
testing for a non-nil slice instead of length. Replace the condition using
len(cfg.ServiceTemplates) with a check like cfg.ServiceTemplates != nil (or
cfg.ServiceTemplates != nil || len(cfg.ServiceTemplates) > 0) so that an
explicit service_templates: [] is treated as unsupported and appended to the
unsupported list (referencing cfg.ServiceTemplates and the unsupported variable
in configutil.go).
Comment on lines
+106
to
+108
| if len(cfg.ServiceTemplates) > 0 { | ||
| errs = append(errs, "service_templates are not supported by current API7 EE") | ||
| } |
There was a problem hiding this comment.
Reject explicitly-empty service_templates sections as well.
Current logic only fails when the slice is non-empty, so service_templates: [] is accepted even though the section is unsupported.
Suggested fix
- if len(cfg.ServiceTemplates) > 0 {
+ if cfg.ServiceTemplates != nil {
errs = append(errs, "service_templates are not supported by current API7 EE")
}📝 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
| if len(cfg.ServiceTemplates) > 0 { | |
| errs = append(errs, "service_templates are not supported by current API7 EE") | |
| } | |
| if cfg.ServiceTemplates != nil { | |
| errs = append(errs, "service_templates are not supported by current API7 EE") | |
| } |
🤖 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/validate/validate.go` around lines 106 - 108, The current
check only rejects non-empty slices (len(cfg.ServiceTemplates) > 0) and
therefore allows an explicitly-present empty section; change the condition to
detect presence instead of length by checking for nil (if cfg.ServiceTemplates
!= nil) and append the same error to errs. Update the validate logic that
references cfg.ServiceTemplates and errs in validate.go so any
explicitly-provided service_templates (including []) is rejected.
Comment on lines
+90
to
92
| if !strings.Contains(body.Cert, "NEWCERT") || !strings.Contains(body.Key, "NEWKEY") { | ||
| return httpmock.Response{}, fmt.Errorf("expected cert/key from flags in payload, got %#v", body) | ||
| } |
There was a problem hiding this comment.
Avoid serializing SSL payloads in assertion errors.
The failure message includes %#v of body, which can dump cert/key material into test logs. Please keep the error message non-sensitive.
Suggested fix
- if !strings.Contains(body.Cert, "NEWCERT") || !strings.Contains(body.Key, "NEWKEY") {
- return httpmock.Response{}, fmt.Errorf("expected cert/key from flags in payload, got %#v", body)
- }
+ if !strings.Contains(body.Cert, "NEWCERT") || !strings.Contains(body.Key, "NEWKEY") {
+ return httpmock.Response{}, fmt.Errorf("expected cert/key from flags in payload")
+ }As per coding guidelines, "Do not log, serialize, or return API keys, tokens (Bearer, JWT, access_token, refresh_token), OAuth client secrets, or plugin configuration payloads without redaction".
📝 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
| if !strings.Contains(body.Cert, "NEWCERT") || !strings.Contains(body.Key, "NEWKEY") { | |
| return httpmock.Response{}, fmt.Errorf("expected cert/key from flags in payload, got %#v", body) | |
| } | |
| if !strings.Contains(body.Cert, "NEWCERT") || !strings.Contains(body.Key, "NEWKEY") { | |
| return httpmock.Response{}, fmt.Errorf("expected cert/key from flags in payload") | |
| } |
🤖 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/ssl/update/update_test.go` around lines 90 - 92, The test currently
serializes the SSL payload (body) into the error using "%#v", which can expose
cert/key material; update the assertion in update_test.go so that when the check
on body.Cert and body.Key fails you return a non-sensitive error message (e.g.,
"unexpected cert/key in payload" or include only boolean context), removing any
serialization of body, Cert, or Key values; ensure the failing message
references only that the cert/key did not match expected flags and does not
print the payload contents.
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple correctness bugs discovered during GA validation of the a7 CLI against a real API7 EE 3.9.12 instance, and adds/updates regression coverage plus GA documentation.
Changes:
- Fix output-format handling for
plugin getandplugin-metadata getso-o yamlemits valid YAML objects (not JSON / byte arrays). - Fix runtime resource command validation/behavior: require
--cert/--keyfor flag-basedssl update, add--name(and API field) for flag-basedstream-route create. - Reject unsupported declarative
service_templatessections inconfig validateand config diff/sync validation paths; add E2E + unit coverage and GA plan/report docs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/stream_route_test.go | Adds E2E regression tests for stream-route flag create requiring --name and successful flag create round-trip assertions. |
| test/e2e/ssl_test.go | Adds E2E regression coverage ensuring flag-based ssl update requires cert/key and does not silently drop changes. |
| test/e2e/plugin_test.go | Adds E2E regression coverage ensuring plugin get -o yaml produces valid YAML (not JSON). |
| test/e2e/plugin_metadata_test.go | Adds E2E regression coverage ensuring plugin-metadata get -o yaml produces a YAML map. |
| test/e2e/config_test.go | Adds E2E coverage that config validate rejects unsupported service_templates. |
| pkg/cmd/stream-route/create/create.go | Adds --name flag and enforces name for both file-based and flag-based stream-route creation; includes Name in request payload. |
| pkg/cmd/stream-route/create/create_test.go | Updates/extends unit tests for required name validation in both flag and -f paths. |
| pkg/cmd/ssl/update/update.go | Prevents flag-based partial ssl update by requiring both --cert and --key. |
| pkg/cmd/ssl/update/update_test.go | Updates unit tests to reflect API7 EE GET behavior (no cert/key) and adds regression test for requiring cert/key. |
| pkg/cmd/plugin/get/get.go | Ensures explicit --output is honored via exporter (yaml/json) instead of falling back to JSON. |
| pkg/cmd/plugin-metadata/get/get.go | Exports decoded metadata map (not json.RawMessage) so YAML output is correct. |
| pkg/cmd/config/validate/validate.go | Adds explicit validation error when service_templates is present in declarative config. |
| pkg/cmd/config/configutil/configutil.go | Extends supported-section validation so diff/sync paths reject service_templates. |
| pkg/api/types_stream_route.go | Adds name field to StreamRoute type to match API7 EE requirements. |
| pkg/api/types_config.go | Adds ServiceTemplates field to detect/reject unsupported declarative section instead of silently dropping it. |
| docs/ga-test-plan.md | Adds GA validation test plan for local API7 EE 3.9.12. |
| docs/ga-test-report.md | Adds GA test report capturing findings, regressions, and exit-criteria assessment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
80
to
+85
| if err := cmdutil.EnsureRequiredStringField(payload, "service_id", opts.ServiceID, "--service-id is required for current API7 EE"); err != nil { | ||
| return err | ||
| } | ||
| if err := cmdutil.EnsureRequiredStringField(payload, "name", opts.Name, "name is required by API7 EE; set it in the file or pass --name"); err != nil { | ||
| return err | ||
| } |
Comment on lines
+166
to
+168
| "nodes": [{"host": "127.0.0.1", "port": 3306, "weight": 1}] | ||
| } | ||
| }`, id, id) |
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
Bug fixes surfaced while validating the
a7CLI against a real API7 EE 3.9.12 instance (the GA test plan run, Phases B–C). Each fix ships with regression coverage. Also adds the GA test plan and the resulting test report underdocs/.Bug fixes
ssl updatesilently dropped partial updates; now requires--cert/--key.plugin-metadata get -o yamlemitted a byte array; now passes the decoded map to the exporter.plugin get -o yamlemitted JSON; now honors any--outputvalue.stream-route createhad no--nameflag; added theNamefield and validation.config validatesilently accepted an unsupported top-levelservice_templatessection; now rejected on the validate and diff/sync paths.Docs
docs/ga-test-plan.md— the GA readiness test plan (local API7 EE 3.9.12).docs/ga-test-report.md— the report from running it: bug write-ups, observations, exit-criteria assessment.Test plan
go build ./...,go vet ./...,go test ./...all pass locally.TestPluginMetadata_GetYAML,TestPlugin_GetYAML,TestStreamRoute_CreateRequiresName,TestConfigValidate*(run with-tags e2eagainst a live instance).Part of #22. Relates to #25 and #26.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation