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

Support pulling crds from an upstream #129

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

joshmeranda
Copy link
Contributor

Problem

While updating the upstream for rancher-monitoring chart, I found that the crd generation code is unable to work with the structure of kube-prometheus-stack. The charts-build-scripts expect crds to be stored in a crds directory at the chart root directory; however, kube-prometheus-stack has moved its crds into a subchart. This causes several problems for crd generation:

  1. Since we clear the charts directory when loading dependencies, the upstream crd subchart is removed before the crd is generated, so we cannot simply support a user specified location to look for crds over the default crds.
  2. The upstream crd subchart is poorly defined so that the version is 0.0.0 and does not match the real upstream version 57.0.3, so we cannot simply manage a rancher-monitoring-crd since the rancher-monitoring and rancher-monitoring-crd charts will not have matching versions.
  3. The current additionalCharts configuration validation code does not allow for specifying both upstreamOptions and crdOptions so we cannot pull from an upstream and then use the crd template.

Solution

Allow for specifying both upstreamOptions to pull the crds directory, and crdOptions to use the crd template.

fixes #13
unblocks #44614

@joshmeranda joshmeranda requested a review from a team March 17, 2024 02:02
Copy link
Contributor

@adamkpickering adamkpickering left a comment

Choose a reason for hiding this comment

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

I think that allowing both crdOptions and upstreamOptions can work.

But, I don't think these changes are going to work like you want. Take a closer read through AdditionalChart.Prepare(): on line 114 there is a check for c.CRDChartOptions != nil. If it is not nil, then the following else clause, which pulls code using the object that results from upstreamOptions, does not run. So I think that using this code as you intend will result in an error because there is no crds/ directory.

Another thing: if you have time, it would be great if you could write some unit tests for your change. charts-build-scripts has none at the moment, and that makes it hard to change.

@joshmeranda
Copy link
Contributor Author

joshmeranda commented Mar 19, 2024

@adamkpickering I might be misunderstanding you but you are correct that if c.CRDChartOptions != nil we won't enter that next else branch to pull the charts. But it will enter the new nested if c.Upstream != nil check to pull down the crds. This works as expected for rancher-logging (no crd upstream) and rancher-monitoring (with crd upstream after rebase)

@adamkpickering
Copy link
Contributor

Ah you're right, I'm not sure how I missed that. Moving too fast I guess. The code looks good to me then.

We do need to update the documentation and comments though - there are places that say that crdOptions is mutually exclusive with upstreamOptions. Specifically templates/template/docs/packages.md and pkg/options/additionalchart.go.

There is duplication of this documentation in rancher/charts due to the templating. Don't worry about this - I will bring this up internal to the MAPPS team, and we'll have to open another PR or make other changes to address this problem.

@joshmeranda
Copy link
Contributor Author

Ope I forgot about the code docs. I'll go through and update what I can in the code. Thanks!

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.

Allow CRD Charts to pull from an upstream
2 participants