Skip to content

Add configEnabled values to toggle CM creation #108

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

Merged
merged 1 commit into from
Apr 17, 2025

Conversation

alexander-demicev
Copy link
Member

Pull Request Checklist

  • Any new images or tags consumed by charts has been added here
  • Chart version has been incremented (if necessary)
  • That helm lint and pack run successfully on the chart.
  • Deployment of the chart has been tested and verified that it functions as expected.
  • Changes to scripting or CI config have been tested to the best of your ability

Types of Change

Add configEnabled values to toggle CM creation, this allows users to pass their own. The change is similar to upstream chart feature https://github.com/kubernetes/cloud-provider-vsphere/blob/master/charts/vsphere-cpi/templates/configmap.yaml#L7

Linked Issues

Additional Notes

After the PR is merged

Once the PR is merged, typically upon a new release, the necessary teams will be notified via Slack hook to perform the RKE2 Charts and RKE2 changes. Any developer working on this issue is not responsible for updating RKE2 Charts or RKE2.

snasovich
snasovich previously approved these changes Apr 9, 2025
Copy link
Collaborator

@snasovich snasovich left a comment

Choose a reason for hiding this comment

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

The change looks straightforward enough and upgrade scenarios should be handled OK as well since configEnabled will not be passed explicitly (as it didn't exist on previous versions of the chart) and will then get is default true value (unless explicitly set to false).

Given limited usage of this field, especially in Rancher use cases, it's fine to not add it to questions.yaml.

@alexander-demicev alexander-demicev force-pushed the configmapvalue branch 2 times, most recently from c69e159 to 84576ae Compare April 11, 2025 12:27
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

LGTM!

Final nit: bump the version in Chart.yaml

Copy link
Member

@jiaqiluo jiaqiluo left a comment

Choose a reason for hiding this comment

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

LGTM.

@brandond brandond merged commit 3d70705 into rancher:main Apr 17, 2025
1 check passed
@alexander-demicev alexander-demicev deleted the configmapvalue branch April 17, 2025 10:06
@furkatgofurov7
Copy link

@brandond @jiaqiluo hey, is this PR made to the new release of vsphere-chart or upcoming one you could mind telling when? I was not able to find the releases on this repo nor any other repo which publishes it. Thanks in advance!

@brandond
Copy link
Member

We don't do "releases" on this repo.

@furkatgofurov7
Copy link

We don't do "releases" on this repo.

Sorry if I was not clear. I was wondering where the chart is/will be published, including the changes in this PR

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.

5 participants