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

restore helm unittests #118

Merged
merged 13 commits into from
Sep 29, 2024
Merged

restore helm unittests #118

merged 13 commits into from
Sep 29, 2024

Conversation

MichaelSp
Copy link
Contributor

For some reason the unit-tests are quite outdated and are not working anymore. This PR restores the unittests and provides a GitHub Actions workflow to run it on PRs

@MichaelSp MichaelSp requested a review from funkypenguin as a code owner May 26, 2024 12:34
@cfis
Copy link
Collaborator

cfis commented May 26, 2024

Cool - happy to have a PR that fixes unit tests, but this PR also includes other additional changes. Could you separate those out?

Also, what does the __snapshot__ directory do and why the snap file extension after yaml?\

@MichaelSp
Copy link
Contributor Author

MichaelSp commented May 28, 2024

Additional changes? You mean fixes in deployment.yaml and typos in values.yaml?

the snapshots will be the generated version that defines the current truth. If you plan to change anything that would change the output, the test will fail because it might be a regression. You can now review the changes in the output of the helm untittest run and decide if you want it or not. If you want to continue with the changes you can run helm unittest -u to update the snapshots. The files will also be commited in to the repo. The yaml.snap files are auto-generated by the tests.
All of that is documented in https://github.com/helm-unittest/helm-unittest

@cfis
Copy link
Collaborator

cfis commented May 28, 2024

Yes, I would like an PR that only contains the changes needed for updating the unit tests. Other changes we can talk about in separate PRs.

Thanks for the explanation on snapshots, that sounds good.

@cfis
Copy link
Collaborator

cfis commented Jun 25, 2024

Just following up - would like to get this merged.

@cfis
Copy link
Collaborator

cfis commented Jul 9, 2024

Ping?

@polarathene
Copy link
Member

Sorry this is a bit off-topic, but just curious @MichaelSp , is SAP using DMS? :)

@MichaelSp
Copy link
Contributor Author

Sorry for the delay here. I couldn't find the time yet to split the PR and timewise it doesn't seem to get any better... (maybe you can accept it in one PR?)

@polarathene AFAIK they are not using DMS 🤣

@cfis
Copy link
Collaborator

cfis commented Jul 10, 2024

@MichaelSp understood - if I find some time I'll see what I can do.

@MichaelSp
Copy link
Contributor Author

Finally I was able to polish this PR a bit. One thing I noticed:

readonly is not a valid param according to the spec https://kubernetes.io/docs/concepts/storage/volumes/. The correct param would be readOnly. I assume this field is ignored, but since I assume the state should not be readOnly, I took the liberty to remove that.

@MichaelSp
Copy link
Contributor Author

@cfis pong :)

@cfis
Copy link
Collaborator

cfis commented Sep 29, 2024

Oh sorry, totally missed your last update. Thanks for updating!

@cfis cfis merged commit 0ad4d47 into docker-mailserver:master Sep 29, 2024
5 checks passed
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