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

Validator integration #44

Open
florianrusch opened this issue Dec 23, 2023 · 10 comments
Open

Validator integration #44

florianrusch opened this issue Dec 23, 2023 · 10 comments

Comments

@florianrusch
Copy link
Contributor

@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
Copy link
Contributor Author

florianrusch commented Dec 23, 2023

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
Copy link
Owner

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.

  • To capture all the outputs of Helm template command, chartsnap uses CombinedOutput(), which captures both stdout and stderr.
  • For handling dynamic values, it decodes the helm output as Kubernetes's struct Unstructured.
    And when the decode fails, it is handled as a kind: Unknown, which is a custom kind in chartsnap (sorry but this has not been documented yet)
  • To expedite development, the current code about snapshot files was reused from my previous project.

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

florianrusch commented Jan 9, 2024

@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
Copy link
Owner

@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
Copy link
Owner

Supported output as YAML in #85 😊

@jlandowner
Copy link
Owner

jlandowner commented May 14, 2024

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

#113 (comment)

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

スクリーンショット 2024-05-14 23 45 44

@jlandowner jlandowner reopened this May 14, 2024
@florianrusch
Copy link
Contributor Author

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
Copy link
Owner

jlandowner commented May 14, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants