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

chart: annotate all CRDs with "crd-install" hook #823

Merged
merged 4 commits into from
Aug 17, 2018

Conversation

ccojocar
Copy link
Contributor

What this PR does / why we need it:

It annotates all CRDs with "crd-install" hook.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

This fix is required in order to make helm properly verify the CRDs
when cert-manager chart is referenced as a dependency of another chart.

Special notes for your reviewer:

@munnerz Do I have to send this patch also to stable/cert-manager chart, or are you going to sync that chart from this repo?

Release note:

@jetstack-bot
Copy link
Contributor

@ccojocar: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jetstack-bot jetstack-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 13, 2018
@munnerz
Copy link
Member

munnerz commented Aug 13, 2018

/ok-to-test

@jetstack-bot jetstack-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 13, 2018
@jetstack-bot jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 13, 2018
This fix is required in order to make helm properly verify the CRDs
when cert-manager chart is referenced as a dependency of another chart.

Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>
Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>
Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>
@ccojocar
Copy link
Contributor Author

@munnerz It seems that the e2e tests fail without being related to my changes. Please could you help with the build? Thanks

@munnerz
Copy link
Member

munnerz commented Aug 14, 2018

It looks to me like Helm is failing to install the CRDs properly on versions of Helm that do not support this hook (i.e. pre-2.10).

We currently do not use Helm 2.10 as part of our CI infrastructure, hence the errors. We'll be upgrading as soon as 2.10 stable comes out, but in the meantime this sort of thing is effectively blocked, as it'll mean using the Helm chart will require users have a RC version of Helm/Tiller installed in their cluster 😬

@ccojocar
Copy link
Contributor Author

@munnerz Thanks for help! Let me see if I can make it backward compatible.

Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>
@ccojocar
Copy link
Contributor Author

@munnerz The annotation is now applied only for helm 2.10

@munnerz
Copy link
Member

munnerz commented Aug 17, 2018

Thanks @ccojocar!

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2018
@munnerz
Copy link
Member

munnerz commented Aug 17, 2018

/release-note-none

@jetstack-bot jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 17, 2018
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2018
@jetstack-bot jetstack-bot merged commit 7cc4083 into cert-manager:master Aug 17, 2018
@marshall007
Copy link

@munnerz now that helm v2.10 has been released, could we get this merged upstream to stable/cert-manager?

munnerz added a commit to munnerz/charts-1 that referenced this pull request Sep 10, 2018
* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>
k8s-ci-robot pushed a commit to helm/charts that referenced this pull request Sep 13, 2018
* cert-manager: fast-forward to upstream bcffc635

* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>

* Update image tag and add description

Signed-off-by: James Munnelly <james@munnelly.eu>
jicowan pushed a commit to jicowan/charts that referenced this pull request Oct 2, 2018
* cert-manager: fast-forward to upstream bcffc635

* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>

* Update image tag and add description

Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: jenkin-x <jicowan@hotmail.com>
Jnig pushed a commit to Jnig/charts that referenced this pull request Nov 13, 2018
* cert-manager: fast-forward to upstream bcffc635

* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>

* Update image tag and add description

Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: Jakob Niggel <info@jakobniggel.de>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
* cert-manager: fast-forward to upstream bcffc635

* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>

* Update image tag and add description

Signed-off-by: James Munnelly <james@munnelly.eu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants