CLI Unification: kafka #936
Conversation
| c.AddCommand(NewAclCommandOnPrem(c.prerunner)) | ||
| c.AddCommand(NewTopicCommandOnPrem(c.prerunner)) |
There was a problem hiding this comment.
So these are going to be shown for ccloud users?
There was a problem hiding this comment.
No, if you're logged in to Confluent Cloud, you get the cloud version of those two commands (there's a return at the end of the above if)
There was a problem hiding this comment.
Ah got it - I understand these are shown pre-login since you can authenticate directly against KRest w/o MDS, but do you think it's a weird UX for these commands to show up for a ccloud user when they aren't logged in, but then go away once they do login?
There was a problem hiding this comment.
not that I have an alternative idea 😅
There was a problem hiding this comment.
No, they'll stick around when the user logs in! There's three cases:
- Not logged in: You get the on-prem command.
- Logged in with MDS: You get the on-prem command.
- Logged in to Cloud: You get the cloud command.
| @@ -164,6 +165,7 @@ func AuthenticatedConfigMock(params mockConfigParams) *Config { | |||
| MetricSink: nil, | |||
| Logger: log.New(), | |||
| }) | |||
| conf.IsTest = true | |||
There was a problem hiding this comment.
The IsCloud() function looks for a Confluent Cloud URL in your config. If IsTest is set, it can also look for a local Confluent Cloud URL (http://localhost:1024) which is what the integration tests are hardcoded to use.
I think it's possible that someone uses http://localhost:1024 as their on-prem MDS URL, so I decided it's better to be safe than sorry and only check for that URL when it's an integration test.
| c.serverCompleter.AddCommand(aclCmd) | ||
| c.serverCompleter.AddCommand(clusterCmd) | ||
| c.serverCompleter.AddCommand(groupCmd) | ||
| c.serverCompleter.AddCommand(groupCmd.lagCmd) |
There was a problem hiding this comment.
Did you manually test that these server completers still work? I know it sounds weird, but I remember back in the day that there was something sensitive about the order of when you added the commands and when you added the completers. But maybe it was just that you have to add the servercompleters after you add the commands (which would make sense).
There was a problem hiding this comment.
Yeah, there's a comment up above about that. Wouldn't hurt to double check though.
Order matters here. If we add to the server-side completer first then the command doesn't have a parent and that doesn't trigger completion.
There was a problem hiding this comment.
It turns out that Cobra has built in arg and flag completion... Wish we had gone down that route instead of creating an entire shell 😅
https://github.com/spf13/cobra/blob/master/shell_completions.md#dynamic-completion-of-nouns
There was a problem hiding this comment.
It honestly might be worth playing around with this and seeing how difficult it is to implement. I feel like the autocomplete stuff is something no one has a great understanding of which is leading to some technical debt (new commands not having the proper ServerComplete functions, etc).
Checklist
[CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
Did you add/update any commands that accept secrets as args/flags?
What
Unified the
kafkacommand. Notably, the on-premkafka aclandkafka topiccommands are "stateless" (they have a--urlflag, so they can be used without logging in). We show the on-prem version of those command when the user is not logged in, or logged in on-prem. Otherwise, we show the cloud version of those commands.Test & Review
Make sure integration tests can infer a cloud or on-prem context. All existing tests pass.