-
Notifications
You must be signed in to change notification settings - Fork 16.8k
Enable testing charts with test values #4157
Conversation
73ffb51
to
a1a8820
Compare
0a57940
to
569ca27
Compare
/retest |
f6039c8
to
4c08467
Compare
I added a simple chart with a test that just logs a value. The You may want to review the commits separately. The actual change in logic is in the second commit. cc @kubernetes/charts-maintainers @kubernetes/helm-maintainers |
4c08467
to
5783d09
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.
These are my quick nits and questions. I'll do a little more testing. I generally like the direction.
.editorconfig
Outdated
charset = utf-8 | ||
|
||
[*.{yml,yaml}] | ||
indent_size = 2 |
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.
While I'm a fan of editorconfig to bring about consistency, is there a reason it's in this PR? I ask because it doesn't seem to fit everything else.
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.
I agree. This is unrelated. I'll remove it.
"ok-to-test" label. This can also be done by commenting "/ok-to-test" on the pull request. | ||
In order for the tests to be kicked off one of the | ||
[Kubernetes member](https://github.com/orgs/kubernetes/people) must add the | ||
"ok-to-test" label. This can also be done by commenting "/ok-to-test" on the pull request. |
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.
The tests are triggered automatically when someone is a member of the Kubernetes GitHub org or when a member adds the like /ok-to-test
is a comment to tell the bot it's ok. Could we get the docs updated to reflect. both cases?
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.
Not related to this PR, but I'll change that.
test/changed.sh
Outdated
|
||
# Exit early if no charts have changed | ||
if [ -z "$CHANGED_FOLDERS" ]; then | ||
exit 0 | ||
if [[ -z "$changed_folders" ]]; then |
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.
I noticed you changed the casing on variables in the shell script. If I understand the k8s practice, which I could be wrong about, uppercase variables are for those which are not mutated (essentially constants) and lowercase variables are the ones that change (e.g., in loops). It might be good to follow that convention. Thoughts?
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.
I'm all for conventions. In any case the casing was a mixture and not clear as it was. To me, a constant is something that is explicitly initialized as such. It should then probably be made readonly
as well. The question is whether any variable qualifies as a constant in its actual sense just because it is never changed after it was assigned. According to Google's Shell Style Guide, a constant should be declared at the top of the file. I would agree with that and would change the casing back where this applies.
You can see in the log that clean-up happens. I bet these are leftovers from my tests when clean-up failed in some cases. |
88dfdd7
to
9b153d5
Compare
9b153d5
to
72752a8
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattfarina, unguiculus 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 |
* upstream/master: (944 commits) Rename service port to http (helm#4442) [stable/neo4j] Change the image of the initContainer examples (helm#4269) move burrow to stable repo (helm#3481) Upgrade kube-state-metrics to 1.2.0, add new collectors (helm#4146) Add review guidelines around pvcs (helm#4223) [stable/parse] Release 0.3.10 (helm#4389) [stable/phabricator] Release 0.5.19 (helm#4433) Support exposing jmx and additional ports (helm#4072) Add default of "" for string comparison (helm#4420) [incubator/kafka] Makes readiness probe configurable (helm#3948) Published stash chart 0.7.0-rc.1 (helm#4410) Enable testing charts with test values (helm#4157) [incubator/kafka] Fix initContainer failure which did not error (helm#4400) [stable/etcd-operator] deployment typos and add tolerations (helm#4139) Typo fix in coscale/README.md (helm#4306) Typo fix in concourse/README.md (helm#4303) Typo fix in cockroachdb/README.md (helm#4302) [stable/jenkins] Bump appVersion (helm#4177) Typo fix in cluster-autoscaler/README.md (helm#4301) [stable/traefik] Bump appVersion to 1.5.4 (helm#4206) ...
* Refactor and fix shellcheck issues * Make constants readonly and uppercase * Remove .editorconfig * Enable testing charts with test values * Remove dummy chart
* Refactor and fix shellcheck issues * Make constants readonly and uppercase * Remove .editorconfig * Enable testing charts with test values * Remove dummy chart
* Refactor and fix shellcheck issues * Make constants readonly and uppercase * Remove .editorconfig * Enable testing charts with test values * Remove dummy chart Signed-off-by: voron <av@arilot.com>
What this PR does / why we need it:
Enables testing charts with test values.
changed.sh
. I ran Shellcheck across it and applied fixes. Some variable were upper, some were lower case. I made that consistent: lower case for script variables, upper case for environment variables.changed.sh
such that the script installs and tests the chart for any*-values.yaml
found in theci
folder within the chart's directory.Which issue this PR fixes:
Fixes #3943