-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Introduce Helm Chart unit tests #10192
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
Introduce Helm Chart unit tests #10192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @maciej-tatarski. I think it looks good, I have just some minor comments:
- Maybe you can link it from the main
Makefile
? Add something like this: https://github.com/strimzi/strimzi-kafka-operator/blob/main/Makefile#L170-L171 for thehelm_unittest
target as well? - I appreciate the GitHub workflow. But I think we will want to plug it into our Azure pipelines workflow to have it run also during releases etc. So I think you can remove the GitHub workflow for it. I'm happy to add it to the Azure pipeline myself in a follow up PR.
- I think the initial tests are completely fine. It is hard to add tests for everything out of the box. But once we have it, it will be much easier to add tests for any new features.
- Could you please add some note about the installation of the Helm plugin to https://github.com/strimzi/strimzi-kafka-operator/blob/main/development-docs/DEV_GUIDE.md#build-pre-requisites for any new contributors? I think just mentioning the plugin and sharing the installation command should be sufficient.
Thanks!
/azp run helm-acceptance |
Azure Pipelines successfully started running 1 pipeline(s). |
@scholzj I applied all the suggestions :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
/azp run helm-acceptance |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot 🙂!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Not an Helm user/expert but the tests seem to make sense to me :-) Thanks!
Thanks for the PR @maciej-tatarski |
Signed-off-by: Pandurang Khandeparker <[email protected]>
Type of change
Description
Adding unit tests for helm charts.
Checklist