-
Notifications
You must be signed in to change notification settings - Fork 69
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
Update Cluster autoscaler operator to consume machine.openshift.io #35
Update Cluster autoscaler operator to consume machine.openshift.io #35
Conversation
/hold |
Both of these need to become |
Thanks for the catch. |
8839627
to
9f8bfda
Compare
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
Looks perfect. Should be good to merge if the tests pass.
39f0bee
to
324ff0d
Compare
324ff0d
to
8db6ac0
Compare
/lgtm |
/test e2e-aws |
1 similar comment
/test e2e-aws |
/retest |
/hold cancel |
{Group: "cluster.k8s.io", Version: "v1alpha1", Kind: "MachineDeployment"}, | ||
{Group: "cluster.k8s.io", Version: "v1alpha1", Kind: "MachineSet"}, | ||
{Group: "machine.openshift.io", Version: "v1beta1", Kind: "MachineDeployment"}, |
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.
Not a blocker, this would belong to a different PR
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.
We need to decide quickly. this will merge RSN if the tests pass as we have LGTM and APPROVE.
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.
What belongs to a different PR? Just adding MachineDeployments
? Or am I misunderstanding?
There's no harm in adding the deployments here.
/test e2e-aws-operator Tests are passing for me locally. Not sure what was up on the last run. |
You could either pin the autoscaler image on cao (we'd need to keep both groups here https://github.com/openshift/cluster-autoscaler-operator/pull/35/files#diff-defb693cda2c7b43fb8c51ddc5cf70deL33) or just disable the deployment test as part of this PR, then re enable it after openshift/kubernetes-autoscaler#29 gets merged |
test/e2e/main.go
Outdated
return err | ||
} | ||
glog.Info("PASS: ExpectClusterAutoscalerAvailable") | ||
// TODO: Disabled temporarily |
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.
can you add some context on the TODO on why disabling?
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.
Should be good now.
a356e5c
to
a9fe0c7
Compare
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
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.