Role-binding flag text, linter bool-flag exception, logout no-op. - #3422
Closed
David Adams (davidadas) wants to merge 5 commits into
Closed
Role-binding flag text, linter bool-flag exception, logout no-op.#3422David Adams (davidadas) wants to merge 5 commits into
David Adams (davidadas) wants to merge 5 commits into
Conversation
…ng list
The flag's help text said "If scopes are unspecified, list only
organization-scoped role bindings," but the command's own documented
example ("for all scopes") and actual behavior return role bindings
across all scopes (org, environment, cluster) in that case. Corrected
the description to match, and updated the 4 golden fixtures that
pinned the old text.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…g the linter RequireValidExamples() flagged any --flag=value in an example, but boolean flags legitimately need "=" to set the non-default value (e.g. --flag=false). Added getBoolFlags() and excluded them from the "=" check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
logout used NewAuthenticatedCLICommand, which required being logged in (erroring "not logged in" with no context) and would auto-login via env-var credentials only to immediately log back out. Switched to NewAnonymousCLICommand (same pattern as `login`) and return immediately when there's no active context, instead of erroring or auto-authenticating. The ccloud client used to revoke the refresh token is now constructed directly in revokeCCloudRefreshToken rather than relying on the Authenticated PreRun to populate it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
--inclusive is only read by the Cloud-path functions (listMyRoleBindings, ccloudListRolePrincipals), both called exclusively from ccloudList. The on-prem path (confluentList) never reads it, even though the flag is registered unconditionally for both login modes. Clarified the flag description as Confluent Cloud-only so on-prem users don't think it does something. Updated all 4 golden fixtures that pin this text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
revokeCCloudRefreshToken accessed c.Version.UserAgent directly, but some PreRunner mocks (e.g. internal/login's cross-package test via mock.Commander) never populate Version, causing a nil pointer panic. Confirmed via the full test suite: TestLoginWithExistingContext panicked in internal/login before this fix, passes after it. Guard with a nil check instead of relying on every PreRunner implementation to set Version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
13 tasks
Contributor
Author
|
Superseded by #3423 — renamed branch from dadams/apie-papercuts-batch-2 to apie-papercuts-batch-2. |
There was a problem hiding this comment.
🟡 Not ready to approve
The new logout no-op path should be covered by an integration test, and there’s a small but concrete naming clarity issue introduced by importing context alongside a local context variable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes three user-facing/help-text and developer-tooling issues in the Confluent CLI: clarifying --inclusive role-binding help text, improving the examples linter to permit --bool=false syntax, and making confluent logout a true no-op when already logged out (without triggering env-var auto-login).
Changes:
- Update
iam rbac role-binding list --inclusiveflag usage text and refresh associated golden fixtures. - Adjust
RequireValidExamples()lint rule to allow--flag=valuespecifically for boolean flags. - Change
logoutto use an anonymous prerun and return immediately when no active context exists; build the CCloud client only when needed to revoke refresh tokens.
File summaries
| File | Description |
|---|---|
| test/fixtures/output/iam/rbac/role-binding/list-help.golden | Updates --inclusive help text to match intended behavior. |
| test/fixtures/output/iam/rbac/role-binding/list-help-onprem.golden | Same --inclusive help-text update for on-prem help output fixture. |
| test/fixtures/output/iam/rbac/role-binding/list-failure-help-onprem.golden | Updates --inclusive help text in failure help-output fixture. |
| test/fixtures/output/iam/rbac/role-binding/list-failure-help-cloud.golden | Updates --inclusive help text in cloud failure help-output fixture. |
| pkg/linter/command_rules.go | Allows --bool=false in examples by excluding bool flags from the --flag=value rejection. |
| internal/logout/command.go | Makes logout anonymous/no-op when already logged out; constructs CCloud client only when revoking token. |
| internal/iam/command_rbac_role_binding_list.go | Updates the --inclusive flag usage string. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
35
to
+39
| } else if cfg.IsOnPremLogin() { | ||
| context = "Confluent Platform" | ||
| c.AuthenticatedCLICommand = pcmd.NewAuthenticatedWithMDSCLICommand(cmd, prerunner) | ||
| } | ||
|
|
||
| c := &command{ |
Comment on lines
55
to
+59
| ctx := c.Config.Context() | ||
| if ctx != nil { | ||
| if ccloudv2.IsCCloudURL(ctx.Platform.Server, c.cfg.IsTest) { | ||
| if _, err := c.revokeCCloudRefreshToken(ctx); err != nil { | ||
| return err | ||
| } | ||
| if ctx == nil { | ||
| // Already logged out: do nothing. | ||
| return nil | ||
| } |
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.
Release Notes
Bug Fixes
confluent iam rbac role-binding list --inclusivehad a help-text description that contradicted its own documented example and actual behavior. (APIE-1362)RequireValidExamples()lint rule incorrectly flagged boolean flags using--flag=valuesyntax in command examples. (APIE-1286)confluent logouterrored or silently auto-logged-in-then-out instead of being a no-op when already logged out. (APIE-1314)Checklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.Unchecked items need human follow-up before merge — no Go toolchain was available in the environment this PR was prepared in, so no build/lint/test run or live verification was possible. This is more important than usual for this PR since APIE-1314 changes which PreRun a command uses — please run
make build && make lint && make test(especiallyTestLogout_RemoveUsernamePasswordandTestLogout_RemoveUsernamePasswordFail) before merging. Opening as a draft for that reason.What
Confluent Cloud and Confluent Platform (on-prem) both — all three fixes touch shared code paths used by both login modes.
--inclusiveflag's help text said "If scopes are unspecified, list only organization-scoped role bindings", contradicting the command's own documented example ("for all scopes") and the reporter's confirmed actual behavior. Corrected the text and the 4 golden fixtures that pinned the old wording.RequireValidExamples()(pkg/linter/command_rules.go) flagged any--flag=valuein an example. Boolean flags legitimately need=to set the non-default value (e.g.--flag=false). Added agetBoolFlags()helper and excluded boolean flags from that check.confluent logoutusedNewAuthenticatedCLICommand, which requires being logged in. With no active session this producedError: not logged in; withCONFLUENT_CLOUD_API_KEY/SECRETenv vars set, it auto-logged-in only to immediately log back out. Switched toNewAnonymousCLICommand(the same patternloginitself uses) and return immediately with no error when there's no active context. The ccloud client used to revoke the refresh token (previously populated by theAuthenticatedPreRun) is now constructed directly insiderevokeCCloudRefreshToken, only when a real session is confirmed to exist.Blast Radius
--helpdescription string. No behavior or serialized-output change.make lint-cli); no runtime CLI behavior changes for customers.confluent logout. Customers who are genuinely logged in see no change (still revokes the token, persists logout, prints the same success message). Customers who runlogoutwhile already logged out previously saw an error or an auto-login/logout cycle; they'll now see nothing happen at all, matching the ticket's explicit ask. Worth a careful look during review since it changes which PreRun the command uses.References
Test & Review
No Go toolchain was available in the environment this PR was prepared in, so
make build/make lint/make testwere not run here — please run these before merging, and pay particular attention totest/logout_test.gogiven the APIE-1314 change.What was actually verified:
RequireValidExamples(), so nothing to break; the newgetBoolFlags()helper mirrors the existinggetAllFlags()/getRequiredFlags()pattern in the same file.NewAnonymousCLICommandis the same constructor already used byinternal/login/command.go; tracedcommand.Version = r.Versioninto theAnonymous()PreRun function (pkg/cmd/prerunner.go:108) to confirmc.Versionis populated beforerevokeCCloudRefreshTokenneeds it; confirmed the existingtest/logout_test.gotests always log in immediately before logging out, so the no-op path isn't exercised by them (a new test for the no-op case would be worth adding).