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

[stable/prometheus-operator] Introduce crds directory for compatibility with Helm v3 #18721

Merged
merged 2 commits into from Nov 13, 2019

Conversation

@bhavin192
Copy link
Contributor

bhavin192 commented Nov 8, 2019

What this PR does / why we need it:

Special notes for your reviewer:

  • This PR does not do the complete migration of this chart to v2 apiVersion of Chart.yaml. This adds changes which ensure that the current chart is installable with Helm v3 as well.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [stable/mychartname])
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2019

Hi @bhavin192. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@vsliouniaev

This comment has been minimized.

Copy link
Collaborator

vsliouniaev commented Nov 8, 2019

/ok-to-test

@vsliouniaev

This comment has been minimized.

Copy link
Collaborator

vsliouniaev commented Nov 8, 2019

I wonder if we can just load the files from the directory and apply them using a list.

Not sure if that would break the CRD install hooks checking for CRDs being established in Helm 2 or break upgrading the chart. Would you be interested in checking this out?

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Nov 8, 2019

Yeah, that's an interesting idea. Thanks, let me try that out.

@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Nov 12, 2019

I deleted the crd-*.yaml files from templates/prometheus-operator and created a crds.yaml in same directory. Also added crd-install hook to files from crds directory. Helm v3 does not take any special action on the YAML if we have the crd-install annotation.

  • With List approach: (creating the List object out of files from crds directory at top level)
    # templates/prometheus-operator/crds.yaml
    apiVersion: v1
    kind: List
    items:
    {{- range $path, $bytes := .Files.Glob "crds/*.yaml" }}
    - {{ $.Files.Get $path | nindent 2 }}
    {{- end }}    
    • Helm v2: fails to install as well upgrade existing chart. Basically it does not consider the crd-install hook from items: section of List object.
    • Helm v3: fails with the error that the CRDs already exist. Reason is same as above. Does not consider the hook which prevents it from ignoring those objects.
  • With separate document approach: (creating one file with all the CRD objects separated by ---)
    # templates/prometheus-operator/crds.yaml
    {{- range $path, $bytes := .Files.Glob "crds/*.yaml" }}
    {{ $.Files.Get $path }}
    ---
    {{- end }}
    • Helm v2: new installation works, CRDs are installed first. Upgrade from previous installation works as well.
    • Helm v3: install as well as upgrade works just fine. Objects from crds.yaml are ingored as they have crd-install hook in them.

@vsliouniaev what do you think, should we go with the second approach?

@vsliouniaev

This comment has been minimized.

Copy link
Collaborator

vsliouniaev commented Nov 12, 2019

@bhavin192 thanks for checking this one out. I think that option #2 sounds like it should be the best option.

@bhavin192 bhavin192 force-pushed the bhavin192:prom-operator-helm3 branch from 97d137e to a7639eb Nov 12, 2019
@helm-bot helm-bot added size/M and removed size/XXL labels Nov 12, 2019
bhavin192 added 2 commits Nov 8, 2019
- This adds `crds` directory which has all 5 CRDs
- As files from crds directory needs to be plain YAMLs, keeps
  app: prometheus-operator label and removes all the templating
- With this change `.Values.prometheusOperator.createCustomResource`
  will be ignored by Helm v3 as it handles the CRDs differently. Which
  means, if the CRDs already present in the cluster then it won't
  touch them at all
- Helm v3 ignores the objects from `templates` directory which have a
  `crd-install` hook
- Update the README.md
- Update the chart version

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
- Deletes the templates/prometheus-operator/crd-*.yaml
- Adds templates/prometheus-operator/crds.yaml which generates YAMLs
  for CRDs. The hooks from these CRDs are detected by Helm v2 as well
  as v3. Helm v2 executes the hook and Helm v3 ignores the hook (YAML
  files are not applied)

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
@bhavin192 bhavin192 force-pushed the bhavin192:prom-operator-helm3 branch from a7639eb to 711f362 Nov 12, 2019
@bhavin192

This comment has been minimized.

Copy link
Contributor Author

bhavin192 commented Nov 13, 2019

@vsliouniaev I made the changes according to the second option, please take a look whenever you are around :)

@vsliouniaev

This comment has been minimized.

Copy link
Collaborator

vsliouniaev commented Nov 13, 2019

/lgtm
Looks like it's working!

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 13, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bhavin192, vsliouniaev

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

@k8s-ci-robot k8s-ci-robot merged commit 89b233e into helm:master Nov 13, 2019
6 checks passed
6 checks passed
DCO DCO
Details
ci/circleci: lint-charts Your tests passed on CircleCI!
Details
ci/circleci: lint-scripts Your tests passed on CircleCI!
Details
dco-labeler All commits have signoff
pull-charts-e2e Job succeeded.
Details
tide In merge pool.
Details
JoseAlban added a commit to JoseAlban/charts that referenced this pull request Nov 22, 2019
…ty with Helm v3 (helm#18721)

* Introduce crds directory for compatibility with Helm v3

- This adds `crds` directory which has all 5 CRDs
- As files from crds directory needs to be plain YAMLs, keeps
  app: prometheus-operator label and removes all the templating
- With this change `.Values.prometheusOperator.createCustomResource`
  will be ignored by Helm v3 as it handles the CRDs differently. Which
  means, if the CRDs already present in the cluster then it won't
  touch them at all
- Helm v3 ignores the objects from `templates` directory which have a
  `crd-install` hook
- Update the README.md
- Update the chart version

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>

* Use files from crds directory to generate CRDs for Helm v2

- Deletes the templates/prometheus-operator/crd-*.yaml
- Adds templates/prometheus-operator/crds.yaml which generates YAMLs
  for CRDs. The hooks from these CRDs are detected by Helm v2 as well
  as v3. Helm v2 executes the hook and Helm v3 ignores the hook (YAML
  files are not applied)

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
@scottrigby scottrigby mentioned this pull request Dec 1, 2019
4 of 4 tasks complete
hakman added a commit to hakman/charts that referenced this pull request Dec 5, 2019
…ty with Helm v3 (helm#18721)

* Introduce crds directory for compatibility with Helm v3

- This adds `crds` directory which has all 5 CRDs
- As files from crds directory needs to be plain YAMLs, keeps
  app: prometheus-operator label and removes all the templating
- With this change `.Values.prometheusOperator.createCustomResource`
  will be ignored by Helm v3 as it handles the CRDs differently. Which
  means, if the CRDs already present in the cluster then it won't
  touch them at all
- Helm v3 ignores the objects from `templates` directory which have a
  `crd-install` hook
- Update the README.md
- Update the chart version

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>

* Use files from crds directory to generate CRDs for Helm v2

- Deletes the templates/prometheus-operator/crd-*.yaml
- Adds templates/prometheus-operator/crds.yaml which generates YAMLs
  for CRDs. The hooks from these CRDs are detected by Helm v2 as well
  as v3. Helm v2 executes the hook and Helm v3 ignores the hook (YAML
  files are not applied)

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
dargolith added a commit to dargolith/charts that referenced this pull request Jan 10, 2020
…ty with Helm v3 (helm#18721)

* Introduce crds directory for compatibility with Helm v3

- This adds `crds` directory which has all 5 CRDs
- As files from crds directory needs to be plain YAMLs, keeps
  app: prometheus-operator label and removes all the templating
- With this change `.Values.prometheusOperator.createCustomResource`
  will be ignored by Helm v3 as it handles the CRDs differently. Which
  means, if the CRDs already present in the cluster then it won't
  touch them at all
- Helm v3 ignores the objects from `templates` directory which have a
  `crd-install` hook
- Update the README.md
- Update the chart version

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>

* Use files from crds directory to generate CRDs for Helm v2

- Deletes the templates/prometheus-operator/crd-*.yaml
- Adds templates/prometheus-operator/crds.yaml which generates YAMLs
  for CRDs. The hooks from these CRDs are detected by Helm v2 as well
  as v3. Helm v2 executes the hook and Helm v3 ignores the hook (YAML
  files are not applied)

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
@yspotts yspotts mentioned this pull request Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.