Role-binding flag text, linter bool-flag exception, logout no-op. - #3421
Closed
David Adams (davidadas) wants to merge 5 commits into
Closed
Role-binding flag text, linter bool-flag exception, logout no-op.#3421David 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>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull request overview
This pull request updates the confluent iam rbac role-binding list command help text for the --inclusive flag and refreshes the corresponding integration-test golden fixtures so the expected --help output matches.
Changes:
- Updates the
--inclusiveflag description ininternal/iam/command_rbac_role_binding_list.go. - Regenerates/updates golden
--helpoutputs for both cloud and on-prem variants (including failure-help fixtures).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/iam/command_rbac_role_binding_list.go |
Updates the --inclusive flag help string for role-binding listing. |
test/fixtures/output/iam/rbac/role-binding/list-help.golden |
Updates expected cloud list --help output for the --inclusive flag line. |
test/fixtures/output/iam/rbac/role-binding/list-help-onprem.golden |
Updates expected on-prem list --help output for the --inclusive flag line. |
test/fixtures/output/iam/rbac/role-binding/list-failure-help-cloud.golden |
Updates expected cloud failure help output for the --inclusive flag line. |
test/fixtures/output/iam/rbac/role-binding/list-failure-help-onprem.golden |
Updates expected on-prem failure help output for the --inclusive flag line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…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>
David Adams (davidadas)
force-pushed
the
dadams/apie-papercuts-batch-2
branch
from
July 30, 2026 21:14
cf69f23 to
bca05bc
Compare
--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>
David Adams (davidadas)
marked this pull request as ready for review
July 30, 2026 22:04
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>
Contributor
Author
|
Superseded by #3422, opened from a branch on confluentinc/cli directly instead of my fork. |
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).