-
Notifications
You must be signed in to change notification settings - Fork 33
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
updated chart objects to have labels #34
Conversation
If there are any problems or questions please feel free to ask or bring up to our rancher support team who can talk with my team. |
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.
Looks pretty good! A few things
- I added comments about the CSI/CPI version updates
- If the image version updates here are necessary, please update the tests and run
make ci
locally to make sure the tests and lint passes - build CI error in
service-account.yaml
charts/rancher-vsphere-csi/templates/node/windows-daemonset.yaml
Outdated
Show resolved
Hide resolved
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.
Nit: the commits need to be squashed into one.
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.
@jacobfra The constraint images are outdated again in values.yaml so just rebase with main, -f push and then this should be good to merge
bf7186a
to
eea1792
Compare
eea1792
to
3b22406
Compare
I had to rebase and force push to get back to main commit and did the change required. |
This is over a year old and is not passing go test suite. going to create a new branch 'add-labels-tocharts2' and resubmit new pull from recently pull main. |
Pull Request Checklist
Types of Change
Linked Issues
#33
Additional Notes
tested in private rancher cluster with charts installed successfully.
After the PR is merged
Once the PR is merged, typically upon a new release, please post an announcement in the following channels: