search

krisnova / naml

Convert Kubernetes YAML to Golang
Apache License 2.0
1.26k stars 37 forks source link

Codify doesn't deal well with unknown resource types #105

Closed larsks closed 2 years ago

larsks commented 2 years ago

I tried running naml codify on an ObjectBucketClaim resource:

apiVersion: objectbucket.io/v1alpha1
kind: ObjectBucketClaim
metadata:
  name: example-obc
spec:
  generateBucketName: example-obc
  storageClassName: openshift-storage.noobaa.io

I was expecting it to fail due to an unknown resource type, but to my surprise it didn't emit any errors. The resulting main.go has no resources (and does not compile).

It would be nice if naml codify would emit an error (and fail) when it is unable to successfully convert the input.

krisnova commented 2 years ago

This one is hard.

So naml codify was never originally intended to guarantee anything, and (at least for right now) we are intentionally ignoring CRDs like this

The feature was intended to be just a "good first step" to helping teams get away from yaml. In other words - expecting folks to be able to fix things like compile time issues after code is generated is kind of... well... that was kind of the idea?

The problem comes in where we have multiple resources. If we have 10 resources, and we support 9 of the 10, what should we do? Does the program error? Or do we just get the user the best thing we can? For our original use case of helping teams get away from YAML, I think 9/10 is better than 0/10 if that makes sense?

larsks commented 2 years ago

If we have 10 resources, and we support 9 of the 10, what should we do?

How about emitting a warning on stderr that you've encountered an unknown resource type and are ignoring it? That way you still process 9/10 resources, but you also avoid surprising the person running codify, which seems like it's worth something.

I haven't looked close at the code yet, but that sounds like it should be a relatively simple change; if I were to submit that as a PR would you accept it?

krisnova commented 2 years ago

but that sounds like it should be a relatively simple change

It is!

if I were to submit that as a PR would you accept it?

Yes! I would love the help! Please open as many PRs as you would like, and feel free to vet them here in the issue tracker as well. Happy to let you contribute as much as you want.

Unfortunately, I already coded this before I read your response.

You can look at #107

If you want to give me a 👍🏻 I would love your review before I merge it and cut a new release.

krisnova commented 2 years ago

If you want to give https://github.com/kris-nova/naml/releases/tag/v1.0.3 a try it should do what you are looking for 🙂