-
Notifications
You must be signed in to change notification settings - Fork 97
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
Scale down of aws-custom-route-controller #634
Scale down of aws-custom-route-controller #634
Conversation
@elchead Labels area/todo, kind/todo do not exist. |
@elchead Thank you for your contribution. |
Thank you @elchead for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
e2e tested |
/invite @ialidzhikov @MartinWeindel |
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.
Thanks, looks good to me.
I have only one small change request. There is another test case named "should return correct control plane chart values (k8s >= 1.18)" (valuesprovider_test.go, line 296ff). Can you check here for replicas = 0?
/reviewed ok-to-test |
@MartinWeindel i am doing this already see l.257 in the definition of |
Didn't know that there was a PR open on this, just discovered it. Did the same thing in my code to remove the overlay but also checked if the
Do you prefer to enable the aws route controller always or only for clusters which have the UseCustomRouteController field specified? |
@DockToFuture AFAIK, the problem is that without setting the Ideally, we would be able to discern if the chart had already been applied at least once (hence there was a CRC running at some point) and only then apply the replicas, but I think this complicates the flow. Since we want to support disabling CRC and effectively setting replicas to 0 (since we don't delete the deployment), why not have only this case to care about. |
Sure without setting the enabled field the chart won't be applied. If we want to keep it like this im fine with it. Can we get it merged then I will rebase my PR on this. |
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
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
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
How to categorize this PR?
/area control-plane
/kind bug
/platform aws
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #628
Special notes for your reviewer:
Release note: