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.

Sign up for GitHub

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

Jump to bottom

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
Draft

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

wants to merge 2 commits into from

Conversation

berlin-ab
Copy link

Helm installs CRDs that are in that directory during helm install

Authored-by: Adam Berlin aberlin@vmware.com

@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 <john.doe@email.org> 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.

@berlin-ab
Place crd in the chart directory's crds directory of the log router
bfe8a2c 
Helm installs CRDs that are in that directory during helm install

Authored-by: Adam Berlin <aberlin@vmware.com>
Signed-off-by: Adam Berlin <aberlin@vmware.com>
@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
View reviewed changes
@javiercri javiercri self-requested a review October 17, 2022 15:17
@javiercri
Copy link
Contributor

javiercri commented Oct 17, 2022 •
edited

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 <john.doe@email.org> 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.

@berlin-ab
Adds installCrd flag to helm chart values.
ce88625 
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 <aberlin@vmware.com>
Signed-off-by: Adam Berlin <aberlin@vmware.com>
@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
Reviewers

@javiercri javiercri Awaiting requested review from javiercri

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@berlin-ab @vmwclabot @javiercri