-
Notifications
You must be signed in to change notification settings - Fork 91
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
Add some basic unit tests and update CI workflow #218
Add some basic unit tests and update CI workflow #218
Conversation
I imagine the build failing has something to do with me not having access to repository / organization secrets. As for the unit test job, while it claims to be passing, the logs suggest my changes to the workflow weren't applied for this run, since it still uses |
thank you @d3adb5 for such a thorough PR! we are fixing the pipelines runtime and then we would really appreciate more PRs which can improve the readability of the chart and have more unit tests; we are also thinking to add integration tests |
Sure thing! I'd be happy to do some refactoring and security touch ups soon! Do you guys know what's going on with the CI builds, @rasheedamir? Maybe I could help with that effort. |
@d3adb5 the pipelines are fixed now! can you please update this PR or create new one? now we are using self hosted github runners on full blown openshift cluster to ensure stuff works |
4e430df
to
2c41d7a
Compare
Seems like plenty changed in the repository. Well, I rebased my changes and hopefully everything's working fine. A careful review would be much appreciated! Thanks, @rasheedamir. |
@d3adb5 validation successful` |
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, thank you @d3adb5 ! Please fix conflicts so we it can land asap :)
Set the API version to v2 in Chart.yaml, as Helm v2 should no longer be supported, considering its EOL was November of 2020. Users of Helm v2 should update to Helm v3, as security patches are no longer released.
Change the URL to the correct repository in Chart.yaml.
Fix the test suite for the Deployment template so that it is actually executed by helm-unittest and does not rely on default chart values.
Update to v2 of d3adb5/helm-unittest-action for running tests with the latest version of helm-unittest.
Add unit tests for the HorizontalPodAutoscaler template.
Use the value of deployment.enabled instead of simply deployment in the condition for rendering the HorizontalPodAutoscaler template.
Use the autoscaling/v2 API version when it is advertised as available by the Kubernetes cluster the release is being deployed to.
Adjust indentation, whitespace gobbling, and trailing whitespace in the HorizontalPodAutoscaler template. The 'with' keyword was used to avoid repetition when referencing values, as well.
Add unit tests for the ServiceAccount template.
Adjust indentation, spacing, and line lengths for the ServiceAccount template. A partial template was added to both shorten and make more manageable the JSON passed to OpenShift as an annotation.
Add unit tests for the PersistentVolumeClaim template.
Add unit tests for the PodDisruptionBudget template.
Adjust indentation, whitespace, and minor templating on the Go template side to improve readability for the PodDisruptionBudget template.
2c41d7a
to
e039f61
Compare
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 🙏
{{ if .Capabilities.APIVersions.Has "autoscaling/v2/HorizontalPodAutoscaler" -}} | ||
{{- if and .Values.autoscaling.enabled .Values.deployment.enabled }} | ||
|
||
{{- if .Capabilities.APIVersions.Has "autoscaling/v2/HorizontalPodAutoscaler" }} |
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.
Thank you for updating these check to GVK instead of GV. I'm deploying most of my apps with ArgoCD and had lot of pain previously with charts not comparing with the full GVK. See argoproj/argo-cd#7291
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.
You can thank the person behind #221 for that instead! I was originally using GV. 👀
Is it okay to merge this, @rasheedamir? |
Hi there! Noticed you guys were using the GitHub action I wrote for running Helm
template unit tests, and decided to come check out your repository. I noticed
the test suite added by @TehreemNisa in #214 wasn't actually running due to the
filename used, and decided to fix that.
I realize this is a somewhat large first PR, but I figured it was worth adding
some unit tests and refactoring some of the existing templates to improve
readability and maintainability.
The CI workflow was updated to use v2 of my unit test action. This version
points to the now official repository, taken over by the plugin maintainer.
Here is a breakdown of the changes, as a list of commits:
If this pull request is accepted and merged, I'd be happy to implement more unit
tests and improve readability for the chart overall.