-
Notifications
You must be signed in to change notification settings - Fork 16
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.
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
Add wait for K8s resources #82
Add wait for K8s resources #82
Conversation
… add_k8s_nodepool_autoscalinglimits_property
* Unused (failed_)supended state removed * AutoScaling node counts must be pointer to uint32, api might return null as value. * Tests added
…toscalinglimits_property
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.
Needs rebase after #80 is merged
go.mod
Outdated
gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect | ||
gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776 // indirect | ||
k8s.io/apimachinery v0.18.6 |
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.
No, please don't import apimachinery here. I see you using one function from there, just re-implement it or write an equivalent function.
k8s.go
Outdated
"k8s.io/apimachinery/pkg/util/wait" | ||
) | ||
|
||
type KubernetesClusterNodePoolState 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.
Remove this type
k8s.go
Outdated
K8sStateTerminated KubernetesClusterNodePoolState = "TERMINATED" | ||
) | ||
|
||
type KubernetesNodeState 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.
Remove this type
k8s.go
Outdated
func (c *Client) WaitForKubernetesNodePoolState( | ||
clusterID, nodePoolID string, | ||
state KubernetesClusterNodePoolState, | ||
timeout, interval time.Duration) (err 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.
The error returned by this function should include which state was polled
k8s.go
Outdated
clusterID, nodePoolID string, | ||
state KubernetesClusterNodePoolState, | ||
timeout, interval time.Duration) (err error) { | ||
return wait.PollImmediate(interval, timeout, func() (done bool, err 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.
These named return values also are not needed
k8s.go
Outdated
if err != nil { | ||
return false, err | ||
} | ||
return np != nil && np.Metadata != nil && np.Metadata.State == string(state), err |
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.
Return nil
instead of err
here
k8s.go
Outdated
} | ||
|
||
func (c *Client) WaitForKubernetesClusterState( | ||
clusterID string, state KubernetesClusterNodePoolState, timeout, interval time.Duration) (err 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.
Same remarks as above
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.
Please run go mod tidy
and goimports
.
k8s.go
Outdated
}) | ||
return np != nil && np.Metadata != nil && np.Metadata.State == state, err | ||
}); err != nil { | ||
return fmt.Errorf("error waiting for nodepool state %s [%w]", state, err) |
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.
Please use : %w
for wraps
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.
LGTM
Bases on: #80