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

Drop CRDs manifest but install CRDs by velero install #44

Closed
jenting opened this issue Jan 23, 2020 · 12 comments
Closed

Drop CRDs manifest but install CRDs by velero install #44

jenting opened this issue Jan 23, 2020 · 12 comments
Assignees
Labels

Comments

@jenting
Copy link
Collaborator

jenting commented Jan 23, 2020

Right now, the velero helm chart CRD manifest is not in-sync with velero repo.
Also the command velero install --crds-only supports install CRDs.

I'm wondering that is it better to drop helm-chart CRDs manifest but install the CRDs by veleror install --crds-only?
This way makes maintaining the CRDs from the single source and does not need to address sync CRD manifest from velero repo to velero helm chart.

Are there any other cons I did not think about? Thanks.

@carlisia
Copy link
Contributor

@cpanato do you have any opinion?

I'm going to add this to next's week community meeting agenda.

@carlisia
Copy link
Contributor

@cpanato
Copy link
Contributor

cpanato commented Jan 31, 2020

I saw other projects that don't install the CRDs as part of the helm installation, but I don't recall which ones were :(

I think this is a good idea but we need to make sure we output the additional commands we need to apply after we install the chart.

@jenting
Copy link
Collaborator Author

jenting commented Jan 31, 2020

I was work on this previously, if this is a good approach, I could help on this 😉

@nrb
Copy link
Contributor

nrb commented Jan 31, 2020

I've also seen other projects do this, due to the way Helm's designed.

It comes down to Helm wanting to manage a release's lifecycle, while a CRD by definition must live much longer than any one release. Helm has tried to fix it in several ways, and so far I haven't seen any of them be particularly reliable in Helm 2; I've not done any experimenting in Helm 3 to know if the situation has improved.

I do agree that maintaining one source of definitions would be good, because having them get out of sync is not good for anyone.

@carlisia
Copy link
Contributor

carlisia commented Feb 3, 2020

Just something to think about: we have the intention of dropping the velero install command: vmware-tanzu/velero#2202

@jenting
Copy link
Collaborator Author

jenting commented Jul 24, 2020

My idea on how to achieve this is:

  1. Run a Job which executes "velero install --crds-only" with helm pre-install hook to make sure that the CRDs are installed.
  2. The tests resources are installed with helm post-install hook.

However, after testing on helm3, it seems like the helm would not waits the Job executes completed then install the other resources. It means that right now I cannot find a way to achieve this.

BTW, I found two problems in current helm charts:

  1. The CRDs are out-of-sync compares to current Velero version v1.4.2
  2. The helm would not handles update existing CRDs when helm upgrade. Ref to https://helm.sh/docs/chart_best_practices/custom_resource_definitions/

@cpanato Do you have any better idea on this?

@carlisia
Copy link
Contributor

We'd need to generate the CRDs originally already containing these annotations right:

https://github.com/vmware-tanzu/helm-charts/blob/main/charts/velero/crds/backups.yaml#L8-L9

@jenting
Copy link
Collaborator Author

jenting commented Aug 5, 2020

We'd need to generate the CRDs originally already containing these annotations right:

https://github.com/vmware-tanzu/helm-charts/blob/main/charts/velero/crds/backups.yaml#L8-L9

According to discussion vmware-tanzu/velero#2779, we wouldn't generate the CRDs with the helm annotation included.

I have an idea that we could have a CI check (by script) to verify the current helm chart app version is aligned with vmware-tanzu/velero released version CRDs and the script also helps to add annotation/labels to the CRDs.

@caiobegotti
Copy link

caiobegotti commented Nov 24, 2020

What's the current supported and correct way of instaling the Helm chart with the CRDs? Right now if I render the chart I get no CRDs with a helm template (this way I can store it in my Git repository). Does it only installs CRDs if I use helm install? I tried installing the CRDs with the Velero CLI as suggested here but that's very sub-optimal and I can't quite get it to work with Kustomize as I'm also trying to keep it under version control (sometimes kubectl complains about the CRDs states being modified every time I try a kubectl apply on them after they're first installed).

So, TL;DR: I'm lost about how 's the correct way to get CRDs ready before deploying Velero with Helm (templated).

@jenting
Copy link
Collaborator Author

jenting commented Nov 25, 2020

@caiobegotti I assume you're using helm3, please add --include-crds flag to display CRDs in the templated output.

@jenting
Copy link
Collaborator Author

jenting commented Dec 19, 2020

Close the PR because we wrapper the way to install/upgrade CRDs by the velero install --crds-only --dry-run -oyaml, this makes helm template display the CRDs manifest not work anymore.

Open a new issue to address the helm upgrade does not upgrade the CRDs manifest, close it.

@jenting jenting closed this as completed Dec 19, 2020
Velero Helm Project Board automation moved this from In progress to Done Dec 19, 2020
ndegory pushed a commit to ndegory/helm-charts that referenced this issue Jul 12, 2021
* Add CODEOWNERS

I used this syntax in CODEOWNERS:

```
/chart/<name-of-chart> @maintainer
```

It matches any files in the chart directory at the root of the repository and any of its  subdirectories.
Without the leading `/` it would also match directories found somewhere
else. It's unlikely that those names would be used, but it does not harm
to do it this way.

Part-of: vmware-tanzu#38

Signed-off-by: Torsten Walter <mail@torstenwalter.de>

* sort charts alphabetically

Signed-off-by: Torsten Walter <mail@torstenwalter.de>

* adjust existing CODEOWNERS

Signed-off-by: Torsten Walter <mail@torstenwalter.de>

* link to CODEOWNERS file and fixed spelling

Signed-off-by: Torsten Walter <mail@torstenwalter.de>
ndegory pushed a commit to ndegory/helm-charts that referenced this issue Jul 12, 2021
* Initial chart direectory rename prometheus-operator to kube-prometheus

See prometheus-community/community#28 (comment)

Signed-off-by: Scott Rigby <scott@r6by.com>

* First attempt at data change of prometheus-operator to kube-prometheus

Signed-off-by: Scott Rigby <scott@r6by.com>

* Helm GH Action files as-is from https://github.com/helm/charts-repo-actions-demo

Signed-off-by: Scott Rigby <scott@r6by.com>

* Bump chart-testing action to v1.0.0. See helm/charts-repo-actions-demo#20

Signed-off-by: Scott Rigby <scott@r6by.com>

* Changes to chart testing config file for this repo

Signed-off-by: Scott Rigby <scott@r6by.com>

* Use built-in GITHUB_TOKEN now that GH Actions bug is fixed. See helm/chart-releaser-action#26

Signed-off-by: Scott Rigby <scott@r6by.com>

* Test actions with version bump to prometheus chart

Signed-off-by: Scott Rigby <scott@r6by.com>

* Bump chart-releaser-action and kind-action to v1.0.0

Signed-off-by: Scott Rigby <scott@r6by.com>

* Release from main not master branch

Signed-off-by: Scott Rigby <scott@r6by.com>

* Revert "Test actions with version bump to prometheus chart"

This reverts commit 81c50e5.

Signed-off-by: Scott Rigby <scott@r6by.com>

* Allow requirements lock files

Signed-off-by: Scott Rigby <scott@r6by.com>

* Initial CODEOWNERS file (vmware-tanzu#18)

Signed-off-by: Scott Rigby <scott@r6by.com>

* Prep initial charts indexing (vmware-tanzu#14)

* [stable/prometheus] update prometheus to 2.20.1 and cm reloader to 0.4.0 (#23506)

* updated prometheus to 2.20.1 and cm reloader to 0.4.0

Signed-off-by: André Bauer <monotek23@gmail.com>

* fix xpp version

Signed-off-by: André Bauer <monotek23@gmail.com>

* Deprecate prometheus-operator chart before helm repo index, so that it won't be listed in the hubs

Signed-off-by: Scott Rigby <scott@r6by.com>

* Update prometheus-community/prometheus chart. Needed to update references to stable repo, but took the opportunity to reorganize, fix and simplify README

Signed-off-by: Scott Rigby <scott@r6by.com>

* Add Helm 3 commands before Helm 2. Add helm update command. Reorganize the 'Upgrading Chart' section

Signed-off-by: Scott Rigby <scott@r6by.com>

* Fix header

Signed-off-by: Scott Rigby <scott@r6by.com>

* Fix markdown linting

Signed-off-by: Scott Rigby <scott@r6by.com>

* Add direct links to values.yaml configuration file for easy browsing by end users without the CLI

Signed-off-by: Scott Rigby <scott@r6by.com>

* Remove prometheus chart OWNERS file

Signed-off-by: Scott Rigby <scott@r6by.com>

* Update prometheus-adapter chart README and bump version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus chart: Return updated, working command example for Sharing Alerts Between Services

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-adapter: fix configure command typos

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-blackbox-exporter: Update readme, delete OWNERS file and bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-cloudwatch-exporter: Update Readme with new template, delete OWNERS file, bump chart version, update CHANGELOG

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-consul-exporter: Update Readme per new template and bump chart

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-couchdb-exporter: Update Readme per new template and bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-mongodb-exporter: Update Readme per new template, remove OWNERS file, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-couchdb-exporter: Fix k8s 1.16 deprecated PodSecurityPolicy in the extensions/v1beta1 API version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-couchdb-exporter: Fix bad YAML indentation. How did this ever work?

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-mysql-exporter: update readme per new template, remove OWNERS file, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-nats-exporter: update readme per new template, move specific config note to values.yaml, remove OWNERS file, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-node-exporter: update readme per new template, remove OWNERS file, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-postgres-exporter: update readme per new template, remove OWNERS file, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-pushgateway: update readme per new template, remove OWNERS file, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-rabbitmq-exporter: update readme per new template, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-redis-exporter: update readme per new template, remove OWNERS file, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-snmp-exporter: update readme per new template, remove OWNERS file, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-to-sd: update readme per new template, remove OWNERS file, bump chart version

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-to-sd: fix deprecated deployment apps/v1beta1

Signed-off-by: Scott Rigby <scott@r6by.com>

* Remove instructions for stable repos from all charts, except prometheus and deprecated prometheus-operator, as only those have dependencies on stable charts

Signed-off-by: Scott Rigby <scott@r6by.com>

* Temporary workaround github API rate limiting

Signed-off-by: Scott Rigby <scott@r6by.com>

* prometheus-to-sd: missing required field "selector" in io.k8s.api.apps.v1.DeploymentSpec

Signed-off-by: Scott Rigby <scott@r6by.com>

* disable chart testing for prometheus-to-sd. If not running on GCE, will error: "Failed to get GCE config"

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: André Bauer <monotek@users.noreply.github.com>

* Add kube-prometheus chart maintainers to CODEOWNERS after merging main

Signed-off-by: Scott Rigby <scott@r6by.com>

* Un-deprecate chart within renaming to kube-prometheus PR

Signed-off-by: Scott Rigby <scott@r6by.com>

* Change all references to old coreos/prometheus-operator and coreos/kube-prometheus git repos to the new prometheus-operator github org

Signed-off-by: Scott Rigby <scott@r6by.com>

* Remove stray CODEOWNERS rule for charts/prometheus-operator/

Signed-off-by: Scott Rigby <scott@r6by.com>

* Fix typo

Signed-off-by: Scott Rigby <scott@r6by.com>

* Update charts/kube-prometheus/hack/README.md

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Update charts/kube-prometheus/hack/README.md

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Update charts/kube-prometheus/hack/README.md

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Update charts/kube-prometheus/templates/prometheus/rules/prometheus-operator.yaml

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Update charts/kube-prometheus/templates/prometheus/rules/node.rules.yaml

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Update charts/kube-prometheus/templates/prometheus/rules/node-network.yaml

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Update charts/kube-prometheus/templates/prometheus/rules/node-time.yaml

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Update charts/kube-prometheus/templates/prometheus/rules/kubernetes-system.yaml

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Update charts/kube-prometheus/README.md

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Update dependency location and remove README note about chart source (it's easy to determine chart source from the dependency definition)

Signed-off-by: Scott Rigby <scott@r6by.com>

* Fix markdownlint

Signed-off-by: Scott Rigby <scott@r6by.com>

* Update kube-prometheus per new README template. See vmware-tanzu#14

Signed-off-by: Scott Rigby <scott@r6by.com>

* Remove requirements lock file for now, otherwise if we release the chart before transferring repo ownership the digest will differ. See helm pkg downloader Manager Build() method check for resolveRepoNames()

Signed-off-by: Scott Rigby <scott@r6by.com>

* Non-functional: update commented links to CRD sources

Co-authored-by: Quentin Bisson <quentin@giantswarm.io>

Signed-off-by: Scott Rigby <scott@r6by.com>

* Add GitHub superlinter to lint markdown (vmware-tanzu#26)

* Create linter.yml

Signed-off-by: Torsten Walter <mail@torstenwalter.de>
Co-authored-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Add configuration for Stale GitHub app (vmware-tanzu#27)

Signed-off-by: Scott Rigby <scott@r6by.com>

* disabled failing linters and fixed markdown issues (vmware-tanzu#32)

- fixes markdown issues reported by markdownlint
- disabled yamllint as helm templates are never valid
- disabled the other linters as there is a problem with a shell script and some python code
  once that is fixed we could enable them again

Signed-off-by: Torsten Walter <mail@torstenwalter.de>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Rename chart dir

Signed-off-by: Scott Rigby <scott@r6by.com>

* Update CODEOWNERS for new chart dir name

Signed-off-by: Scott Rigby <scott@r6by.com>

* Rename instances of kube-prometheus to kube-prometheus-stack. Take care to leave references to the upstream kube-prometheus project (and related configs) as kube-prometheus

Signed-off-by: Scott Rigby <scott@r6by.com>

* Chart testing needs this repo info to test chart dependencies in the same repo

Signed-off-by: Scott Rigby <scott@r6by.com>

* Auto-sync README from main to gh-pages (vmware-tanzu#41)

* Auto-sync README from main to gh-pages

Signed-off-by: Scott Rigby <scott@r6by.com>

* Only runs on push to main

even if this workflow is copied to a new branch

Signed-off-by: Scott Rigby <scott@r6by.com>

* Improve README for main and gh pages (vmware-tanzu#43)

Signed-off-by: Scott Rigby <scott@r6by.com>

* [prometheus] unify labels and annotations across all deploymens and statefulsets (vmware-tanzu#45)

Signed-off-by: Ondrej Homolka <ondrej.homolka@gmail.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

* [prometheus-redis-exporter] Add zanhsieh as maintainer (vmware-tanzu#46)

Signed-off-by: zanhsieh <zanhsieh@gmail.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

* added link to github to readme (vmware-tanzu#51)

Signed-off-by: André Bauer <monotek23@gmail.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

* Add PROCESSES document (vmware-tanzu#44)

* Add CODEOWNERS

I used this syntax in CODEOWNERS:

```
/chart/<name-of-chart> @maintainer
```

It matches any files in the chart directory at the root of the repository and any of its  subdirectories.
Without the leading `/` it would also match directories found somewhere
else. It's unlikely that those names would be used, but it does not harm
to do it this way.

Part-of: vmware-tanzu#38

Signed-off-by: Torsten Walter <mail@torstenwalter.de>

* sort charts alphabetically

Signed-off-by: Torsten Walter <mail@torstenwalter.de>

* adjust existing CODEOWNERS

Signed-off-by: Torsten Walter <mail@torstenwalter.de>

* link to CODEOWNERS file and fixed spelling

Signed-off-by: Torsten Walter <mail@torstenwalter.de>

* feat: adding issue templates (vmware-tanzu#54)

* feat: adding issue templates

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* feat: PR template and review comments

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

* [prometheus-consul-exporter] add gkarthiks as additional maintainers (vmware-tanzu#50)

* adding gkarthiks for additional maintainers

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* fix: new line char

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* adding gkarthiks to codeowners against consul

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

* [prometheus] - adds monotek to prometheus maintainers (vmware-tanzu#55)

* added monotek to prometheus maintainers

Signed-off-by: André Bauer <andre.bauer@kiwigrid.com>

* rearrange the new codeowner for prometheus chart

Signed-off-by: Xtigyro <miroslav.hadzhiev@gmail.com>

Co-authored-by: Miroslav Hadzhiev <miroslav.hadzhiev@gmail.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

* [prometheus-blackbox-exporter] fix linting failure due to deprecated api version (see issue vmware-tanzu#56) (vmware-tanzu#57)

* fix linting failure due to deprecated api version (see issue vmware-tanzu#56)

Signed-off-by: Jorrit Salverda <jsalverda@travix.com>

* use rbac.apiVersion template to set correct apiVersion for role and rolebinding

Signed-off-by: Jorrit Salverda <jsalverda@travix.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

* formatted GitHub templates and made minor adjustments (vmware-tanzu#59)

* formatted GitHub templates and made minor adjustments

Signed-off-by: Torsten Walter <mail@torstenwalter.de>
Signed-off-by: Scott Rigby <scott@r6by.com>

* [kube-prometheus-stack] Fix Chart Name and Rm Whitespaces in "NOTES.txt" (vmware-tanzu#60)

* fix chart name in NOTES.txt

Signed-off-by: Xtigyro <miroslav.hadzhiev@gmail.com>

* rm whitespaces in NOTES.txt

Signed-off-by: Xtigyro <miroslav.hadzhiev@gmail.com>
Signed-off-by: Scott Rigby <scott@r6by.com>

* feat: replacing grafana rom stable to its own repo + additional chart maintainer (vmware-tanzu#53)

* feat: replacing grafana own repo

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* fix: trailing white spaces

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* fix: reverting the grafana values

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* adding grafana repo for actions

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* add: adding grafana repo in linter

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* doc(lint): making doc stmt as single stmt

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* revert: reverting the old README statement

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

* feat: adding gkarthiks to codeowners against kube-prometheus-stack

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

Signed-off-by: Karthikeyan Govindaraj <30545166+gkarthiks@users.noreply.github.com>

* Add scottrigby as co-maintainer of kube-prometheus-stack

Signed-off-by: Scott Rigby <scott@r6by.com>

* add xtigyro as maintainer for kube-prometheus-stack (vmware-tanzu#73)

Signed-off-by: Miroslav Hadzhiev <miroslav.hadzhiev@gmail.com>

* Revert header for simplicity

Co-authored-by: Cédric de Saint Martin <cdesaintmartin@wiremind.fr>

Signed-off-by: Scott Rigby <scott@r6by.com>

Co-authored-by: André Bauer <monotek@users.noreply.github.com>
Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Co-authored-by: Torsten Walter <mail@torstenwalter.de>
Co-authored-by: hmlkao <ondrej.homolka@gmail.com>
Co-authored-by: zanhsieh <zanhsieh@gmail.com>
Co-authored-by: Karthikeyan Govindaraj <30545166+gkarthiks@users.noreply.github.com>
Co-authored-by: Miroslav Hadzhiev <miroslav.hadzhiev@gmail.com>
Co-authored-by: Jorrit Salverda <JorritSalverda@users.noreply.github.com>

Signed-off-by: Scott Rigby <scott@r6by.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants