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

chore(helm): add hook-delete-policy for Helm chart tests #480

Conversation

fty4
Copy link
Member

@fty4 fty4 commented Jun 14, 2023

WHAT

Adding Hook deletion policies for the Helm tests.

WHY

When running tests with Helm the pods will not be tidied up.
When the annotation "helm.sh/hook-delete-policy": hook-succeeded is added, after a successful run the pod will be deleted afterwards.
If the pod fails it will still be present.

Also the value before-hook-creation was added to be able to run the test again if it failed without manually deleting the pod.


Marco Lecheler [email protected] Mercedes-Benz Tech Innovation GmbH (ProviderInformation)

@fty4 fty4 added the helmcharts Anything that has to do with helm charts label Jun 14, 2023
@fty4
Copy link
Member Author

fty4 commented Jun 14, 2023

Test is failing because test-pod is cleaned up as expected when successful and not longer present:

# execute the helm test
helm test tx-prod --logs

@fty4
Copy link
Member Author

fty4 commented Jun 15, 2023

The command should not be the problem - there is a bug that --logs will not act as expected:

helm/helm#10603

For the ci tests the deletion will be disabled.

Currently Helm will fail if the hook is set when using flag `--logs`
see: helm/helm#10603
@paullatzelsperger
Copy link
Contributor

The command should not be the problem - there is a bug that --logs will not act as expected:

helm/helm#10603

I tried this on my local machine, and there it works, but if removing the --logs flag solves it on CI, that would be fine. can you remove the flag in the workflow file, adding a comment about the helm bug and the link you posted?

@fty4
Copy link
Member Author

fty4 commented Jun 15, 2023

Sure then i would revert the last commit which adds the tests.hookDeletePolicy value in 7a832c7 to keep the value file more clean, right?

I thought removing the --logs isn't an option to preserve potential error messages.

@paullatzelsperger
Copy link
Contributor

Sure then i would revert the last commit which adds the tests.hookDeletePolicy value in 7a832c7 to keep the value file more clean, right?

I thought removing the --logs isn't an option to preserve potential error messages.

if i understand this right we have two options: to not delete the test pods, or to not have logs, right?

@fty4
Copy link
Member Author

fty4 commented Jun 15, 2023

Right - here both options compared:

1. Option: persist test-pods for test workflow (current implementation of the PR)
With the last commit the workflow succeeds.
Here the Helm default will be overwritten to delete the test-pod after a successful run.
But because the workflow failed I have overwritten it again in test values back to the Helm defaults:

see: edc-tests/deployment/src/main/resources/helm/tractusx-connector-test.yaml

This would work and the pod will not be deleted in the test infrastructure which is no problem (will be deleted anyway).

+ Logs of failed pods in test workflow
- Additional values required in values.yaml and tractusx-connector-test.yaml

2. Option: no logs for test workflow
When removing the --logs flag the workflow would also succeed but in case of an error the log is not present in the workflow logs due the mentioned bug.

+ No additional values required
- No logs of failed pods in test workflow


I think I would prefer option 1.

@sonarcloud
Copy link

sonarcloud bot commented Jun 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@paullatzelsperger
Copy link
Contributor

paullatzelsperger commented Jun 15, 2023

Additional values required in values.yaml and tractusx-connector-test.yaml

would that mean, if someone forgot to add those values to their xyz-values.yml, they'd still end up with zombie test pods?

@fty4
Copy link
Member Author

fty4 commented Jun 15, 2023

would that mean, if someone forgot to add those values to their xyz-values.yml, they'd still end up with zombie test pods?

No the test-pods will be removed because in the default values.yaml
file the policy is set to "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded which removes succeeded test-pods.
Failed will still be available - because you want to see the error messages here therefore no deletion.

If you want all to persist test-pods you can adapt to the tractusx-connector-test.yaml test values file.

Without this PR the test-pods will not be removed after successful testing.

@paullatzelsperger
Copy link
Contributor

Without this PR the test-pods will not be removed after successful testing.

ok, that sounds reasonable, thanks for the clarification.

@paullatzelsperger paullatzelsperger merged commit 14b5e8d into eclipse-tractusx:main Jun 15, 2023
@fty4 fty4 deleted the chore/helm-test-deletepolicy branch June 15, 2023 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helmcharts Anything that has to do with helm charts
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

2 participants