-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Cluster-autoscaler: cloud provider interface implementation for GCE #1323
Conversation
f689c13
to
cd74019
Compare
|
||
// AddNodeGroup adds node group defined in string spec. Format: | ||
// minNodes:maxNodes:migUrl | ||
func (gce *GceCloudProvider) AddNodeGroup(spec string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be passed in a constructor? Do you have any use case for adding NodeGroup in flight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
// Url builds GCE url for the MIG. | ||
func (mig *Mig) Url() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this? It's equivalent to Id().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
94e886a
to
4d22a72
Compare
// IncreaseSize increases Mig size | ||
func (mig *Mig) IncreaseSize(delta int) error { | ||
if delta <= 0 { | ||
return fmt.Errorf("size increase must be positive") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so why not making delta uint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suppose that someone has -1 in an int variable. Stupid int->uint conversion would make it some huge number that added to the current size couple lines below would effectively mean -1. In this way -1 would pass through the checks.
fe08aef
to
678951d
Compare
LGTM |
} | ||
|
||
// Belongs retruns true if the given node belongs to the NodeGroup. | ||
func (mig *Mig) Belongs(node *kube_api.Node) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwielgus Does this method need to be exported?
…provider Cluster-autoscaler: cloud provider interface implementation for GCE
…provider Cluster-autoscaler: cloud provider interface implementation for GCE
cc: @piosz @fgrzadkowski @jszczepkowski
Part of #1311