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 Unit Tests #29

Merged
merged 37 commits into from
Nov 15, 2023
Merged

Helm Unit Tests #29

merged 37 commits into from
Nov 15, 2023

Conversation

bpblanken
Copy link
Contributor

@bpblanken bpblanken commented Nov 14, 2023

Adds some unit tests, a single "integration" test, and CI for the seqr/hail-search templating.

@bpblanken bpblanken changed the title Benb/barebones unittests Helm Unittests Nov 14, 2023
@bpblanken bpblanken changed the title Helm Unittests Helm Unit Tests Nov 14, 2023
@@ -4,6 +4,9 @@ description: A Helm chart for deploying the hail backend of Seqr, an open source
sources:
- https://github.com/broadinstitute/seqr
- https://github.com/broadinstitute/seqr-helm
maintainers:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The chart linter not only requires the maintainers section, but requires that the name be discoverable on GitHub, e.g. https://github.com/seqr.

@@ -52,7 +52,7 @@ initContainers: |-
{{ end }}
{{- range $path, $queries := $.Values.cachedReferenceDatasetQueries -}}
{{- range $query := $queries -}}
- name: sync-cached-reference-dataset-queries-{{ $path | replace "/" "-" | replace "_" "-" | lower}}
- name: sync-crdq-{{ $path | replace "/" "-" | replace "_" "-" | lower}}-{{ $query | replace "_" "-" | lower}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing found a bug with this line... need to be < 63 chars and need to include "$query" in the resource name to avoid having multiple resources with the same name.

@@ -103,13 +103,13 @@ environment:
ELASTICSEARCH_SERVICE_PORT: "9200"
# -- The URL protocol that seqr should use to connect to elasticsearch
ELASTICSEARCH_PROTOCOL: "http"
#ELASTICSEARCH_CA_PATH: "/elasticsearch-certs/ca.crt"
# ELASTICSEARCH_CA_PATH: "/elasticsearch-certs/ca.crt"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint related changes.

os.path.join(WORK_DIR, 'values.yaml')
]

class TestSeqrChart(unittest.TestCase):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a little test harness that just sanity checks the helm install --dry-run with some values permutations.

The chart-testing tool doesn't have great support for overriding values, and I thought generating a full running set of kubernetes resources for different permutations of values was a little overkill to solve the problem we're having with having bad yaml whitespace.

helm install also gives us a little more validation than helm template as well, as you can see from the cronjob name correctly failing to render.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's unfortunate that overriding is janky :/, but i think this is a reasonable solution.

If you're looking to validate templated output against the k8s API schema without running kubernetes, there is https://github.com/yannh/kubeconform, which can be helpful. Though, i have found cases with this over in clingen where we're outputting YAML that is API spec compliant, but won't result in a successful deploy, so having some kind of helm install in there somewhere is still good to have.

@@ -0,0 +1,112 @@
replicaCount: 4
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a set of values that better reflects what we (or also a potential user of this chart) are using.

kubectl create secret generic seqr-secrets --from-literal=django_key='securely-generated-key'

# The extra variable is to allow kubernetes to ping our postrgres running locally
ct install --charts charts/seqr --namespace default --helm-extra-set-args "--set=environment.POSTGRES_SERVICE_HOSTNAME=172.17.0.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line basically validates that we can get a seqr service up and passing a health check with the default set of values provided for the seqr chart (so no redis, no ingress, etc). It's useful but limited.

Getting a similar test for hail-search is trickier because that service requires files during the init and at startup... getting that fixed seemed out of scope.

@bpblanken bpblanken marked this pull request as ready for review November 15, 2023 14:28
Copy link
Collaborator

@sjahl sjahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me -- seems similar to the flow I had ended up on for developing some of the gnomAD charts 👍

@bpblanken bpblanken merged commit cd6b017 into main Nov 15, 2023
1 check passed
@bpblanken bpblanken deleted the benb/barebones_unittests branch November 15, 2023 19:06
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