fix: honor secret create file positional id#13
Conversation
📝 WalkthroughWalkthroughWhen creating a secret from a file, the CLI now enforces a non-empty Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI)
participant CLI as Create Command
participant Payload as Payload (file/flags)
participant HTTP as HTTP Client
participant Server as API Server
User->>CLI: run `a7 secret create --file ...` [maybe --id]
CLI->>Payload: load JSON from file and read flags
CLI->>Payload: if --id provided -> set payload["id"]=trim(--id)
CLI->>Payload: else ensure payload["id"] exists and trimmed non-empty -> else error
CLI->>HTTP: build request (PUT if id present, else POST) with payload
HTTP->>Server: send request
Server-->>HTTP: response
HTTP-->>CLI: return response
CLI-->>User: print result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var secret map[string]interface{} | ||
| runA7JSON(t, env, &secret, "secret", "get", secretID, "-g", gatewayGroup, "-o", "json") | ||
| assert.Equal(t, secretID, secret["id"]) | ||
| assert.Equal(t, "e2e-secret-crud", secret["id"]) |
There was a problem hiding this comment.
The E2E JSON assertion now expects only the suffix ("e2e-secret-crud"), but the CLI/API elsewhere (e.g., pkg/cmd/secret/get tests) treats id as the full compound ID ("vault/"). Since secretID is "vault/e2e-secret-crud", this will likely fail or mask regressions. Consider asserting against secretID (or explicitly document/justify why the API returns a stripped id here).
| assert.Equal(t, "e2e-secret-crud", secret["id"]) | |
| assert.Equal(t, secretID, secret["id"]) |
| return err | ||
| } | ||
| if opts.ID != "" { | ||
| payload["id"] = opts.ID |
There was a problem hiding this comment.
In file mode, if the user runs a7 secret create -f <file> without a positional ID/--id and the file also omits id, this code will still POST the payload. That contradicts the documented workflow (docs/user-guide/secret.md shows create requires a compound ID) and can lead to confusing server-side errors or unintended auto-generated IDs. Suggest validating that either opts.ID is set or the loaded payload already contains id, and returning the same "secret provider id is required; use a positional arg or --id" error when neither is present.
| payload["id"] = opts.ID | |
| payload["id"] = opts.ID | |
| } else { | |
| if id, ok := payload["id"]; !ok || id == nil { | |
| return fmt.Errorf("secret provider id is required; use a positional arg or --id") | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/secret_test.go`:
- Around line 78-80: The test asserts the short secret id but the command
returns the raw API id (prefixed); update the expectation in
test/e2e/secret_test.go to assert the full id "vault/e2e-secret-crud" (or
alternatively add normalization in pkg/cmd/secret/get/get.go to strip the
"vault/" prefix before printing), referencing the code path that uses item.ID
and the unmarshaling in pkg/api/types_secret.go so the test matches the actual
output.
🪄 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: 97a8f56a-340c-4397-8fc7-4a498ccd6d96
📒 Files selected for processing (3)
pkg/cmd/secret/create/create.gopkg/cmd/secret/create/create_test.gotest/e2e/secret_test.go
| assert.Equal(t, "e2e-secret-crud", secret["id"]) | ||
| assert.Equal(t, "https://vault.example.com", secret["uri"]) | ||
| assert.Equal(t, "kv/apisix", secret["prefix"]) |
There was a problem hiding this comment.
Expect the full secret id here, or normalize it in the command first.
pkg/cmd/secret/get/get.go returns item.ID as the API sent it, and pkg/api/types_secret.go unmarshals id unchanged. Without a separate normalization layer, this assertion should expect vault/e2e-secret-crud, not just e2e-secret-crud.
Suggested fix
- assert.Equal(t, "e2e-secret-crud", secret["id"])
+ assert.Equal(t, "vault/e2e-secret-crud", secret["id"])📝 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.
| assert.Equal(t, "e2e-secret-crud", secret["id"]) | |
| assert.Equal(t, "https://vault.example.com", secret["uri"]) | |
| assert.Equal(t, "kv/apisix", secret["prefix"]) | |
| assert.Equal(t, "vault/e2e-secret-crud", secret["id"]) | |
| assert.Equal(t, "https://vault.example.com", secret["uri"]) | |
| assert.Equal(t, "kv/apisix", secret["prefix"]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/secret_test.go` around lines 78 - 80, The test asserts the short
secret id but the command returns the raw API id (prefixed); update the
expectation in test/e2e/secret_test.go to assert the full id
"vault/e2e-secret-crud" (or alternatively add normalization in
pkg/cmd/secret/get/get.go to strip the "vault/" prefix before printing),
referencing the code path that uses item.ID and the unmarshaling in
pkg/api/types_secret.go so the test matches the actual output.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/secret/create/create.go`:
- Around line 82-86: Reject whitespace-only IDs by trimming before validation
and assignment: when handling opts.ID, call strings.TrimSpace(opts.ID) and if
the result is empty return the same error instead of accepting it, and assign
the trimmed value to payload["id"]; similarly, when checking payload["id"] use
strings.TrimSpace(fmt.Sprint(id)) == "" (or convert+trim once) to detect and
reject whitespace-only values. Also ensure the strings package is imported.
🪄 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: 8a7555c5-939b-43c8-be27-0ca3efcaec44
📒 Files selected for processing (3)
pkg/cmd/secret/create/create.gopkg/cmd/secret/create/create_test.gotest/e2e/secret_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/secret_test.go
- pkg/cmd/secret/create/create_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| id, ok := payload["id"] | ||
| if !ok || id == nil { | ||
| return fmt.Errorf("secret provider id is required; use a positional arg or --id") | ||
| } | ||
| trimmedID := strings.TrimSpace(fmt.Sprint(id)) | ||
| if trimmedID == "" { | ||
| return fmt.Errorf("secret provider id is required; use a positional arg or --id") | ||
| } | ||
| payload["id"] = trimmedID | ||
| } |
There was a problem hiding this comment.
After the new ID enforcement/normalization, payload["id"] is always set for --file runs (either from opts.ID or validated from the file). The later if id, ok := payload["id"]; ok { ... } else { ... } means the POST branch is now effectively unreachable in file mode. Consider simplifying this path to always issue the PUT-by-id request once the ID is guaranteed, to reduce dead/duplicated control flow.
| // The runtime secret provider API returns the provider-local ID; the CLI | ||
| // still uses the compound ID for addressing resources. | ||
| assert.Equal(t, "e2e-secret-crud", secret["id"]) |
There was a problem hiding this comment.
This assertion hard-codes the provider-local ID ("e2e-secret-crud") while the test defines secretID as the compound manager/id. To avoid the test becoming inconsistent if secretID changes, derive the expected provider-local ID from secretID (e.g., split on the first '/').
| if opts.ID != "" { | ||
| id := strings.TrimSpace(opts.ID) | ||
| if id == "" { | ||
| return fmt.Errorf("secret provider id is required; use a positional arg or --id") | ||
| } |
There was a problem hiding this comment.
In file mode you now trim/validate opts.ID, but in the non-file path the only check is opts.ID == "". That means a7 secret create " " --uri ... (or --id " ") will pass local validation and fail later at the API, which is inconsistent with the stricter behavior added for file-based create. Consider normalizing/validating the ID (trim + non-empty) once and reusing it for both file and non-file modes (and ideally rejecting non-string payload["id"] rather than fmt.Sprint-coercing it).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cmd/secret/create/create_test.go (1)
105-153: Optional: extract shared file-test setup into helpers.
TempDirfile creation andOptionsconstruction are repeated; a small helper would reduce duplication and keep future test additions cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/secret/create/create_test.go` around lines 105 - 153, The two tests TestCreateSecret_FileRequiresID and TestCreateSecret_FileRejectsWhitespaceID duplicate TempDir file creation, writing JSON to a file, building ios, registry and constructing an Options struct for actionRun; extract that repeated setup into a small helper (e.g., newSecretFileTest or setupSecretFileTest) that returns (ios, registry, filePath, Options) or returns components needed (ios, registry, filePath, baseOptions) so both tests call the helper to create the temp file with provided JSON content and get a prepopulated Options (IO, Client func using registry.GetClient, Config func returning mockConfig, GatewayGroup) then set/override only the File field and call actionRun, keeping test assertions unchanged (referencing TestCreateSecret_FileRequiresID, TestCreateSecret_FileRejectsWhitespaceID, actionRun, Options, File, mockConfig, registry).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/secret/create/create_test.go`:
- Around line 105-153: The two tests TestCreateSecret_FileRequiresID and
TestCreateSecret_FileRejectsWhitespaceID duplicate TempDir file creation,
writing JSON to a file, building ios, registry and constructing an Options
struct for actionRun; extract that repeated setup into a small helper (e.g.,
newSecretFileTest or setupSecretFileTest) that returns (ios, registry, filePath,
Options) or returns components needed (ios, registry, filePath, baseOptions) so
both tests call the helper to create the temp file with provided JSON content
and get a prepopulated Options (IO, Client func using registry.GetClient, Config
func returning mockConfig, GatewayGroup) then set/override only the File field
and call actionRun, keeping test assertions unchanged (referencing
TestCreateSecret_FileRequiresID, TestCreateSecret_FileRejectsWhitespaceID,
actionRun, Options, File, mockConfig, registry).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a354970-3c3a-4154-9a19-53d035391a0c
📒 Files selected for processing (2)
pkg/cmd/secret/create/create.gopkg/cmd/secret/create/create_test.go
Summary
a7 secret create <provider/id> -f fileinject the positional provider ID into the file payload.Validation
go test ./pkg/cmd/secret/create ./pkg/cmd/secret/update ./pkg/cmd/secret/get ./pkg/cmd/secret/list ./pkg/cmd/secret/deletego test -c -tags=e2e ./test/e2ego test ./...git diff --checkSummary by CodeRabbit
Bug Fixes
Tests