Group cluster create flags for readability #328

mumoshu · 2018-11-30T03:27:35Z

Hi! Thanks for maintaining an awesome project ☺️

Towards #298, I've tried to group create cluster flags as I think appropriate.

I've intentionally left the too-long descriptions as-is. I'll work on it in an another PR if you like.

eksctl create cluster -h now looks like:

Create a cluster

Usage:
  eksctl create cluster [flags]

Node Flags:
      --asg-access                enable iam policy dependency for cluster-autoscaler
      --full-ecr-access           enable full access to ECR
      --max-pods-per-node int     maximum number of pods per node (set automatically if unspecified)
      --node-ami string           Advanced use cases only. If 'static' is supplied (default) then eksctl will use static AMIs; if 'auto' is supplied then eksctl will automatically set the AMI based on region/instance type; if any other value is supplied it will override the AMI to use for the nodes. Use with extreme care. (default "static")
      --node-ami-family string    Advanced use cases only. If 'AmazonLinux2' is supplied (default), then eksctl will use the offical AWS EKS AMIs (Amazon Linux 2); if 'Ubuntu1804' is supplied, then eksctl will use the offical Canonical EKS AMIs (Ubuntu 18.04). (default "AmazonLinux2")
  -P, --node-private-networking   whether to make initial nodegroup networking private
  -t, --node-type string          node instance type (default "m5.large")
      --node-volume-size int      Node volume size (in GB)
  -N, --nodes int                 total number of nodes (desired capacity of ASG) (default 2)
  -M, --nodes-max int             maximum nodes in ASG (leave unset for a static nodegroup)
  -m, --nodes-min int             minimum nodes in ASG (leave unset for a static nodegroup)
      --ssh-access                control SSH access for nodes
      --ssh-public-key string     SSH public key to use for nodes (import from local path, or use existing EC2 key pair) (default "~/.ssh/id_rsa.pub")
      --storage-class             if true (default) then a default StorageClass of type gp2 provisioned by EBS will be created (default true)

Networking Flags:
      --vpc-cidr ipNet                 global CIDR to use for VPC (default 192.168.0.0/16)
      --vpc-from-kops-cluster string   re-use VPC from a given kops cluster
      --vpc-private-subnets strings    re-use private subnets of an existing VPC
      --vpc-public-subnets strings     re-use public subnets of an existing VPC
      --zones strings                  (auto-select if unspecified)

Stack Flags:
  -r, --region string         AWS region
      --tags stringToString   A list of KV pairs used to tag the AWS resources (e.g. "Owner=John Doe,Team=Some Team") (default [])

Other Flags:
      --auto-kubeconfig          save kubeconfig file by cluster name, e.g. "/Users/kuoka-yusuke/.kube/eksctl/clusters/adorable-monster-1543548069"
      --kubeconfig string        path to write kubeconfig (incompatible with --auto-kubeconfig) (default "/Users/kuoka-yusuke/.kube/config")
  -n, --name string              EKS cluster name (generated if unspecified, e.g. "adorable-monster-1543548069")
  -p, --profile string           AWS credentials profile to use (overrides the AWS_PROFILE environment variable)
      --set-kubeconfig-context   if true then current-context will be set in kubeconfig; if a context is already set then it will be overwritten (default true)
      --timeout duration         max wait time in any polling operations (default 20m0s)
      --write-kubeconfig         toggle writing of kubeconfig (default true)

Global Flags:
  -C, --color         toggle colorized logs (default true)
  -v, --verbose int   set log level, use 0 to silence, 4 for debugging and 5 for debugging with AWS debug logging (default 3)

`eksctl create cluster -h` now looks like: ``` Create a cluster Usage: eksctl create cluster [flags] Node Flags: --asg-access enable iam policy dependency for cluster-autoscaler --full-ecr-access enable full access to ECR --max-pods-per-node int maximum number of pods per node (set automatically if unspecified) --node-ami string Advanced use cases only. If 'static' is supplied (default) then eksctl will use static AMIs; if 'auto' is supplied then eksctl will automatically set the AMI based on region/instance type; if any other value is supplied it will override the AMI to use for the nodes. Use with extreme care. (default "static") --node-ami-family string Advanced use cases only. If 'AmazonLinux2' is supplied (default), then eksctl will use the offical AWS EKS AMIs (Amazon Linux 2); if 'Ubuntu1804' is supplied, then eksctl will use the offical Canonical EKS AMIs (Ubuntu 18.04). (default "AmazonLinux2") -P, --node-private-networking whether to make initial nodegroup networking private -t, --node-type string node instance type (default "m5.large") --node-volume-size int Node volume size (in GB) -N, --nodes int total number of nodes (desired capacity of ASG) (default 2) -M, --nodes-max int maximum nodes in ASG (leave unset for a static nodegroup) -m, --nodes-min int minimum nodes in ASG (leave unset for a static nodegroup) --ssh-access control SSH access for nodes --ssh-public-key string SSH public key to use for nodes (import from local path, or use existing EC2 key pair) (default "~/.ssh/id_rsa.pub") --storage-class if true (default) then a default StorageClass of type gp2 provisioned by EBS will be created (default true) Networking Flags: --vpc-cidr ipNet global CIDR to use for VPC (default 192.168.0.0/16) --vpc-from-kops-cluster string re-use VPC from a given kops cluster --vpc-private-subnets strings re-use private subnets of an existing VPC --vpc-public-subnets strings re-use public subnets of an existing VPC --zones strings (auto-select if unspecified) Stack Flags: -r, --region string AWS region --tags stringToString A list of KV pairs used to tag the AWS resources (e.g. "Owner=John Doe,Team=Some Team") (default []) Other Flags: --auto-kubeconfig save kubeconfig file by cluster name, e.g. "/Users/kuoka-yusuke/.kube/eksctl/clusters/adorable-monster-1543548069" --kubeconfig string path to write kubeconfig (incompatible with --auto-kubeconfig) (default "/Users/kuoka-yusuke/.kube/config") -n, --name string EKS cluster name (generated if unspecified, e.g. "adorable-monster-1543548069") -p, --profile string AWS credentials profile to use (overrides the AWS_PROFILE environment variable) --set-kubeconfig-context if true then current-context will be set in kubeconfig; if a context is already set then it will be overwritten (default true) --timeout duration max wait time in any polling operations (default 20m0s) --write-kubeconfig toggle writing of kubeconfig (default true) Global Flags: -C, --color toggle colorized logs (default true) -v, --verbose int set log level, use 0 to silence, 4 for debugging and 5 for debugging with AWS debug logging (default 3) ``` Ref eksctl-io#298

errordeveloper · 2018-11-30T04:49:02Z

It think this is really great, thanks a lot! I am still on vacation until next week, so shall review thoroughly when I am back in the office.

mumoshu · 2018-11-30T11:05:12Z

Thanks! Please enjoy your vacation 🍹

This is a successor of eksctl-io#281 but rebased onto the eksctl-io#328 The original credit goes to Richard Case <richard.case@outlook.com> Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

errordeveloper · 2018-12-06T13:44:36Z

pkg/ctl/create/cluster.go

+
+	// The usage template is based on the one bundled into cobra
+	// https://github.com/spf13/cobra/blob/1e58aa3361fd650121dceeedc399e7189c05674a/command.go#L397
+	origFlagUsages := `


Let's use https://github.com/lithammer/dedent here

Umm, the whole purpose of this seemingly redundant empty lines is to make necessary and enough space between the usage` and the flags section.

That is, if I just wrap it with Decent like origFlagUsages := Decent( .... ) what you get becomes:

Usage: eksctl create cluster [flags] General flags: -n, --name string EKS cluster name (generated if unspecified, e.g. "adorable-monster-1543548069") -r, --region string AWS region --zones strings (auto-select if unspecified) --tags stringToString A list of KV pairs used to tag the AWS resources (e.g. "Owner=John Doe,Team=Some Team") (default []) Initial nodegroup flags: --max-pods-per-node int maximum number of pods per node (set automatically if unspecified)

Probably this isn't your intention?

Sorry, I didn't notice that. The reason I was suggesting this is to avoid braking indentation, as currently the string has no indentation, and it's in a middle of a function, so it jumps straight at me when I look at it. Perhaps this is just my OCD :)

errordeveloper · 2018-12-06T13:45:40Z

pkg/ctl/create/cluster.go

 	return cmd
 }

+func groupFlagsInUsage(cmd *cobra.Command) {
+	// Group flags by their categories determined by name prefixes
+	groupToPatterns := map[string][]string{


Eventually would be nice to automate this, so we don't have to keep two copies, but I'm happy to merge this as is to start with!

errordeveloper · 2018-12-06T13:55:42Z

The main thing I would like to discuss here is grouping, the code LGTM.

So here is my take on the actual grouping:

Create a cluster

Usage:
  eksctl create cluster [flags]

General flags:
  -n, --name string           EKS cluster name (generated if unspecified, e.g. "adorable-monster-1543548069")
  -r, --region string         AWS region
      --zones strings         (auto-select if unspecified)
      --tags stringToString   A list of KV pairs used to tag the AWS resources (e.g. "Owner=John Doe,Team=Some Team") (default [])

Initial nodegroup flags:
      --max-pods-per-node int     maximum number of pods per node (set automatically if unspecified)
      --node-ami string           Advanced use cases only. If 'static' is supplied (default) then eksctl will use static AMIs; if 'auto' is supplied then eksctl will automatically set the AMI based on region/instance type; if any other value is supplied it will override the AMI to use for the nodes. Use with extreme care. (default "static")
      --node-ami-family string    Advanced use cases only. If 'AmazonLinux2' is supplied (default), then eksctl will use the offical AWS EKS AMIs (Amazon Linux 2); if 'Ubuntu1804' is supplied, then eksctl will use the offical Canonical EKS AMIs (Ubuntu 18.04). (default "AmazonLinux2")
  -P, --node-private-networking   whether to make initial nodegroup networking private
  -t, --node-type string          node instance type (default "m5.large")
      --node-volume-size int      Node volume size (in GB)
  -N, --nodes int                 total number of nodes (desired capacity of ASG) (default 2)
  -M, --nodes-max int             maximum nodes in ASG (leave unset for a static nodegroup)
  -m, --nodes-min int             minimum nodes in ASG (leave unset for a static nodegroup)
      --ssh-access                control SSH access for nodes
      --ssh-public-key string     SSH public key to use for nodes (import from local path, or use existing EC2 key pair) (default "~/.ssh/id_rsa.pub")

VPC networking:
      --vpc-cidr ipNet                 global CIDR to use for VPC (default 192.168.0.0/16)
      --vpc-from-kops-cluster string   re-use VPC from a given kops cluster
      --vpc-private-subnets strings    re-use private subnets of an existing VPC
      --vpc-public-subnets strings     re-use public subnets of an existing VPC

Flags for add-ons:
      --storage-class             if true (default) then a default StorageClass of type gp2 provisioned by EBS will be created (default true)
      --asg-access                enable iam policy dependency for cluster-autoscaler
      --full-ecr-access           enable full access to ECR

Flags for AWS client:
  -p, --profile string           AWS credentials profile to use (overrides the AWS_PROFILE environment variable)
      --timeout duration         max wait time in any polling operations (default 20m0s)

Flags for kubeconfig:
      --auto-kubeconfig          save kubeconfig file by cluster name, e.g. "/Users/kuoka-yusuke/.kube/eksctl/clusters/adorable-monster-1543548069"
      --kubeconfig string        path to write kubeconfig (incompatible with --auto-kubeconfig) (default "/Users/kuoka-yusuke/.kube/config")
      --set-kubeconfig-context   if true then current-context will be set in kubeconfig; if a context is already set then it will be overwritten (default true)
      --write-kubeconfig         toggle writing of kubeconfig (default true)

Other Flags:
  -C, --color         toggle colorized logs (default true)
  -v, --verbose int   set log level, use 0 to silence, 4 for debugging and 5 for debugging with AWS debug logging (default 3)

Eventually, it'd be nice to also enforce ordering, but for now I think we can live with alphabetical order.

mumoshu · 2018-12-07T06:11:52Z

@errordeveloper Thanks for your review!

I slightly prefer --zones to reside in `VPC networking. But I agree with your idea in general!

Would you mind if I contribute an another PR to address your comment, if this looks ok implementation-wise?

Doing so would make me focus on improving things incrementally, without emptying my time due to repetitive rebases across pull requests... 😃

mumoshu · 2018-12-07T06:12:38Z

Perhaps you want VPC networking flags: rather than VPC networking:, and Add-ons flags rather than Flags for add-ons and so on for consistency?

errordeveloper · 2018-12-07T08:39:11Z

Would you mind if I contribute an another PR to address your comment, if this looks ok implementation-wise?

Sure, happy to merge, and follow up with a PR of my own, so we can cut a release today.

Perhaps you want VPC networking flags: rather than VPC networking:, and Add-ons flags rather than Flags for add-ons and so on for consistency?

Yes, probably better. In that version I was just playing with different variations, I wasn't sure if word flags has to be there or not.

errordeveloper · 2018-12-07T08:42:34Z

Regarding --zones, it can also be seen as a nodegroup-specific one. We might actually want to break it into --cluster-zones and --nodegroup-zones at somepoint, but that's a separate topic.

mumoshu · 2018-12-07T09:03:44Z

Thanks!

In that version I was just playing with different variations, I wasn't sure if word flags has to be there or not.

For consistency, having "flags" at the end seems good. I once considered options like VPC networking options. But I discarded the idea because then we have to differentiate between options and must-have's. I also tried config like VPC networking config, but discarded it as well because I had no strong opinion to prefer it over just flags which is already used widely for flags.

break it into --cluster-zones and --nodegroup-zones at somepoint

Sounds good 👍

Add node selector for linux node only

mumoshu mentioned this pull request Nov 30, 2018

User-defined flag sets spf13/cobra#272

Closed

mumoshu mentioned this pull request Nov 30, 2018

feat: CloudFormation Service Role support #330

Merged

mumoshu mentioned this pull request Dec 3, 2018

feat: Nodegroup as a resource #332

Closed

7 tasks

Merge branch 'master' into improve-help

c4b764e

errordeveloper reviewed Dec 6, 2018

View reviewed changes

errordeveloper approved these changes Dec 7, 2018

View reviewed changes

errordeveloper merged commit 8e59f85 into eksctl-io:master Dec 7, 2018

mumoshu deleted the improve-help branch December 7, 2018 12:28

errordeveloper mentioned this pull request Dec 8, 2018

Automate flag grouping with and abstraction #342

Merged

2 tasks

torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022

Merge pull request eksctl-io#328 from leakingtapan/windows

f5d0c0d

Add node selector for linux node only

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group cluster create flags for readability #328

Group cluster create flags for readability #328

mumoshu commented Nov 30, 2018

errordeveloper commented Nov 30, 2018

mumoshu commented Nov 30, 2018

errordeveloper Dec 6, 2018

mumoshu Dec 7, 2018

errordeveloper Dec 7, 2018

errordeveloper Dec 6, 2018

errordeveloper commented Dec 6, 2018 •

edited

Loading

mumoshu commented Dec 7, 2018

mumoshu commented Dec 7, 2018

errordeveloper commented Dec 7, 2018

errordeveloper commented Dec 7, 2018

mumoshu commented Dec 7, 2018

Group cluster create flags for readability #328

Group cluster create flags for readability #328

Conversation

mumoshu commented Nov 30, 2018

errordeveloper commented Nov 30, 2018

mumoshu commented Nov 30, 2018

errordeveloper Dec 6, 2018

Choose a reason for hiding this comment

mumoshu Dec 7, 2018

Choose a reason for hiding this comment

errordeveloper Dec 7, 2018

Choose a reason for hiding this comment

errordeveloper Dec 6, 2018

Choose a reason for hiding this comment

errordeveloper commented Dec 6, 2018 • edited Loading

mumoshu commented Dec 7, 2018

mumoshu commented Dec 7, 2018

errordeveloper commented Dec 7, 2018

errordeveloper commented Dec 7, 2018

mumoshu commented Dec 7, 2018

errordeveloper commented Dec 6, 2018 •

edited

Loading