CLI Unification: Root Command #917

brianstrauch · 2021-07-07T23:32:33Z

Checklist

[CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
- yes: ok
Did you add/update any commands that accept secrets as args/flags?
- no: ok

What

The root command no longer requires cliName, which was previously passed in as a linker flag (-ldflags "-X main.cliName=confluent")
The CLI will not run if there are any errors while loading the config file
When user isn't logged in, only commands which can exist for cloud and on-prem can be run
After login, based on IsCloud() and IsOnPrem(), show the commands available in the user's current context.

At this point, the confluent binary is unified! Future PRs will unify individual commands.

Plan of action: https://confluentinc.atlassian.net/wiki/spaces/Foundations/pages/2423849814/1-Pager+CLI+Unification+Implementation+Details

Test & Review

New unit and integration tests based on showing appropriate commands

Open questions / Follow ups

Should the confluent local command be always shown, since it doesn't require login? Are there any other commands that don't require a login that I'm missing?

* unify config command * unified config file, IsCloud and IsOnPrem * consolidate ccloud hostnames var * finish config unification and test * add golden files * check for test url second * add back missing return * support all cpdev platforms * trim trailing slash from platform names, and use contains * revert trim trailing slash * fix error

mtodzo

looks awesome !

cmd/confluent/main.go

internal/cmd/command.go

mtodzo · 2021-07-09T00:30:36Z

internal/cmd/command.go

+	// TODO: Remove once unification is complete
+	cliName := "confluent"
+	if cfg.IsCloud() {
+		cliName = "ccloud"


is this meant to be deleted?

Yes, the config file is loaded in the main function now. The CLI needs a correctly formatted config file in order to work, since it shows a certain subset of commands based on the contents of the config file.

mtodzo · 2021-07-09T00:46:22Z

cmd/confluent/main.go

 	isTest  = "false"
 )

 func main() {
-	viper.AutomaticEnv()
+	cfg, err := cmd.LoadConfig()


do we need to fail immediately because you need the config to tell if it's ccloud or confluent?

i forget if that was a field added to the config .. or if you're just parsing the url from the context name but either way

Yep exactly. We're using the --url flag to determine whether we should log in to cloud or on-prem, and then once we log in and the context gets stored, we use the current context's platform name (a config field) to decide between cloud and on-prem

internal/cmd/login/command.go

mtodzo · 2021-07-09T00:56:27Z

internal/pkg/cmd/prerunner.go

@@ -44,7 +45,6 @@ const DoNotTrack = "do-not-track-analytics"
 // PreRun is the standard PreRunner implementation
 type PreRun struct {
 	Config                  *v3.Config
-	ConfigLoadingError      error


the part of me that doesn't love how coupled the CLI is with the config wants to keep this (i.e. not fail immediately on config load err) ... i.e. confluent kafka topic list could work if you have the login env vars set and passed a --cluster and --environment value

Oh interesting... didn't know that was a thing you could do. I'll have to think about this some more but I wonder if that's something we want people to be able to do in the first place; like, why can kafka commands bypass the login command, but not other commands.

Ok, I've thought about this a bit and I want to make a proper config a hard requirement. If a user changed their config file somehow and it broke, they would see a limited subset of the commands and get no help with how to fix it.

For example, if a user had the intention of using the CLI for listing their Kafka clusters, and the kafka command wasn't showing up (even though they thought they had a working config file) they'd be very confused.

i think this makes sense .. and i don't think we've seen any customer feedback around not wanting a config file but cc: @DABH since we've discussed not requiring a config file

@DABH the tldr is that the config is now a hard requirement (fail immediately if it doesn't load) since the config is needed to determine whether the user is logged into ccloud or on-prem

Just want to clarify that if there's no config, then it shows the default set of commands that don't require authentication (login, logout, completion, config, help, local, update, version).

It only fails if the config exists and is broken in some way.

The only thing this doesn't support (and maybe should?) is running the commands mentioned above with a broken config. Although... a bunch of the commands listed above read from or write to the config (config, login, logout, update), so they would fail anyway.

At the end of the day, it looks like the only (useful) functionality changing this would enable is being able to run completion, help, local, and version with a broken config.

I actually think we would like to be able to run without a config file. The use case is multiple instances of the CLI being used on the same machine/user at the same time, e.g. on a CI machine where multiple jobs run simultaneously. In fact, we've advertised this capability as "stateless mode" -- you can just pass flags for whatever credentials/cluster/env you need etc., and assuming there are valid credentials (env vars / netrc / etc.), the command will work, regardless of what's in the config file, if the config file is corrupted, etc.

I don't think we really want to totally break this functionality. But I do see the problem with needing to know what version of a command to run. So maybe we change things a bit... e.g. we could require an extra flag like "--context"? But that would indeed require at least having a config file. Can we think about this a bit more?

internal/pkg/config/v3/config.go

internal/pkg/utils/utils.go

test/fixtures/output/help/help-no-context.golden

test/fixtures/output/version/version-flag.golden

mtodzo · 2021-07-15T16:58:01Z

cmd/lint/main.go

+			CurrentContext: "no context",
+		},
+		{
+			Contexts:       map[string]*v3.Context{"cloud": {PlatformName: testserver.TestCloudURL.String()}},


When does this run, during make lint-go? if so im not sure the test servers will have been started so testserver.TestCloudURL might be empty

make lint-go runs golangci-lint. This is the main file for make lint-cli, which lints command descriptions/flags. There are three possible subsets of commands that need to be linted, so we need to pass in three configs.

testserver.TestCloudURL.String() is a constant, so this works fine! But... now that you mention it, it would be cleaner to pass in confluent.cloud to the second config, because they're not technically tests.

DABH · 2021-07-15T21:57:58Z

Should the confluent local command be always shown, since it doesn't require login? Are there any other commands that don't require a login that I'm missing?

Yeah, I think this is ok... you do need to have CP downloaded in order to use these commands (confluent local doesn't download CP for you), but, someone who doesn't have a CP installation with MDS setup (i.e. can't login to CP) might still want to run local commands.

DABH · 2021-07-16T05:31:33Z

@brianstrauch Jenkins doesn't like this:
Error: unknown command "secret" for "confluent"\nRun \'confluent --help\' for usage.\
So there might be a bit of extra work to do here :|

brianstrauch and others added 15 commits July 1, 2021 19:04

unify config command

5d58f3d

unified config file, IsCloud and IsOnPrem

061202c

consolidate ccloud hostnames var

c529ea9

finish config unification and test

44b69b2

add golden files

6c000c9

check for test url second

c3cd7d5

add back missing return

4cd08a3

fix merge conflicts

cbe8d2e

fix merge conflicts

6f896d3

unify root command

f8c43b6

unified config file, IsCloud and IsOnPrem

7d80078

finish config unification and test

312f7e8

unify root command

183ffd4

Merge branch 'main' into unify-confluent

ae50ac5

brianstrauch requested a review from a team as a code owner July 7, 2021 23:32

brianstrauch added 5 commits July 7, 2021 18:40

fix lint

dbd59c8

remove dead code

e498408

add back accidentally deleted files

d2e854b

fix unit test for windows

74455d1

trigger build

b5a4297

mtodzo reviewed Jul 9, 2021

View reviewed changes

brianstrauch added 6 commits July 9, 2021 09:12

move automaticenv back

82ddf67

change help command description based on context

e513791

lint no-context commands

f33bba3

move local and kafka to no-context section

119aca3

add suggestion to long description

6a8f82e

hide local command if windows

6e826ad

mtodzo self-requested a review July 15, 2021 16:40

mtodzo approved these changes Jul 15, 2021

View reviewed changes

simplify configs in linter

4f56502

always show secret command

d3014ca

DABH approved these changes Jul 20, 2021

View reviewed changes

brianstrauch merged commit 90440c2 into main Jul 20, 2021

brianstrauch deleted the unify-confluent branch July 20, 2021 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI Unification: Root Command #917

CLI Unification: Root Command #917

brianstrauch commented Jul 7, 2021 •

edited

Loading

mtodzo left a comment

mtodzo Jul 9, 2021

brianstrauch Jul 9, 2021

mtodzo Jul 9, 2021

mtodzo Jul 9, 2021

brianstrauch Jul 9, 2021

mtodzo Jul 9, 2021

brianstrauch Jul 9, 2021

brianstrauch Jul 9, 2021

mtodzo Jul 13, 2021

mtodzo Jul 13, 2021

brianstrauch Jul 13, 2021 •

edited

Loading

DABH Jul 16, 2021

mtodzo Jul 15, 2021

brianstrauch Jul 15, 2021

DABH commented Jul 15, 2021

DABH commented Jul 16, 2021

CLI Unification: Root Command #917

CLI Unification: Root Command #917

Conversation

brianstrauch commented Jul 7, 2021 • edited Loading

Checklist

What

Test & Review

Open questions / Follow ups

mtodzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianstrauch Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DABH commented Jul 15, 2021

DABH commented Jul 16, 2021

brianstrauch commented Jul 7, 2021 •

edited

Loading

brianstrauch Jul 13, 2021 •

edited

Loading