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.

Sign up for GitHub

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

Jump to bottom

feat(chart): add helm-unittest framework #5137

Merged
merged 14 commits into from
Mar 4, 2025
Merged

feat(chart): add helm-unittest framework #5137

merged 14 commits into from
Mar 4, 2025

Conversation

ivankatliarchuk
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk commented Mar 2, 2025 •
edited
Loading

Description

Fixes #4989
Fixes #3960
Fixes #4997

Note; this is not a silver bullet for all chart/helm related issues

Where helm unit-testing excel

Cons:

  • One to know how to write test for helm templates
  • Require a decent coverage, and workflow when new changes added to helm chart

If approach is acceptable, in follow up I'll add more coverage

Checklist

  • Unit tests updated
  • End user documentation updated

@ivankatliarchuk
feat(chart): add helm-unittest framework
1f3108e 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 2, 2025
@ivankatliarchuk ivankatliarchuk changed the title feat(chart): add helm-unittest framework WIP feat(chart): add helm-unittest framework Mar 2, 2025
@ivankatliarchuk ivankatliarchuk marked this pull request as draft March 2, 2025 21:11
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 2, 2025
@ivankatliarchuk
feat(chart): add helm-unittest framework
21aa5fb 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 2, 2025
@ivankatliarchuk
feat(chart): add helm-unittest framework
afc6782 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk
feat(chart): add helm-unittest framework
762fc1c 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk
Copy link
Contributor Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 2, 2025
@ivankatliarchuk
feat(chart): add helm-unittest framework
c1b1d2a 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk
feat(chart): add helm-unittest framework
7f229d3
@ivankatliarchuk
feat(chart): add helm-unittest framework
dc93cd6 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk
feat(chart): add helm-unittest framework
c19a786 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk
feat(chart): add helm-unittest framework
72c7597 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2025
@ivankatliarchuk
feat(chart): add helm-unittest framework
de24416 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk
feat(chart): add helm-unittest framework
83bc63c 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk
feat(chart): add helm-unittest framework
7b9fd47 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk ivankatliarchuk changed the title WIP feat(chart): add helm-unittest framework feat(chart): add helm-unittest framework Mar 3, 2025
@ivankatliarchuk ivankatliarchuk marked this pull request as ready for review March 3, 2025 09:28
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 3, 2025
@ivankatliarchuk
Copy link
Contributor Author

Hi @stevehipwell wdyt?

@ivankatliarchuk
Merge branch 'master' into issue-4989-add-helm-unit-test
4e989ab 
* master:
  feat(aws): always create AAAA alias records in route53 (kubernetes-sigs#5111)
  feat(aws): fetch zones with tags batching (kubernetes-sigs#5058)
stevehipwell
stevehipwell reviewed Mar 3, 2025
View reviewed changes
Copy link
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivankatliarchuk this looks good, my only reservation is that if unit tests are added then they will need to be maintained in step with the chart. I'm not sure I want to take on any additional work around this.

Also I think you need to add the tests to the .helmignore file so they don't get published.

@ivankatliarchuk
Copy link
Contributor Author

ivankatliarchuk commented Mar 3, 2025 •
edited
Loading

100% agree, they need to be maintained.

What I will try to do, in follow-ups I'll add few more tests, to cover as much as possible lines

I could take care of unit-testing, probably @mloiseleur familiar with framework too, if you ok with it?
Is this arrangement will work?

@ivankatliarchuk
feat(chart): add helm-unittest framework
404dd58 
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@stevehipwell
Copy link
Contributor

@ivankatliarchuk as long as there are people willing to maintain this I'm happy. @mloiseleur what do you think?

@mloiseleur
Copy link
Contributor

@stevehipwell I'm more than happy to have unit test on the Chart.

@stevehipwell
Copy link
Contributor

Thanks @ivankatliarchuk, you'll need one of the full maintainers to approve this as it has non chart changes.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2025
@mloiseleur
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mloiseleur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2025
@k8s-ci-robot k8s-ci-robot merged commit 5f26223 into kubernetes-sigs:master Mar 4, 2025
14 checks passed
ivankatliarchuk added a commit to gofogo/k8s-sigs-external-dns-fork that referenced this pull request Mar 6, 2025
@ivankatliarchuk
Merge branch 'master' into chore/added-renovate
a4e72e1 
* master: (198 commits)
  fix(aws-sd): service instances registration and deregistration (kubernetes-sigs#5135)
  chore(docs): generate docs/monitoring/metrics.md file (kubernetes-sigs#5117)
  feat(chart): add helm-unittest framework (kubernetes-sigs#5137)
  feat(chart): add helm-unittest framework
  feat(aws): always create AAAA alias records in route53 (kubernetes-sigs#5111)
  feat(aws): fetch zones with tags batching (kubernetes-sigs#5058)
  docs: openwrt webhook (kubernetes-sigs#5132)
  docs(proposal): ipv6 internal node ip rollback plan (kubernetes-sigs#5081)
  docs(proposal): update date format
  chore(deps): bump the dev-dependencies group across 1 directory with 7 updates
  Update README.md with proper link to dev guide
  Add OpenStack Designate webook provider to readme
  chore(deps): bump the dev-dependencies group with 3 updates
  chore(deps): bump the dev-dependencies group with 20 updates
  chore(deps): bump azure/setup-helm in the dev-dependencies group
  style: formatting
  fix: remove broken test
  fix test name
  chore: upgrade ExternalDNS to go 1.24
  chore-makefile-coverage
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szuecs szuecs Awaiting requested review from szuecs

@stevehipwell stevehipwell Awaiting requested review from stevehipwell

Assignees

@stevehipwell stevehipwell

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
4 participants
@ivankatliarchuk @stevehipwell @mloiseleur @k8s-ci-robot