fix: resolve CLI bugs found during GA validation#31
Merged
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.
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds GA readiness plan and report docs; rejects top-level ChangesGA Readiness Testing & Bug Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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: 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
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.
- stream-route create: decode response before passing to the exporter so -o yaml renders a YAML map, not a byte sequence - config validate / configutil: reject explicit empty unsupported sections (upstreams, consumer_groups, service_templates) via != nil check; declaring an unsupported section is itself an error - ssl update test: drop %#v of the SSL body from the mock-mismatch error message - ssl e2e: also assert the rejected SNI is absent after a failed update - stream-route e2e helper: use upstreamNodeHost/Port helpers for consistency - new e2e: TestStreamRoute_CreateFromFileYAML guards the json.RawMessage regression on the file-based create path - new unit test: TestConfigValidate_EmptyUnsupportedSections covers the three empty-section cases
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/ssl/update/update_test.go`:
- Around line 121-123: The failing assertions leak cert/key material by printing
the full test output using "%+v"; replace those fatal messages to avoid
serializing secrets (e.g., remove "%+v" usage and only assert silently or log
safe fields). Specifically, update the assertions that reference the test
variable "output" (and comparisons against "api.RedactedSSLKey") so they call
t.Fatalf with a non-sensitive message like "expected ssl key to be redacted in
output" (or include only the redacted value), and apply the same fix to the
other occurrences around the test checks noted (the blocks that compare
output.Key/output.Cert to api.RedactedSSLKey) so no cert/key bytes are ever
formatted into the error string.
🪄 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: 35b3bd8b-c89c-43c7-85fd-e4f75768a5a0
📒 Files selected for processing (7)
pkg/cmd/config/configutil/configutil.gopkg/cmd/config/validate/validate.gopkg/cmd/config/validate/validate_test.gopkg/cmd/ssl/update/update_test.gopkg/cmd/stream-route/create/create.gotest/e2e/ssl_test.gotest/e2e/stream_route_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cmd/config/validate/validate.go
- pkg/cmd/stream-route/create/create.go
- test/e2e/stream_route_test.go
- ssl update_test: drop %+v output / %#v payload from the remaining three failure messages - config validate / configutil: correct the comment to match what != nil actually catches (explicitly-empty sections); bare 'upstreams:' / 'upstreams: null' still slip through and need a separate key-presence pass tracked as a follow-up - docs/ga-test-plan: replace hardcoded contributor path with <path-to-a7-repo>
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
Tests