Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI Unification: Warn if command is unavailable #962

Merged
merged 30 commits into from
Aug 27, 2021
Merged

Conversation

brianstrauch
Copy link
Member

@brianstrauch brianstrauch commented Aug 6, 2021

Checklist

  1. [CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?

    • yes: ok
  2. Did you add/update any commands that accept secrets as args/flags?

    • no: ok

What

Instead of completely removing certain commands from the CLI when a user isn't logged in with the correct context, we hide the command and show an error when the command is used.

The approach is to use Cobra annotations to label the parent commands that require contexts (on-prem, cloud, cloud with API key, etc). For example, confluent price is labeled as requiring a cloud login, so confluent price and all of its subcommands will err when used without a cloud login.

Examples:

The user is not logged in, but they try using a cloud command.

$ confluent price list      
Error: you must log in to Confluent Cloud with a username and password to use this command

Suggestions:
    Log in with "confluent login"

The user is not logged in, but they try using an on-prem command.

$ confluent cluster
Error: you must log in to Confluent Platform to use this command

Suggestions:
    Log in with "confluent login --url <mds-url>"

Test & Review

Manually verified each command works as intended and changed a few integration tests.

@brianstrauch brianstrauch requested a review from a team as a code owner August 6, 2021 01:01
Copy link
Contributor

@mtodzo mtodzo left a comment

Choose a reason for hiding this comment

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

Looks good overall - def feel like we're doing extra work of building the whole command tree and then looping back through and hiding them, but then again I don't have a better solution :)

internal/cmd/audit-log/command.go Show resolved Hide resolved
cmd/lint/main.go Outdated Show resolved Hide resolved
internal/cmd/audit-log/command_migrate.go Show resolved Hide resolved
internal/cmd/cluster/command.go Show resolved Hide resolved
internal/pkg/cmd/prerunner.go Outdated Show resolved Hide resolved
internal/cmd/signup/command_signup.go Outdated Show resolved Hide resolved
internal/cmd/iam/command.go Show resolved Hide resolved
internal/cmd/kafka/command_cluster.go Show resolved Hide resolved
internal/cmd/kafka/command_cluster.go Show resolved Hide resolved
internal/cmd/command.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mtodzo mtodzo left a comment

Choose a reason for hiding this comment

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

last couple questions ... and i assume Ethan has approved this? Maybe we should reword some descriptions so it's clear users need to sign in to see other commands? and maybe for signup to advertise too for users who install the CLI to play around but then realize they can't

@brianstrauch
Copy link
Member Author

@mtodzo Yeah, let me send a copy of the binary to Ethan before merging. And I like your signup idea; we could add that as a second suggestion if a non-cloud user tries to run a cloud command.

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.

None yet

2 participants