Skip to content
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

Expose flag to explicitly disable cloud-provider=external kubelet argument #60

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

anmazzotti
Copy link
Contributor

The coud-provider=external kubelet argument was hardcoded.

I did implement a flag to explicitly set it. Note that this will change the current behavior, as the default value for this new flag is false. If this is not visible, I can reverse the flag to keep the current behavior in place.

The same flag is also used to disable the embedded cloud controller (since the external one will be used)

Copy link
Contributor

@mkmik mkmik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to not introduce a backward incompatible change. Can we define this setting with a polarity that would have natural default that preserves backward compatibility?

@richardcase
Copy link
Collaborator

I agree with @mkmik , we need to preserve the existing behaviour which is to set the cloud provider to external.

Should we allow the user to set a value for the cloud-provider to something else (e.g. aws)? Or is this just above having an option to remove this?

@anmazzotti
Copy link
Contributor Author

Thank you @mkmik and @richardcase for the feedback.
I agree that is not that user friendly to change the behavior now.

I pushed one last commit to reverse the flag, so that cloud-provider=external will need to be explicitly disabled.

@anmazzotti anmazzotti changed the title Expose flag to explicitly set cloud-provider=external kubelet argument Expose flag to explicitly disable cloud-provider=external kubelet argument Oct 11, 2023
Copy link
Collaborator

@richardcase richardcase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@zawachte zawachte merged commit 4a936a9 into k3s-io:main Oct 12, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants