Skip to content

fix(credential): drop misleading [id] positional from create#46

Closed
shreemaan-abhishek wants to merge 1 commit into
masterfrom
fix/credential-create-drop-id
Closed

fix(credential): drop misleading [id] positional from create#46
shreemaan-abhishek wants to merge 1 commit into
masterfrom
fix/credential-create-drop-id

Conversation

@shreemaan-abhishek
Copy link
Copy Markdown
Contributor

@shreemaan-abhishek shreemaan-abhishek commented May 26, 2026

Summary

  • Closes credential: positional [id] is ignored — clarify or fix #36.
  • The credential create [id] positional was wired into the request body's name field, not id, so the help signature was misleading. Rename the positional to [name] and reflect that in the Use string, Options field name, the existing positional test, and the user-guide doc.
  • Behavior on the wire is unchanged: POST /apisix/admin/consumers/<u>/credentials with name set, server assigns the id.

Test plan

  • go test ./pkg/cmd/credential/... passes (renamed TestCreateCredential_PositionalSetsName covers the positional -> name path).
  • go test ./... passes.
  • go build ./... clean.
  • Manual smoke: a7 credential create my-key --consumer alice -g default --plugins-json '{"key-auth":{"key":"k"}}' returns a credential whose name is my-key and id is a server-generated UUID; a7 credential create --help no longer advertises [id].

Note: make lint surfaces a pre-existing yaml typecheck error in pkg/cmdutil and pkg/cmd/config/configutil — reproducible on plain master, unrelated to this change.

Summary by CodeRabbit

  • Documentation

    • Updated credential create command documentation with new parameter details, behavior clarification, and inline creation examples
  • New Features

    • Credential creation now uses server-generated IDs; positional [name] argument now represents display name only
    • Added inline credential creation options: --plugins-json and --labels flags
    • Introduced --desc flag for credential descriptions; --file parameter now optional
    • Changed default --output format to JSON

Review Change Stack

The `credential create [id]` positional was ignored on the wire: the
CLI POSTed to /apisix/admin/consumers/<u>/credentials with the value
placed in `name`, and the server returned a fresh UUID for `id`. The
help text promised something the code never did.

Rename the positional to `[name]` and reflect that in:
- the `Use` string and short description (id is server-generated)
- the `Options.ID` -> `Options.Name` field (matches what the value
  actually maps to in the request body)
- the existing positional test, renamed to drop "ID" from the title
- the user-guide doc, with a note that the returned `id` differs from
  the positional `[name]`

Behavior on the wire is unchanged. Closes #36.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

📝 Walkthrough

Walkthrough

This PR addresses the misleading credential create command signature by renaming the positional argument from [id] to [name] and clarifying that credential IDs are always server-generated. The command options struct, argument handling, payload construction, test validation, and user documentation are all updated cohesively to reflect this change.

Changes

Credential create: positional name instead of ID

Layer / File(s) Summary
Command options and signature
pkg/cmd/credential/create/create.go
Options struct field changed from ID to Name; cobra command updated to Use: "create [name]" with help text clarifying that IDs are server-generated; positional argument now populates opts.Name.
Payload construction and API request
pkg/cmd/credential/create/create.go
Resource file payload handling updated to set payload["name"] from opts.Name and remove any id field; API credential object now constructed with Name: opts.Name instead of opts.ID.
Test updates
pkg/cmd/credential/create/create_test.go
Test function renamed from TestCreateCredential_PositionalIDSetsName to TestCreateCredential_PositionalSetsName; test assertions require positional input to map to name only with explicit validation that id is absent from request payload; test options now use Name field.
User documentation
docs/user-guide/credential.md
Command description rewritten to document server-generated id and optional positional [name]; flags table expanded to include inline creation options (--plugins-json, --labels, --desc) and reflect --file as optional; new inline creation example added with note clarifying that returned id differs from positional [name] and must be used for follow-up operations.

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: the positional argument for the credential create command is renamed from the misleading [id] to [name] to reflect that it maps to the name field, not the server-generated id.
Linked Issues check ✅ Passed The pull request fully addresses issue #36 by choosing option 2: dropping the misleading [id] positional, renaming it to [name], updating all related code and tests, and documenting that credential IDs are server-generated.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the misleading credential create positional argument per issue #36; no unrelated changes detected.
E2e Test Quality Review ✅ Passed E2E test validates server-generated ID; unit tests comprehensive; proper error handling; scope matches issue #36; no unrelated changes.
Security Check ✅ Passed No security vulnerabilities found. PR correctly normalizes credential name handling without introducing sensitive data exposure or authorization bypasses.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/credential-create-drop-id

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
pkg/cmd/credential/create/create_test.go (1)

1-1: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add Apache 2.0 license header to this Go test file.

This modified .go test file is missing the required Apache 2.0 license header.

As per coding guidelines, "Use Apache 2.0 license for all source files".

🤖 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/credential/create/create_test.go` at line 1, The test file
create_test.go is missing the required Apache 2.0 license header; add the
standard Apache 2.0 license comment block at the top of the file above the line
"package create" so every source file follows the project's licensing guideline;
ensure the header matches the project's canonical Apache 2.0 text and formatting
used across other Go files in the repo.
pkg/cmd/credential/create/create.go (1)

1-1: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add Apache 2.0 license header to this Go source file.

This modified .go file is missing the required Apache 2.0 license header.

As per coding guidelines, "Use Apache 2.0 license for all source files".

🤖 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/credential/create/create.go` at line 1, This file lacks the required
Apache 2.0 license header; add the standard Apache 2.0 comment block at the very
top of the file (above the package declaration) including the correct year and
copyright/owner, so the package create source (create.go) begins with the full
Apache 2.0 header comment followed by "package create".
🤖 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/user-guide/credential.md`:
- Around line 59-61: Update the later YAML examples used in the create flows to
remove any manual `id` fields and instead rely on the server-generated id;
specifically edit the examples that show creating credentials (refer to the `a7
credential create [name] --consumer <user> -g <gateway-group>` usage) and any
YAML blocks that currently include `id:` (also referenced near the second create
example around the later block). Ensure the examples only include allowed input
fields such as `name`, `consumer`, `gateway-group` (or equivalent) and add a
brief comment or sentence showing that the `id` will be returned/generated by
the server rather than provided by the user.

---

Outside diff comments:
In `@pkg/cmd/credential/create/create_test.go`:
- Line 1: The test file create_test.go is missing the required Apache 2.0
license header; add the standard Apache 2.0 license comment block at the top of
the file above the line "package create" so every source file follows the
project's licensing guideline; ensure the header matches the project's canonical
Apache 2.0 text and formatting used across other Go files in the repo.

In `@pkg/cmd/credential/create/create.go`:
- Line 1: This file lacks the required Apache 2.0 license header; add the
standard Apache 2.0 comment block at the very top of the file (above the package
declaration) including the correct year and copyright/owner, so the package
create source (create.go) begins with the full Apache 2.0 header comment
followed by "package create".
🪄 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: 29d9978a-731e-491d-8742-6c7caca95f12

📥 Commits

Reviewing files that changed from the base of the PR and between 3fbbb89 and fc87fbe.

📒 Files selected for processing (3)
  • docs/user-guide/credential.md
  • pkg/cmd/credential/create/create.go
  • pkg/cmd/credential/create/create_test.go

Comment on lines +59 to +61
Creates a new credential for a consumer. The credential `id` is always server-generated; the optional positional `[name]` (or the `name` field in a file payload) is stored as the credential's display name.

**Usage:** `a7 credential create [name] --consumer <user> -g <gateway-group>`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align later config examples with the new server-generated id guidance.

This section correctly says id is server-generated, but later YAML examples still show setting id during create flows, which is misleading after this change.

Also applies to: 86-87

🤖 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/credential.md` around lines 59 - 61, Update the later YAML
examples used in the create flows to remove any manual `id` fields and instead
rely on the server-generated id; specifically edit the examples that show
creating credentials (refer to the `a7 credential create [name] --consumer
<user> -g <gateway-group>` usage) and any YAML blocks that currently include
`id:` (also referenced near the second create example around the later block).
Ensure the examples only include allowed input fields such as `name`,
`consumer`, `gateway-group` (or equivalent) and add a brief comment or sentence
showing that the `id` will be returned/generated by the server rather than
provided by the user.

@shreemaan-abhishek
Copy link
Copy Markdown
Contributor Author

Closing in favor of option 1 from the issue. After auditing api7/api7-ee-3 control-plane, PUT /apisix/admin/consumers//credentials/ upserts a credential with the client-supplied id (middleware: interceptor.go:386-400; DAO: consumer.go:352-364 OnConflict UpdateAll; BeforeCreate hook generates UUID only when CredentialID is empty). Reopening as a new PR that forwards the positional as id via PUT and adds a separate --name flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

credential: positional [id] is ignored — clarify or fix

1 participant