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

tests: introduce golden tests in kong chart using chartsnap #978

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

czeslavo
Copy link
Contributor

What this PR does / why we need it:

An alternative to #953 using the existing helm-chartsnap helm plugin that became useful for us thanks to its maintainer @jlandowner quickly fixing an issue that was a blocker for us: jlandowner/helm-chartsnap#30.

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • New or modified sections of values.yaml are documented in the README.md
  • Commits follow the Kong commit message guidelines

@czeslavo czeslavo self-assigned this Dec 21, 2023
@czeslavo czeslavo force-pushed the tests/introduce-golden-chartsnap branch 5 times, most recently from 5a0c3e8 to d8c436d Compare December 21, 2023 16:18
@czeslavo czeslavo marked this pull request as ready for review December 21, 2023 16:20
@czeslavo czeslavo requested a review from a team as a code owner December 21, 2023 16:20
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Make target seems a bit off? It looks like this was intended to only print the "failed" message if the command fails, but it's printing always, even though all the individual checks have PASSED and the update target results in no changes:

$ make test.golden       
helm chartsnap -c ./charts/kong -f ./charts/kong/ci/ || \
(echo ">> Golden tests have failed.\n>> Please run 'make test.golden.update' to update golden files and commit the changes if they're expected." && \
exit 1)

What all should trigger a failure? Are there certain things that have blanket exemptions even if we haven't explicitly ignored them?

From poking around a bit, it seems that some changes don't actually trigger one even though they're not in a dynamic field. For example, adding an annotation to the main Deployment's Pod template:

$ git diff
diff --git a/charts/kong/templates/deployment.yaml b/charts/kong/templates/deployment.yaml
index 28f9b06..a861edf 100644
--- a/charts/kong/templates/deployment.yaml
+++ b/charts/kong/templates/deployment.yaml
@@ -41,6 +41,7 @@ spec:
   template:
     metadata:
       annotations:
+        kuma.io/service-account-token-volume: "change!"
         {{- if (and (not .Values.deployment.serviceAccount.automountServiceAccountToken) (or .Values.deployment.serviceAccount.create .Values.deployment.serviceAccount.name)) }}
         kuma.io/service-account-token-volume: {{ template "kong.serviceAccountTokenName" . }}
         {{- end }}

and doing a basic default values helm template ana /tmp/symkong/ to compare output does yield a diff in the output:

$ diff -u /tmp/old /tmp/new

@@ -514,6 +514,7 @@
   template:
     metadata:
       annotations:
+        kuma.io/service-account-token-volume: "change!"
         kuma.io/service-account-token-volume: ana-kong-token
         kuma.io/gateway: "enabled"
         traffic.sidecar.istio.io/includeInboundPorts: ""

but still gets an All snapshots matched result. That annotation block is present in the snapshot, so I'm not quite sure of what to make of that.

Comparatively, flipping the Deployment/DaemonSet if condition does fail:

$ git diff
diff --git a/charts/kong/templates/deployment.yaml b/charts/kong/templates/deployment.yaml
index 28f9b06..091fbb4 100644
--- a/charts/kong/templates/deployment.yaml
+++ b/charts/kong/templates/deployment.yaml
@@ -1,6 +1,6 @@
 {{- if or .Values.deployment.kong.enabled .Values.ingressController.enabled }}
 apiVersion: apps/v1
-{{- if .Values.deployment.daemonset }}
+{{- if not .Values.deployment.daemonset }}
 kind: DaemonSet
 {{- else }}
 kind: Deployment
$ make test.golden       
helm chartsnap -c ./charts/kong -f ./charts/kong/ci/ || \
(echo ">> Golden tests have failed.\n>> Please run 'make test.golden.update' to update golden files and commit the changes if they're expected." && \
exit 1)
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/admin-api-service-clusterip-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/custom-labels-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/default-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/kong-ingress-1-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/kong-ingress-2-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/kong-ingress-3-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/kong-ingress-4-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/service-account.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/single-image-default-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/test-enterprise-version-3.4.0.0-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/test1-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/test2-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/test3-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/test4-values.yaml
 RUNS  Snapshot testing chart=./charts/kong values=charts/kong/ci/test5-values.yaml
 FAIL  Snapshot does not match chart=./charts/kong values=charts/kong/ci/admin-api-service-clusterip-values.yaml
Expected to match
--- kind=ConfigMap name=chartsnap-kong line=2
  - object:
      apiVersion: apps/v1
-     kind: Deployment
+     kind: DaemonSet
      metadata:
          labels:
              app.kubernetes.io/component: app

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Okay, the annotation not detecting was because I had a duplicate annotation name, which is not really allowed. Not sure what chartsnap's doing under the hood but I guess it runs it through some processing that processes the collision and chooses the last value (this is how the Kubernetes API handles them) before checking rather than just using raw template output. Kinda weird but whatever, I at least understand it now.

The instruction print is also not processing the newline properly. The output at the end (not the initial command echo) is still a single line:

>> Golden tests have failed.\n>> Please run 'make test.golden.update' to update golden files and commit the changes if they're expected.

So if we can sort that and suppress the earlier print we're good. I thought we could use something like

false
if [[ $? ]]
then
		echo "good"
fi

but alas, the wonders of screwy Make syntax to do something like that bash elude me.

@czeslavo czeslavo force-pushed the tests/introduce-golden-chartsnap branch from d8c436d to 2800838 Compare January 9, 2024 09:46
@czeslavo
Copy link
Contributor Author

czeslavo commented Jan 9, 2024

The test.golden makefile target was printing the command itself as there was no @ before it to suppress the output. With that added, it should no longer misguide with the "Golden tests have failed" in the positive scenario. Also, the newline should work now (it's been working for me without the changes, but apparently that might be an issue in some shells 🤷): 0dccf83

@czeslavo czeslavo force-pushed the tests/introduce-golden-chartsnap branch from 2800838 to 0dccf83 Compare January 9, 2024 09:53
@czeslavo czeslavo requested a review from rainest January 9, 2024 09:55
@rainest rainest enabled auto-merge (squash) January 9, 2024 18:35
@rainest rainest merged commit d5e1c3c into main Jan 9, 2024
24 checks passed
@rainest rainest deleted the tests/introduce-golden-chartsnap branch January 9, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants