Adding support for node autoscaling #368
Conversation
cli/cmd/cluster_create.go
Outdated
| cmd.Flags().IntVar(&r.args.createClusterMinNodeCount, "min-nodes", int(-1), "Minimum Node count (only for EKS, AKS and GKE clusters)") | ||
| cmd.Flags().IntVar(&r.args.createClusterMaxNodeCount, "max-nodes", int(-1), "Maximum Node count (only for EKS, AKS and GKE clusters)") |
There was a problem hiding this comment.
I think you should explain the value of -1 in the description
pkg/kotsclient/cluster_create.go
Outdated
| Tags: opts.Tags, | ||
| } | ||
|
|
||
| if opts.MinNodeCount != -1 { |
There was a problem hiding this comment.
What if it equals -2? Should this instead be >= 0?
emosbaugh
left a comment
emosbaugh
left a comment
There was a problem hiding this comment.
im approving this but I thought id share an example of what i thought was an elegant solution in a mature cli. feel free to disregard the suggestion if you do not agree
cli/cmd/cluster_create.go
Outdated
| cmd.Flags().IntVar(&r.args.createClusterNodeCount, "nodes", int(1), "Node count") | ||
| cmd.Flags().IntVar(&r.args.createClusterMinNodeCount, "min-nodes", int(-1), "Minimum Node count (only for EKS, AKS and GKE clusters)") | ||
| cmd.Flags().IntVar(&r.args.createClusterMaxNodeCount, "max-nodes", int(-1), "Maximum Node count (only for EKS, AKS and GKE clusters)") | ||
| cmd.Flags().IntVar(&r.args.createClusterMinNodeCount, "min-nodes", int(-1), "Minimum Node count (only for EKS, AKS and GKE clusters). A negative value will be ignored.") |
There was a problem hiding this comment.
I particularly like how docker cli handles this in the run command
--memory-swappiness int Tune container memory swappiness (0 to 100) (default -1)
pkg/kotsclient/cluster_create.go
Outdated
| Tags: opts.Tags, | ||
| } | ||
|
|
||
| if opts.MinNodeCount >= 0 { |
There was a problem hiding this comment.
Sorry I think i steered you in the wrong direction here. This will unexpectedly drop a value if its invalid. I actually think you need to do some validation in the runners.createCluster function.
ideally what id like to see in that function is
if r.args.createClusterMinNodeCount >= 0 {
opts.MinNodeCount = r.args.createClusterMinNodeCount
} else if r.args.createClusterMinNodeCount != -1 {
return errors.New("invalid value for flag --min-node-count (valid values are ...)", )
}
this function should just omit the conditionals and set the opts regardless of value
pkg/kotsclient/cluster_create.go
Outdated
| MinNodes int `json:"min_node_count"` | ||
| MaxNodes int `json:"max_node_count"` |
There was a problem hiding this comment.
I believe these need to be pointers to ints otherwise they will be 0 if unset
cli/cmd/cluster_create.go
Outdated
| } | ||
| if r.args.createClusterMinNodeCount >= 0 { | ||
| opts.MinNodeCount = &r.args.createClusterMinNodeCount | ||
| } else if r.args.createClusterMinNodeCount != -1 { |
There was a problem hiding this comment.
Can this be a string input and the value parsed in code here? Then just check if it's empty sting to use default values.
|
|
jdewinne
force-pushed
the
joshd/sc-99717/nodegroups-autoscaling-support
branch
from
44e1449 to
5c452da
Compare
No description provided.