jlandowner / helm-chartsnap

Snapshot testing tool for Helm charts
MIT License
55 stars 7 forks source link

Validator integration #44

Open florianrusch opened 9 months ago

florianrusch commented 9 months ago

@jlandowner thanks for this amazing tool!

I was looking for exactly something like this, because I work in a project with multiple different developers, which all have different levels of experiences with helm and how to write and modify helm charts.

I'm just wondering, if it's somehow possible to also integrate a validator tool to validate the generated yaml code against the kubernetes api. Did you or someone else already thought about something like that? It would be really easy, if the snapshots would be stored as plain yaml files.

florianrusch commented 9 months ago

One possibility I found so far is to use the --output-dir option of the helm template command. One downside of this is that you only can execute one test, else the last one are overriding each other. And the diff is not the right one.

$ helm chartsnap -c my-chart/ -u -- --output-dir ./foo/test
 RUNS  Snapshot testing chart=my-chart/ values=
time=2023-12-23T13:44:58.450+01:00 level=INFO msg="loading helm output is done with warning"
WARN: failed to recognize a resource. snapshot as Unknown: 
---
object:
    apiVersion: helm-chartsnap.jlandowner.dev/v1alpha1
    kind: Unknown
    raw: |+
        wrote ./foo/my-chart/templates/cronjob.yaml

---
 FAIL  Snapshot does not match chart=my-chart/ values=
Expected to match
--- kind= name= line=1
  - object:
-     apiVersion: batch/v1
-     kind: CronJob
-     metadata:
         ...
+     apiVersion: helm-chartsnap.jlandowner.dev/v1alpha1
+     kind: Unknown
+     raw: |+
+         wrote ./foo/my-chart/templates/cronjob.yaml
+ 
jlandowner commented 9 months ago

Hi @florianrusch, Thank you for raising!

Could you provide more details about the integration of a varidator tool?

integrate a validator tool to validate the generated yaml code against the kubernetes api.

Basically, all that is needed for Helm is the chart and values. Therefore, for integration testing, I thought you can use the test values files to e.g. helm upgrade --dry-run, instead of using snapshot files. This is one of the reasons why chartsnap uses the pure values.yaml file as test spec.

So it was not intended for the snapshot files to integrate with other tools for now.

Also some implementation reasons led to the current snapshot file format.

I'm open to supporting a better format and appriciate the suggestion 🙌 I would like to understand the plain yaml format you expected more clearly.

florianrusch commented 9 months ago

Hi, thanks for your reply!

I don't have a specific tool in mind, but for example kubectl validate would require the yaml resources to validate against the official schema: https://github.com/kubernetes-sigs/kubectl-validate/#usage

There are also a lot of other tools that would require yaml resources. You can find a good list here: https://github.com/HighwayofLife/kubernetes-validation-tools

So the idea would be to use helm-chartsnap to generate the resources in yaml format and do a first validation (snapshot test). Then we could reuse the yaml output as input for the other tools.

jlandowner commented 9 months ago

Thank you for more information!

As I mentioned above, chartsnap captures stdout and stderr and unknown output as kind: Unknown. Currently this seems to be a hurdle even if snapshot file is a standard YAML.

I will look into them and try to find how chartsnap can be integrated with them(including using test values files, not snapshot file)

florianrusch commented 9 months ago

@jlandowner thanks for your reply! I'm looking at this right now too. Basically, the bottom change would be to swap out the toml en/decoder with a yaml en/decoder. But I still have the following in every snapshot file:

default:
  snapshot: |
    - object:
        apiVersion: batch/v1
        kind: CronJob
        metadata:
        ...
    - object:
        apiVersion: ...

What I try to archive would be something like this:

apiVersion: batch/v1
kind: CronJob
metadata:
...

---

apiVersion: ...

while looking through the code I came across the SnapshotId/SnapId, but I don't really understand what the purpose of it is. Could you help me understand it?

EDIT: I have already solved the puzzle myself. It's needed for the tests, isn't it? Are there any other use cases apart from the tests?

jlandowner commented 9 months ago

@florianrusch Thanks for looking into my codes. The codes about snapshot files (pkg/snap) was reused from my previous project. It can take multiple snapshots in a single snapshot file so it requires snapshot id to recognize a snapshot in a file. You seemed to find it in test code. https://github.com/jlandowner/helm-chartsnap/blob/main/pkg/snap/__snapshots__/snapshot_test.snap#L15

But for chartsnap, a single snapshot(=a test values file, set of output of helm template) is stored in a single snapshot file so it does not so matter. It's just the name of a test values file(default if not) and you can find it in the first line of snapshot files. https://github.com/jlandowner/helm-chartsnap/blob/main/example/app1/__snapshots__/default.snap#L1

jlandowner commented 6 months ago

Supported output as YAML in #85 😊

jlandowner commented 4 months ago

I found snapshot file extention should be changed from .snap to .yaml in order to integrate with kubectl-validate

https://github.com/jlandowner/helm-chartsnap/issues/113#issuecomment-2110434846

Then we can validate all the snapshots by a single command👍

スクリーンショット 2024-05-14 23 45 44
florianrusch commented 4 months ago

Personally, I like the '.snap' file extensions. It makes clear, that these files are not normal k8s resource yaml files and that they contain snapshots.

Maybe we can think of adding an additional suffix like .snap.yaml or .snapshot.yaml.

WDYT?

jlandowner commented 4 months ago

I agree. Actually I am using .snap.yaml in the screenshot. Also, since the base name of the snapshot file is the same as the values file, it would be confused.

florianrusch commented 2 months ago

@jlandowner is this still a todo?

jlandowner commented 2 months ago

Absolutely, but I couldn't work on it these days😓