adding params list #1029

kensipe · 2019-11-05T14:19:47Z

Provides a table list of params for varies reasons. It will work for local or repo packages.
It can be useful for the operator developer... but likely it is most useful to the user of the packages for the follow reasons:

lists all params and their defaults
list all required params (params that are required but don't have defaults)

Currently there are several flags with their reasons:

--names-only displays only the param names. the uitables is very limiting and the column widths sometimes truncate the names. names-only doesn't truncate.
--required displays only column names as well... but the list is limited to only the params which are required but do not have a default. You can think of required differently here... here it is user required to install this operator.
--descriptions the default table will print name, required and default and no desc (which can be length). specifying --descriptions adds the desc column.

** several operators were used for building this feature out... Kafka was the most interesting based on number of params and length of param names and descriptions.

Example output:

default view

go run cmd/kubectl-kudo/main.go package params list ~/repo/flink-0.1.1.tgz 
Name                      	Default                            	Required
flink_jobmanager_replicas 	1                                  	true    
flink_storage_accessmodes 	ReadWriteOnce                      	true    
flink_taskmanager_replicas	2                                  	true    
high_availability         	NONE                               	true    
zookeeper_path            	/flink                             	true    
zookeeper_url             	zk-zk-0.zk-hs:2181,zk-zk-1.zk-hs...	true

required view

go run cmd/kubectl-kudo/main.go package params list ~/repo/flink-0.1.1.tgz --required
no required parameters without default values found

names-only

go run cmd/kubectl-kudo/main.go package params list ~/repo/flink-0.1.1.tgz --names-only
Name                      
flink_jobmanager_replicas 
flink_storage_accessmodes 
flink_taskmanager_replicas
high_availability         
zookeeper_path            
zookeeper_url

description view

go run cmd/kubectl-kudo/main.go package params list ~/repo/flink-0.1.1.tgz -d
Name                      	Default                            	Required	Descriptions                       
flink_jobmanager_replicas 	1                                  	true    	Number of job managers to run      
flink_storage_accessmodes 	ReadWriteOnce                      	true    	Defines the access modes for       
                          	                                   	        	Persistent Volume Claims e.g., can 
                          	                                   	        	be mounted once read/write or many 
                          	                                   	        	times read-only                    
flink_taskmanager_replicas	2                                  	true    	Number of task managers to run     
high_availability         	NONE                               	true    	Defines high-availability mode used
                          	                                   	        	for the cluster execution. To      
                          	                                   	        	enable high-availability, set this 
                          	                                   	        	mode to "ZOOKEEPER" or specify FQN 
                          	                                   	        	of factory class.                  
zookeeper_path            	/flink                             	true    	Path to store Flink data in        
                          	                                   	        	Zookeeper                          
zookeeper_url             	zk-zk-0.zk-hs:2181,zk-zk-1.zk-hs...	true    	Connection information for         
                          	                                   	        	Zookeeper

It also supports pulling from a repo... but the community repo is not compat with kudo master right now.

go run cmd/kubectl-kudo/main.go package params list flink --repo community
# will print default from the community repo download

mpereira · 2019-11-05T15:10:55Z

This looks great, thanks @kensipe.

As an operator developer I can see two different but similar parameter-related sets of operations: on packages and on instances.

For listing package parameters it would probably make sense to specify a package version, since parameters will potentially change across versions. Would it make sense to interact in terms of operatorversions in the CLI?

For listing instance parameters it would make sense to show the actual current parameter value as well, and differently from listing package parameters, the version is implied by the instance itself.

I was trying to think of a CLI UX that is consistent with kubectl and thought of a rough sketch looking something like this (in the same sentiment as Alena's suggestion):

(Brackets signify optional arguments).

Listing a package's parameters:

$ kubectl kudo get \
    --package=cassandra \
    --all-parameters \
    [--package-version=0.1.0]
Name          Default          Required          Description
<...>         <...>            <...>             <...>

Listing an instances's parameters (note the "Value" column):

$ kubectl kudo get cassandra \
    --namespace=cassandra-ns \
    --all-parameters
Name          Value          Default          Required          Description
<...>         <...>          <...>            <...>             <...>

Showing an instances's NODE_COUNT parameter:

$ kubectl kudo get cassandra \
    --namespace=cassandra-ns \
    --parameter=NODE_COUNT
Name          Value          Default          Required          Description
NODE_COUNT    <...>          <...>            <...>             <...>

All of these commands also possibly accepting --names-only, --required and --descriptions. To be honest, initially, I don't see too much value in these flags given that the output should be easily transformed with shell.

mpereira · 2019-11-05T15:17:18Z

For context, the package/instance distinction exists in DC/OS services. A service operator can both get the package's parameters (with default, required, description), and a service instance's parameters (with the actual values).

kensipe · 2019-11-05T15:18:11Z

I think I prefer to use of describe for things out of the repo... and get for things that are instances in the cluster.

kensipe · 2019-11-05T15:21:04Z

The PR desc provides the reasons for the flags... there is definitely value in --required

mpereira · 2019-11-05T15:28:17Z

I think I prefer to use of describe for things out of the repo... and get for things that are instances in the cluster.

Makes sense!

kensipe · 2019-11-05T21:47:08Z

@mpereira I really like you proposal... however it is for a completely different feature. It seems to be focused on getting params from instances. This PR is for operators (which are not installed yet). I will write up an issue to make sure we don't miss this... it's a great idea.

This PR now, reorgs package under operator cmd to become kudo operator package than adds a params list which in full looks like kudo operator params list. The focus of this PR is to aid a kudo user... a user that needs to install an operator but doesn't know or understand the params they are able to change or those that must be provided.

kensipe · 2019-11-05T21:55:15Z

New feature details here: #1030

The review of this PR should be focused on operators (to be installed day 1) not instances in day 2.

alenkacz

I like the direction, I think there's not much left to do to land this once we agree on the UX of the commands (I had few suggestions).

alenkacz · 2019-11-06T12:19:17Z

pkg/kudoctl/cmd/operator.go

+provide a list of parameters from a remote operator given a url or repository along with the name and version.
+`
+
+const operatorExamples = `  kubectl kudo operator package [operator folder]


wondering why did you go with the extra operator level here. To me it's kind of redundant... Do you think that following would not be enough?

kudo package params
kudo package verify
kudo package info

or something along those lines

the use of package is challenging if we use package to package an operator. It works if we perhaps add a package create.... which is currently operator package

prior to this PR... kudo package was an action... it took an operator folder and made a package. This PR and feature is provide info back... it seems slightly incongruent... but the issue is package [folder] and package params [xyz] doesn't work together. If we are stuck on putting it all under package... than we would need a package create [folder] for the current package feature.

alenkacz · 2019-11-06T12:20:57Z

pkg/kudoctl/cmd/operator_params_list.go

+	cmd := &cobra.Command{
+		Use:     "list [operator]",
+		Short:   "List operator parameters",
+		Example: "  kubectl kudo params list",


I think this is no longer true

doesn't match the use either right?

lets get agreement on the UX before we critic the examples...

alenkacz · 2019-11-06T12:21:18Z

pkg/kudoctl/cmd/operator_params_list.go

+		Short:   "List operator parameters",
+		Example: "  kubectl kudo params list",
+		RunE: func(cmd *cobra.Command, args []string) error {
+			//list.home = Settings.Home


do we need this? Otherwise please remove it...

this is removed in the merging in PR for verify

alenkacz · 2019-11-06T12:25:52Z

pkg/kudoctl/cmd/operator_params_list.go

+		return nil
+	}
+	// names only
+	if cmd.namesOnly {


so this is named namesOnly but you can still combine it with --descriptions which means you get also other? I guess those two should be mutually exclusive?

What do you think about moving this closer to kubectl. That mean having a pre-defined set of columns that are printed by default (some basic ones) and then supporting -o custom-columns=xxx as kubectl does. I really like that design...

i like that, but maybe not for this PR? this gets me thinking too, we should probably specify additionalPrintColumns for our CRDs (also not part of this PR)

(the custom columns that is - agreed on making them mutually exclusive today)

then let's have a predefined set of columns in this PR and add the ability to customize it in any way in the next one... (removing flags is also breaking change :))

alenkacz · 2019-11-06T12:26:53Z

pkg/kudoctl/cmd/operator_verify.go

@@ -0,0 +1,38 @@
+package cmd


let's many separate this into a different PR?

gerred

I have nothing valuable to add on top of Alena's review, mostly echoing her. Overall everything looks good to me.

mpereira · 2019-11-06T15:01:13Z

@kensipe agreed wrt to "instance params" being a whole different feature. Thanks for creating the issue for it!

kensipe requested review from alenkacz, gerred and zen-dog as code owners November 5, 2019 14:19

kensipe force-pushed the ken/params-list branch from 9cce73a to 9097136 Compare November 5, 2019 15:27

kensipe mentioned this pull request Nov 5, 2019

List Parameters of an Instance #1030

Open

alenkacz reviewed Nov 6, 2019

View reviewed changes

gerred reviewed Nov 6, 2019

View reviewed changes

kensipe added 5 commits November 6, 2019 10:49

adding params list

25163ef

changes to cmd layout for packages and params list

3547f5f

changes to cmd layout for packages and params list

e90dc0d

grammar fixes

5eddebd

adding verification to operators around params (#1033)

fbeb3ff

kensipe force-pushed the ken/params-list branch from 1f908fe to fbeb3ff Compare November 6, 2019 16:49

kensipe added 2 commits November 6, 2019 12:50

restructured parms and verification around package

5d0759e

cli examples and docs updated to new structure

08dc758

gerred self-requested a review November 6, 2019 19:03

gerred approved these changes Nov 6, 2019

View reviewed changes

kensipe merged commit e94e801 into master Nov 6, 2019

kensipe deleted the ken/params-list branch November 6, 2019 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding params list #1029

adding params list #1029

kensipe commented Nov 5, 2019 •

edited

Loading

mpereira commented Nov 5, 2019

mpereira commented Nov 5, 2019 •

edited

Loading

kensipe commented Nov 5, 2019

kensipe commented Nov 5, 2019

mpereira commented Nov 5, 2019

kensipe commented Nov 5, 2019

kensipe commented Nov 5, 2019

alenkacz left a comment

alenkacz Nov 6, 2019

kensipe Nov 6, 2019

kensipe Nov 6, 2019

alenkacz Nov 6, 2019

gerred Nov 6, 2019

kensipe Nov 6, 2019

alenkacz Nov 6, 2019

kensipe Nov 6, 2019

alenkacz Nov 6, 2019

gerred Nov 6, 2019

gerred Nov 6, 2019

alenkacz Nov 6, 2019

alenkacz Nov 6, 2019

gerred Nov 6, 2019

gerred left a comment

mpereira commented Nov 6, 2019

adding params list #1029

adding params list #1029

Conversation

kensipe commented Nov 5, 2019 • edited Loading

mpereira commented Nov 5, 2019

mpereira commented Nov 5, 2019 • edited Loading

kensipe commented Nov 5, 2019

kensipe commented Nov 5, 2019

mpereira commented Nov 5, 2019

kensipe commented Nov 5, 2019

kensipe commented Nov 5, 2019

alenkacz 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

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

gerred left a comment

Choose a reason for hiding this comment

mpereira commented Nov 6, 2019

kensipe commented Nov 5, 2019 •

edited

Loading

mpereira commented Nov 5, 2019 •

edited

Loading