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

updated chart objects to have labels #34

Closed

Conversation

jacobfra
Copy link
Contributor

Pull Request Checklist

  • Adding a common label for both charts with a helper function
  • Adding the ability to have pod labels

Types of Change

  • updated helm charts

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:

  • #discuss-rancher-feature-charts
  • #discuss-rancher-feature-vsphere
  • #discuss-rancher-k3s-rke2

@jacobfra
Copy link
Contributor Author

jacobfra commented Oct 24, 2022

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.

Copy link
Contributor

@a-blender a-blender left a 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

@jiaqiluo jiaqiluo requested a review from a-blender November 2, 2022 16:33
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.

Nit: the commits need to be squashed into one.

@jiaqiluo jiaqiluo self-requested a review January 26, 2023 22:01
jiaqiluo
jiaqiluo previously approved these changes Jan 26, 2023
Copy link
Contributor

@a-blender a-blender left a 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

@jacobfra jacobfra force-pushed the add-labels-tocharts branch from eea1792 to 3b22406 Compare January 27, 2023 17:35
@jacobfra
Copy link
Contributor Author

@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

I had to rebase and force push to get back to main commit and did the change required.

@jacobfra
Copy link
Contributor Author

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.

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.

3 participants