test: add helm test hook to test resources #25

Kortekaasy · 2023-10-23T12:55:42Z

What type of PR is this?
cleanup

Which issue does this PR fix:
When deploying the helm chart, the two secrets from the test-connection.yaml test are also deployed in the K8s cluster.

What does this PR do / Why do we need it:
This PR adds the "helm.sh/hook": test annotation to these two secrets, such that they are only deployed when a Helm test is run.

If an issue # is not available please add repro steps and logs showing the issue:
To reproduce, install helm chart and check the secrets associated to the helm chart.

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?
Probably not? Unless a deployment is dependent on these testing resources being present

Does this PR introduce any user-facing change?:

yes

No longer deploys `servercert` and `serverkey` Secrets when not testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Kortekaasy · 2023-10-23T13:53:52Z

Not sure why test here is failing, when using a fresh minikube cluster, the helm install&test seems to work fine...

% minikube delete              
🔥  Deleting "minikube" in docker ...
🔥  Deleting container "minikube" ...
🔥  Removing /Users/kortekaasy/.minikube/machines/minikube ...
💀  Removed all traces of the "minikube" cluster.
% minikube start               
😄  minikube v1.31.2 on Darwin 14.0 (arm64)
✨  Automatically selected the docker driver. Other choices: qemu2, ssh, podman (experimental)
📌  Using Docker Desktop driver with root privileges
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
🔥  Creating docker container (CPUs=2, Memory=4000MB) ...
🐳  Preparing Kubernetes v1.27.4 on Docker 24.0.4 ...
    ▪ Generating certificates and keys ...
    ▪ Booting up control plane ...
    ▪ Configuring RBAC rules ...
🔗  Configuring bridge CNI (Container Networking Interface) ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: storage-provisioner, default-storageclass
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default
% helm install myzot charts/zot
NAME: myzot
LAST DEPLOYED: Mon Oct 23 15:49:59 2023
NAMESPACE: default
STATUS: deployed
REVISION: 1
NOTES:
Get the application URL by running these commands:
  export NODE_PORT=$(kubectl get --namespace default -o jsonpath="{.spec.ports[0].nodePort}" services myzot)
  export NODE_IP=$(kubectl get nodes --namespace default -o jsonpath="{.items[0].status.addresses[0].address}")
  echo http://$NODE_IP:$NODE_PORT
% helm test myzot              
NAME: myzot
LAST DEPLOYED: Mon Oct 23 15:49:59 2023
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE:     servercert
Last Started:   Mon Oct 23 15:51:13 2023
Last Completed: Mon Oct 23 15:51:13 2023
Phase:          Succeeded
TEST SUITE:     serverkey
Last Started:   Mon Oct 23 15:51:13 2023
Last Completed: Mon Oct 23 15:51:13 2023
Phase:          Succeeded
TEST SUITE:     myzot-test-connection-fails
Last Started:   Mon Oct 23 15:51:11 2023
Last Completed: Mon Oct 23 15:51:13 2023
Phase:          Succeeded
TEST SUITE:     myzot-test-connection
Last Started:   Mon Oct 23 15:50:56 2023
Last Completed: Mon Oct 23 15:51:11 2023
Phase:          Succeeded
NOTES:
Get the application URL by running these commands:
  export NODE_PORT=$(kubectl get --namespace default -o jsonpath="{.spec.ports[0].nodePort}" services myzot)
  export NODE_IP=$(kubectl get nodes --namespace default -o jsonpath="{.items[0].status.addresses[0].address}")
  echo http://$NODE_IP:$NODE_PORT

Kortekaasy · 2023-10-30T12:30:15Z

@rchincha @andaaron @peusebiu @Andreea-Lupu Any comments on this PR?

Andreea-Lupu · 2023-10-30T12:51:52Z

Hi @Kortekaasy, the reason for what the checks failed is that zot can't start because secretcert and secretkey secrets can't be found because they are not deployed yet. I saw that the chart with the given values file is installed first and after that the other resources with test hook are deployed, so the secrets will be deployed after installing chart which will fail because it needs those external secrets(for the config in ci/tls-values.yaml). And I think the reason for what your helm install&test works fine is that it uses the default values file which doesn't need secrets, if you will try to run the same commands for ci/tls-values.yaml values file(without creating first those secrets) you will have the same problem with the secrets. We need to find a better way to install those secrets(before deploying the chart using ci/*-values.yaml files) only when helm test is running and I started to work on this.

Kortekaasy · 2023-10-31T15:25:39Z

Hi @Andreea-Lupu, thanks for the response. Would it make sense to create separate helm charts for every CI test, that refer to the zot-registry chart as sub-chart? That way you could put the extra resources needed for your CI tests in the templates directory of the concerned CI test.

rchincha · 2023-11-01T16:58:06Z

@Kortekaasy Pls also look at Andreea-Lupu@d3394d9#diff-662dff05c7dfe6681c1007ae601e8573d413a03f9f9d53d951c2cb99caba4fd4 and https://github.com/grafana/helm-charts/blob/main/charts/grafana/values.yaml#L109

There are some options here.

Kortekaasy · 2023-11-02T13:58:31Z

@rchincha @Andreea-Lupu

Thanks for the pointers. I've played around with testing the different deployments as well, and I've come up with the solution described in my helm-chart-tests branch.

I create separate 'test' helm charts in the /ci/ directory, that refer to the zot chart in the /charts/zot/ directory, and that contain the values & resources needed for that specific test. I'm personally a fan of this approach, since it cleanly separates which resources are needed for which tests, and there is very little probability of 'cross contamination' between different tests.

Let me know what you think about this approach, would be happy to create a PR for it

rchincha · 2023-11-09T05:57:32Z

@Kortekaasy would put those under tests/ci. But like the approach though. Pls post an updated PR and let's take a lot.

Kortekaasy · 2023-11-13T10:05:08Z

@rchincha

Sorry, forgot to ping you after I finished my changes. I think everything should be ready like this. Let me know if there's anything else I need to change.

Andreea-Lupu · 2023-11-15T13:34:55Z

Hi @Kortekaasy . I tried your PR and it looks good for the most part. I like this structure because everything looks cleaner and more organized, but I only have 2 concerns:

If ct install is run locally then it will generate some new files and directories, so we should be careful to not push them when commit some new changes, maybe we need something like a .gitignore file to ignore these files
If the version of our chart changes then depedencies.zot.version field should also be updated in tests/ci/*/Chart.yaml, otherwise the tests will fail. So we have to take care of that.
Pls correct me if I'm wrong.

Otherwise lgtm, I like this approach.

Kortekaasy · 2023-11-15T14:06:09Z

Hi @Andreea-Lupu, thanks for the kind words. Regarding your first comment, could you tell me which files? I don't have ct installed at the moment, so it's not very straightforward to test for me. Regarding your second comment, you're right. Maybe this could be solved with a pre-commit hook? I'm not sure what the right approach would be here. At least the CI-tests will notify you of the missed change.

Andreea-Lupu · 2023-11-15T18:04:35Z

Hi @Andreea-Lupu, thanks for the kind words. Regarding your first comment, could you tell me which files? I don't have ct installed at the moment, so it's not very straightforward to test for me. Regarding your second comment, you're right. Maybe this could be solved with a pre-commit hook? I'm not sure what the right approach would be here. At least the CI-tests will notify you of the missed change.

For example, ct install --charts tests/ci/default generates these new resources which should be ignored:
tests/ci/default
├── Chart.lock
├── charts
│ └── zot-0.1.35.tgz

so I think tests/ci/*/Chart.lock and tests/ci/*/charts should be added in a .gitignore file.
And for updating the version fields, I think we could do this manually, without a pre-commit hook.

@Kortekaasy could you please add the .gitignore file?

Signed-off-by: Yoep Kortekaas <[email protected]>

Kortekaasy · 2023-11-16T14:45:44Z

@Andreea-Lupu added

Andreea-Lupu

lgtm

andaaron

lgtm

rchincha · 2023-11-16T21:47:26Z

@Kortekaasy thanks for the PR!

Kortekaasy requested review from rchincha, andaaron, eusebiu-constantin-petu-dbk and Andreea-Lupu as code owners October 23, 2023 12:55

Kortekaasy force-pushed the main branch 3 times, most recently from df42dd2 to 9ebaf27 Compare November 9, 2023 09:40

test: create separate helm charts for each test case

cd41be0

Signed-off-by: Yoep Kortekaas <[email protected]>

Kortekaasy force-pushed the main branch from 9ebaf27 to cd41be0 Compare November 16, 2023 14:45

Andreea-Lupu approved these changes Nov 16, 2023

View reviewed changes

andaaron approved these changes Nov 16, 2023

View reviewed changes

rchincha merged commit 1fb0438 into project-zot:main Nov 16, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add helm test hook to test resources #25

test: add helm test hook to test resources #25

Kortekaasy commented Oct 23, 2023

Kortekaasy commented Oct 23, 2023

Kortekaasy commented Oct 30, 2023

Andreea-Lupu commented Oct 30, 2023

Kortekaasy commented Oct 31, 2023

rchincha commented Nov 1, 2023 •

edited

Loading

Kortekaasy commented Nov 2, 2023

rchincha commented Nov 9, 2023

Kortekaasy commented Nov 13, 2023

Andreea-Lupu commented Nov 15, 2023

Kortekaasy commented Nov 15, 2023

Andreea-Lupu commented Nov 15, 2023 •

edited

Loading

Kortekaasy commented Nov 16, 2023

Andreea-Lupu left a comment

andaaron left a comment

rchincha commented Nov 16, 2023

test: add helm test hook to test resources #25

test: add helm test hook to test resources #25

Conversation

Kortekaasy commented Oct 23, 2023

Kortekaasy commented Oct 23, 2023

Kortekaasy commented Oct 30, 2023

Andreea-Lupu commented Oct 30, 2023

Kortekaasy commented Oct 31, 2023

rchincha commented Nov 1, 2023 • edited Loading

Kortekaasy commented Nov 2, 2023

rchincha commented Nov 9, 2023

Kortekaasy commented Nov 13, 2023

Andreea-Lupu commented Nov 15, 2023

Kortekaasy commented Nov 15, 2023

Andreea-Lupu commented Nov 15, 2023 • edited Loading

Kortekaasy commented Nov 16, 2023

Andreea-Lupu left a comment

Choose a reason for hiding this comment

andaaron left a comment

Choose a reason for hiding this comment

rchincha commented Nov 16, 2023

rchincha commented Nov 1, 2023 •

edited

Loading

Andreea-Lupu commented Nov 15, 2023 •

edited

Loading