search

yannh / kubeconform

A FAST Kubernetes manifests validator, with support for Custom Resources!
Apache License 2.0
2.15k stars 121 forks source link

refactor: expose Kubeconform as a module #189

Open aabouzaid opened 1 year ago

aabouzaid commented 1 year ago

Hi again :wave:

As mentioned in PR no. #170, instead of dealing with KRM inside Kubeconform, it's better to expose it as a module and use it externally.

In the PR:

Fixes: #160

Best Regards.

yannh commented 1 year ago

Hi @aabouzaid , Kubeconform can be used as a module, there is an example here: https://github.com/yannh/kubeconform/blob/master/examples/main.go - would it work for you? Your PR is pretty extensive and I'm not sure I agree with all changes... Congrats on shipping though :clap: !!

aabouzaid commented 1 year ago

@yannh thanks for the link, but it's only the Kueconform individual packages that are exposed, not the app itself. A big chunk of the code in main is not exposed.

That means everyone who wants to use Kueconform needs to implement the same execution logic that uses the underneath Kubeconform packages. This is a huge redundancy IMO (I'm not sure what is the actual reason not to expose the app but only its libs).

Also, for example, the config is exposed, but it doesn't have any YAML or JSON tags, which also means extra work if someone wants to extend Kubeconform.

So given that I don't try to use Kubeconform in my app but actually writing an interface to use Kubeconform, it's not even helpful to reimplement what you already did (KubeconformValidator is just a wrapper for Kubeconform).

All changes here are minimal to make Kubeconform app reusable, so if anything is not clear, I'd be happy to clarify the reasoning behind the changes (and even remove some of the changes if they don't fit for one reason or another).

Thank you :bow:

aabouzaid commented 1 year ago

@yannh I hope that's better :bow: The PR doesn't change any existing behaviour.

yannh commented 1 year ago

Quite a lot to like about that PR thanks :bow: Can you expand on this "NGConfig", why that is interesting? Have a few nits here and there, but I can see where you're going. Adding a few comments for discussion in the code.

aabouzaid commented 1 year ago

Quite a lot to like about that PR thanks bow Can you expand on this "NGConfig", why that is interesting? Have a few nits here and there, but I can see where you're going. Adding a few comments for discussion in the code.

The idea of the NG config is to overcome the diversion between the input of Kubeconform and the actual data structure in the Config struct.

Let's take Config.SkipKinds as an example. The field SkipKinds represents -skip CLI argument. -skip takes a "comma-separated list of kinds or GVKs to ignore", which simply should be presented as a list of string []string, however, it's presented as map[string]struct{}.

I've read most of the code, and it's not clear to me why that data type is used if the skipped items don't have extra config. the only thing I can think about is that it's just used to make it easy to check with the if condition since Golang doesn't provide a method to check if an item is in an array, but a smart way to make it is to use a map!

Here is an example of what I mean: https://github.com/yannh/kubeconform/blob/e3bb34851d02b04671be2792153eb2eff5c05c56/pkg/validator/validator.go#L118

So what is the problem here? the problem is that the data structure will look odd in YAML:

skip:
  ConfigMap:
  Service:
  Deployment:

which is confusing if anyone looked at the arg equivalent, which is -skip "ConfigMap,Service,Deployment".

The idea of NG (new generation) config is to bridge the new with the old format so skip could be presented in YAML as:

skip:
- ConfigMap
- Service
- Deployment

This will work out of the box, it will not introduce any breaking changes, and a deprecation note could be added. Later after a couple of versions, the old method would be removed, and the new one would be used by default.

If anything is not clear, feel free to mention it, I'd be happy to add more details,

yannh commented 1 year ago

I'll start committing parts of these to main in small commits, I ll put you as co-committer :bow: Not impossible this will trigger a few conflicts though, but it is easier for me to do in small chunks.

yannh commented 1 year ago

I do get the feeling behind the array. The reason it was a map was for easy lookups in here. https://github.com/yannh/kubeconform/blob/8bc9f42f39e99681e88baea78a4564ddb83a78de/pkg/validator/validator.go#L117 . There should be no need for backward compatibility at the moment.