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

Helm test only pod should be ignored #547

Closed
sunshineo opened this issue Mar 5, 2021 · 8 comments
Closed

Helm test only pod should be ignored #547

sunshineo opened this issue Mar 5, 2021 · 8 comments
Assignees

Comments

@sunshineo
Copy link

Description of the bug:

When using the Helm construct to load a helm chart, the generated k8s yaml file contains pod that is test only. According to helm doc https://helm.sh/docs/topics/charts_hooks/ , those are only used when run helm test. The Helm construct should ignore it.

Reproduction Steps:

One helm chart can be used to reproduce is

  - name: elasticsearch
    repository: https://helm.elastic.co
    version: 7.6.2
    condition: elasticsearch.enabled

The final k8s yaml file will contain something like this when it should not

apiVersion: "v1"
kind: "Pod"
metadata:
  annotations:
    helm.sh/hook: "test-success"
  name: "temporal-fjobr-test"
spec:
  containers:
    - command:
        - "sh"
        - "-c"
        - "#!/usr/bin/env bash -e

          curl -XGET --fail
          'elasticsearch-master:9200/_cluster/health?wait_for_status=green&time\
          out=1s'\n"
      image: "docker.elastic.co/elasticsearch/elasticsearch:7.10.1"
      name: "temporal-yuizz-test"
  restartPolicy: "Never"

Error Log:

After kubectl apply you will see this

temporal-fjobr-test                                 0/1     Error        0          33s

This is 🐛 Bug Report

@sunshineo sunshineo added bug Something isn't working needs-triage Priority and effort undetermined yet labels Mar 5, 2021
@iliapolo
Copy link
Member

iliapolo commented Mar 8, 2021

@mbonig What do you think about this? is there a way we can remove those "test" pods?

@mbonig
Copy link
Contributor

mbonig commented Mar 8, 2021

@iliapolo, the Helm construct only does a simple helm template command. If this is rendering a test pod, then it seems like that's appropriate. If the chart defines the pod and renders it by default and you haven't provided an (available) override to prevent it, I don't really see it being the cdk8s job to understand what a 'test pod' is and remove it. This could lead to some nasty consequences later where someone is expecting a test pod and not getting it.

@sunshineo Are you seeing this happen on more than just that one Elasticsearch helm chart?

@mbonig
Copy link
Contributor

mbonig commented Mar 8, 2021

@sunshineo Reviewing the template more... it looks like there is a flag that can be provided to helm to prevent any tests from being create: --skip-tests. You should be able to provide this on the helmFlags property to suppress those that test.

@sunshineo
Copy link
Author

@mbonig Elasticsearch is the only chart I encountered doing this but any helm chart with test pods will encounter this.

I'll try the helmFlags property. Thank you very much!

@iliapolo
Copy link
Member

iliapolo commented Mar 9, 2021

@mbonig Thanks for taking a look. you reasoning makes perfect sense.

@sunshineo Let us know if helmFlags allow you bypass this issue so we can close it out.

Thanks!

@iliapolo iliapolo added guidance and removed needs-triage Priority and effort undetermined yet bug Something isn't working labels Mar 9, 2021
@sunshineo
Copy link
Author

I confirm --skip-tests works. Need to have helm version >= 3.5

@iliapolo
Copy link
Member

Cool. Closing this out then.

@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

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

No branches or pull requests

3 participants