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

Place crd in the chart directory's crds directory of the log router #343

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

berlin-ab
Copy link

Helm installs CRDs that are in that directory during helm install

Authored-by: Adam Berlin [email protected]

@vmwclabot
Copy link
Member

@berlin-ab, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Helm installs CRDs that are in that directory during helm install

Authored-by: Adam Berlin <[email protected]>
Signed-off-by: Adam Berlin <[email protected]>
@berlin-ab berlin-ab force-pushed the include-fluentd-crd-in-helm-install branch from 61880f3 to bfe8a2c Compare October 12, 2022 17:59
@berlin-ab
Copy link
Author

After installing the CRD, I couldn't figure out how to make the CRDs active. I eventually pulled up the codebase and took a look around. What do you think about allowing multiple datasources? If this is meant to be used by many teams, each team might have their own preference (configmap vs crds for example) and we should support both at the same time.

@berlin-ab
Copy link
Author

Even though the CRD gets installed by the operator once datasource: crd is set in the values.yaml. I think that it's an odd experience to perform a helm install and the CRD is automatically installed.

@javiercri
Copy link
Contributor

/lgtm

@javiercri javiercri self-requested a review October 17, 2022 15:16
javiercri
javiercri previously approved these changes Oct 17, 2022
@javiercri javiercri self-requested a review October 17, 2022 15:17
@javiercri
Copy link
Contributor

javiercri commented Oct 17, 2022

Could we add some helm value to install or not the crd like .Values.InstallCrd: enabled?

@berlin-ab
Copy link
Author

@javiercri That makes sense. In that case I think the default should be .values.installCrd==true

@javiercri
Copy link
Contributor

Sure. That is a good option

@javiercri javiercri dismissed their stale review October 17, 2022 15:57

improves suggested

@vmwclabot
Copy link
Member

@berlin-ab, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

This solves the user experience problem of reading the documentation about
the CRD, running helm install, and not being able to see the CRD in the
api-resources.

Previously the CRD was only installed if the datasource was crd, or the
crdMigrationMode was turned on, which was a confusing experience for me
as a newcomer.

Authored-by: Adam Berlin <[email protected]>
Signed-off-by: Adam Berlin <[email protected]>
@berlin-ab berlin-ab force-pushed the include-fluentd-crd-in-helm-install branch from 3485024 to ce88625 Compare October 21, 2022 15:58
@berlin-ab
Copy link
Author

@javiercri I introduced the helm chart flag installCrd with a default of enabled. I tested these changes via helm template locally because I didn't see a standard way of testing these properties.

@berlin-ab berlin-ab marked this pull request as draft October 21, 2022 16:03
@berlin-ab
Copy link
Author

We need to figure out what to do about the kubectl/crd.yaml file

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