-
Notifications
You must be signed in to change notification settings - Fork 371
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
Proposal: implement a manifest style validation tool for krew-index specifically #586
Comments
This is a sound plan. The only thing I'd add is that we strive to build our krew-index manifest validation tool in a way that it can be re-used by other projects. After all, everybody should be able to easily check what we think are good practices for plugin manifests. |
yeah. I think it would be reusable by public indexes out of the box. private indexes have different requirements (no LICENSE etc). |
tl;dr I'm thinking we should rename the existing tool to |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle rotten |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifycycle stale |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@fejta-bot: Closing this issue. 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. |
/reopen |
@corneliusweig: Reopened this issue. 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. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
Rotten issues close after 30d of inactivity. Send feedback to sig-contributor-experience at kubernetes/community. |
@fejta-bot: Closing this issue. 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. |
Currently, our plugin manifest validation functionality which is in the YAML parsing path has some style/cosmetic checks that have no effect on plugin installation. This is a result of not separating "cosmetic validation" and "functional validation".
If we continue on this trajectory, we will be adding more "cosmetic validation" in future versions, which will break older plugin manifests. So we can't do that. Furthermore, with custom plugin indexes (#566), we cannot control the plugin manifests out in the wild –but we have to be able to continue working with them.
I propose that we strip off these "cosmetic validations" from the logic in krew machinery, and create a
krew_index_manifest_validator
tool, specifically forkrew-index
that runs as part of its CI for each PR.This would help reduce code review toil we have been manually enforcing in
krew-index
, as well as separate structural validation vs cosmetic validation, such as:shortDescription
must not contain line break charactersshortDescription
must not have a a trailing period (.
)description
must be wrapped at 80 character linesfiles: {from: '*', to: '.'}
is not needed, dropfiles
on specifiedarchive
, there should be a LICENSE file in the directory.The problem is: We already have some validation in
cmd/validate-krew-manifest
tool that is run only forkrew-index
. This validation:Validate()
spec.platforms[]
items are usedspec.platform[].selector
sspec.platforms[]
install fineSo maybe we just make
cmd/validate-krew-manifest
more about "styling" and make it clear it's meant for krew-index./kind proposal
/cc @corneliusweig
The text was updated successfully, but these errors were encountered: