-
Notifications
You must be signed in to change notification settings - Fork 485
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
Add plugin: kubeval #259
Add plugin: kubeval #259
Conversation
Signed-off-by: Nicolas Lamirault <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nlamirault The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
plugins/kubeval.yaml
Outdated
 name: validate-manifests | ||
spec: | ||
 version: "0.14.0" | ||
 shortDescription: Validate your Kubernetes configuration files, supports multiple Kubernetes versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supports multiple Kubernetes versions
part is redundant for a "short" description, it's better to actually keep it short.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also use the description
field if necessary.
Normally we don't accept plugins with kube* prefix, but since this is a well-established tool it qualifies for an exception to preserve its name. However, this also might be an opportunity for you to rename and distribute your tool as I also recommend making sure the help (-h/--help) message for |
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
@nlamirault what’s your take on the naming? Should we keep the existing name fi it's well-recognized, or do you want your users to be primarily invoking it like |
we can ask the author of the plugin what he thinks ? |
@garethr Have you seen the PR regarding |
Mmm, turning it into a general question about Krew index opinions again :)
For that reason, I'd suggest plugins carry the name of the underlying tool. That also makes support less surprising. If kubeval is hidden then users won't find it easy to file issues in the right place. But I'm of the view the Krew index should be less opinionated I think. |
Regarding naming:
Regarding distributing as a plugin: kubeval already has packaging for Windows, Linux, macOS. Why does it make sense to add yet another distribution (krew)? At this point, I don't think it even makes sense to stop distributing with existing methods and use Krew. That would leave existing users behind. Also, krew helps plugins which haven't created distributions for multiple OSes yet, but kubeval is already there. Does kubeval benefit from being a krew plugin? Maybe the users can benefit, as they can declaratively install most of their kubernetes client-side tooling via krew. But overall, I'm not even sure if that's a huge benefit. |
From a users perspective, discovery and central management are useful in my view. That's normally the whole point of a package manager. See Homebrew as a good example. I think the Krew project needs to make clear it's not intending to be a generic package manager for Kubectl plugins, rather it's a very opinionated distribution of Kubectl plugins. I think that's fine, but I do think that leaves a gap in the market for a registry/tool for discovering/ managing any and all kubectl plugins. At least until/unless Krew adds the ability to have additional search paths (at which point I'd be happy to create the equivalent of a Homebrew tap) for this plugin definition. |
So if I understand both of you correctly, you both agree (for maybe different reasons) to not include |
@corneliusweig I'd be happy to see it included under |
@garethr Yeah we can release it as a plugin. Some cooperation required from your side would be:
At any rate, please review this PR for accuracy of description etc. |
@ahmetb sounds good. Issue on kubeval opened instrumenta/kubeval#187 Not sure if you're using CODEOWNERS or similar? I'm seeing some multi-user repos do similar. Happy to be added if useful. The description and other details look correct, thanks @nlamirault for submitting. |
I am not sure if Kubernetes/prow has support for CODEOWNERS so we aren’t doing that. Also I am not sure how it is relevant and what you have in mind to achieve with it. |
@ahmetb spurred by a conversation today regarding tekton https://github.com/tektoncd/catalog/pull/104/files. They are starting to use owners as a way of scaling shared content and routing issues. |
Yeah, we don't have a per-plugin directory structure to support that. We already have the standard OWNERS files and we know who the owners are. I hope I'm not missing something. |
@nlamirault do you have time to help instrumenta/kubeval#187, then we can merge this as /retitle Add plugin: kubeval |
 bin: kubeval | ||
 files: | ||
 - from: "./kubeval" | ||
 to: "." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also install a LICENSE
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The archive already includes a LICENSE
file, so you can simply copy out another file like so:
- from: LICENSE
to: .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's resolve this PR in one or the other way. I think the manifest is in good shape and could be merged.
From the discussion above, there are a few open points:
- Rename the plugin to
kubectl kubeval
. Thus existing users can recognize the tool inkrew
, even though it stutters. - Adjust help text when
kubeval
is run as plugin. As the plugin namekubeval
is the same as the standalone binary, this is a nice-to-have but no blocker for merging in my opinion. - You need to install the LICENSE file.
- Get the CI build green.
@nlamirault If these points are addressed, I'm in favor of merging.
plugins/kubeval.yaml
Outdated
 version: "0.14.0" | ||
 shortDescription: Validate your Kubernetes configuration files | ||
description: | | ||
The plugin validate a Kubernetes YAML or JSON configuration file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin validate a Kubernetes YAML or JSON configuration file. | |
The plugin validates a Kubernetes YAML or JSON configuration file. |
 bin: kubeval | ||
 files: | ||
 - from: "./kubeval" | ||
 to: "." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The archive already includes a LICENSE
file, so you can simply copy out another file like so:
- from: LICENSE
to: .
plugins/kubeval.yaml
Outdated
 name: validate-config | ||
spec: | ||
 version: "0.14.0" | ||
 shortDescription: Validate your Kubernetes configuration files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
 shortDescription: Validate your Kubernetes configuration files | |
 shortDescription: Validate schema of resource files |
plugins/kubeval.yaml
Outdated
apiVersion: krew.googlecontainertools.github.com/v1alpha2 | ||
kind: Plugin | ||
metadata: | ||
 name: validate-config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
 name: validate-config | |
 name: kubeval |
If nobody’s signing up to maintain this manifest, I suggest we don’t proceed. We equally need support from kubeval to look like a plugin (help msgs etc, and release bundle not breaking). |
Signed-off-by: Nicolas Lamirault <[email protected]>
@ahmetb @corneliusweig i just push a commit with your remarks |
So to sum up @ahmetb's concerns:
Provided that @nlamirault agrees to maintain this manifest, I'm still supportive of merging. |
description: | | ||
The plugin validates a Kubernetes YAML or JSON configuration file. | ||
It does so using schemas generated from the Kubernetes OpenAPI specification, | ||
and therefore can validate schemas for multiple versions of Kubernetes | ||
homepage: https://github.com/instrumenta/kubeval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation of these two fields is wrong. Please fix this to get the CI to pass.
@nlamirault Nothing has happened here for almost a month. If you don't plan to continue, could you please close the issue? |
/close |
@ahmetb: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This plugin allow for running
kubeval
, a tool for validating a Kubernetes YAML or JSON configuration file.It does so using schemas generated from the Kubernetes OpenAPI specification, and therefore can validate schemas for multiple versions of Kubernetes.