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

fix: put Druid crds in the appropriate folder specified by Helm #162

Merged

Conversation

TessaIO
Copy link
Collaborator

@TessaIO TessaIO commented May 17, 2024

This PR aims to put CRDs folder in the appropriate folder specified by Helm which is charts/crds, so that we can skip installing CRDs whenever we want.
Please refer to: https://helm.sh/docs/topics/charts/#custom-resource-definitions-crds

Description


This PR has:

  • been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
  • been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Key changed/added files in this PR
  • chart/templates/crds

@TessaIO TessaIO force-pushed the fix-put-crds-in-appropriate-folder branch 3 times, most recently from 884e8b0 to edcacde Compare May 17, 2024 14:19
Signed-off-by: ahmed.g <ahmed.g@adjoe.io>
Signed-off-by: TessaIO <ahmedgrati1999@gmail.com>
@TessaIO TessaIO force-pushed the fix-put-crds-in-appropriate-folder branch from edcacde to a823d27 Compare May 17, 2024 14:21
@TessaIO
Copy link
Collaborator Author

TessaIO commented May 17, 2024

@AdheipSingh is the test flaky? It seems that it's not related to the change.

@mariuskimmina
Copy link
Contributor

Would really like to see this getting merged, for some context:

This would enable users to spin up multiple druid clusters where every cluster is managed by a separate operator, currently this doesn't work because installing a second operator will fail, because the CRD is already present. This PR enables the use of --skip-crds during helm install which solves this problem.

@TessaIO
Copy link
Collaborator Author

TessaIO commented Jun 3, 2024

ping @AdheipSingh

@AdheipSingh AdheipSingh merged commit 643c4bf into datainfrahq:master Jun 3, 2024
1 check failed
TessaIO added a commit to TessaIO/druid-operator that referenced this pull request Jul 24, 2024
…infrahq#162)

Signed-off-by: ahmed.g <ahmed.g@adjoe.io>
Signed-off-by: TessaIO <ahmedgrati1999@gmail.com>
AdheipSingh added a commit that referenced this pull request Jul 28, 2024
* Ingestion Controller (#53)

* ingestion spec acc to v3

* task creation

* supprt native batch

* fix router url

* revert license change

* revert go mod change

* fix main

* fix: made some changes as per review comments

* fix: removed unused package from ingestion reconciler

* rebase 1

* add example

* add review

* update dockerfile

---------

Co-authored-by: avtarOPS <avtarsingh12015@gmail.com>

* Update Docs and Tutorials (#138)

* docs and tutorials

* Refactor/ordering (#123)

* (ordering): refactor code

* (ordering): refactor code

* (ordering): testing

* chore(branch): rebase branch with master

* fix(tests): validate nodes order by regex

* Bump controller-tools version (#140)

* Utilize the DruidIngestion controller in e2e tests (#146)

* adds needed volumes to eks deployment spec and improves getting started documentation by noting minio dependency (#149)

* Add support for annotations on Deployment/StatefulSet resources in DruidNodeSpec (#145)

* Add support for annotations on Deployment/StatefulSet resources

* Support setting ReplicationControllerAnnotations at the cluster-level

* rename replicationControllerAnnotations to workloadAnnotations

* suggestions from code review

* Add support for multi tier nodes with different PVC sizes (#106) (#152)

Co-authored-by: Farhad Farahi <farhad@adjoe.io>

* fix: put Druid crds in the appropriate folder specified by Helm (#162)

Signed-off-by: ahmed.g <ahmed.g@adjoe.io>
Signed-off-by: TessaIO <ahmedgrati1999@gmail.com>

* Adds service account name to each druid node optionally (#164)

* Adds service account to each druid node optionally

* Use controller-gen v0.11.2

---------

Signed-off-by: ahmed.g <ahmed.g@adjoe.io>
Signed-off-by: TessaIO <ahmedgrati1999@gmail.com>
Co-authored-by: AdheipSingh <34169002+AdheipSingh@users.noreply.github.com>
Co-authored-by: avtarOPS <avtarsingh12015@gmail.com>
Co-authored-by: Itamar Marom <46691031+itamar-marom@users.noreply.github.com>
Co-authored-by: Jesper Larsson <4522613+MrLarssonJr@users.noreply.github.com>
Co-authored-by: Sam Wheating <samwheating@gmail.com>
Co-authored-by: Evan Jones <evan.a.jones3@gmail.com>
Co-authored-by: Farhad Farahi <farhad.farahi@gmail.com>
Co-authored-by: Farhad Farahi <farhad@adjoe.io>
Co-authored-by: Sadananda Aithal <111732128+saithal-confluent@users.noreply.github.com>
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