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.

Sign up for GitHub

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

Group cluster create flags for readability #328

Merged
merged 2 commits into from
Dec 7, 2018
Merged

Group cluster create flags for readability #328

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb00133
Group cluster create flags for readability
mumoshu Nov 30, 2018
c4b764e
Merge branch 'master' into improve-help
mumoshu Dec 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions pkg/ctl/create/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@ package create
import (
"fmt"
"os"
"strings"
"unicode"

"github.com/kubicorn/kubicorn/pkg/logger"
"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"github.com/weaveworks/eksctl/pkg/ami"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/eks"
Expand Down Expand Up @@ -94,9 +97,58 @@ func createClusterCmd() *cobra.Command {

fs.BoolVarP(&ng.PrivateNetworking, "node-private-networking", "P", false, "whether to make initial nodegroup networking private")

groupFlagsInUsage(cmd)

return cmd
}

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

Choose a reason for hiding this comment

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

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!

"Node": {"node", "storage-class", "ssh", "max-pods-per-node", "full-ecr-access", "asg-access"},
"Networking": {"vpc", "zones",},
"Stack": {"region", "tags"},
"Other": {},
}
groups := []string{}
for k := range groupToPatterns {
groups = append(groups, k)
}
groupToFlagSet := make(map[string]*pflag.FlagSet)
for _, g := range groups {
groupToFlagSet[g] = pflag.NewFlagSet(g, /* Unused. Can be anythng. */ pflag.ContinueOnError)
}
cmd.LocalFlags().VisitAll(func(f *pflag.Flag) {
for _, g := range groups {
for _, p := range groupToPatterns[g] {
if strings.HasPrefix(f.Name, p) {
groupToFlagSet[g].AddFlag(f)
return
}
}
}
groupToFlagSet["Other"].AddFlag(f)
})

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)


Flags:
{{.LocalFlags.FlagUsages | trimTrailingWhitespaces}}`

altFlagUsages := ``
for _, g := range groups {
set := groupToFlagSet[g]
altFlagUsages += fmt.Sprintf(`

%s Flags:
%s`, g, strings.TrimRightFunc(set.FlagUsages(), unicode.IsSpace))
}

cmd.SetUsageTemplate(strings.Replace(cmd.UsageTemplate(), origFlagUsages, altFlagUsages, 1))
}

func doCreateCluster(p *api.ProviderConfig, cfg *api.ClusterConfig, ng *api.NodeGroup, nameArg string) error {
meta := cfg.Metadata
ctl := eks.New(p, cfg)
Expand Down