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

Proposal: implement a manifest style validation tool for krew-index specifically #586

Closed
ahmetb opened this issue Apr 3, 2020 · 17 comments
Labels
kind/proposal lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@ahmetb
Copy link
Member

ahmetb commented Apr 3, 2020

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 for krew-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 characters
  • shortDescription must not have a a trailing period (.)
  • description must be wrapped at 80 character lines
  • explicit files: {from: '*', to: '.'} is not needed, drop
  • after executing files on specified archive, there should be a LICENSE file in the directory.
  • [... other stuff that I can't think about right now...]

The problem is: We already have some validation in cmd/validate-krew-manifest tool that is run only for krew-index. This validation:

  1. runs all structural validation through Validate()
  2. ensures: all spec.platforms[] items are used
  3. ensures: no overlapping spec.platform[].selectors
  4. ensures: all spec.platforms[] install fine
  • point 1: Already validated by krew, during parsing
  • point 2: Not a functional validation, if there's unused platforms[], krew continues to work fine.
  • point 3: I believe Krew continues to work fine when there are overlapping selectors –though, the behavior is undefined (not sure which one would be picked up, as we don't guarantee it). We could move this to "functional validation", though I'm not sure we totally should.
  • point 4: Not "structural validation", just ensures plugins merged to krew-index are installable, we can't enforce this in custom indexes.

So 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

@corneliusweig
Copy link
Contributor

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.

@ahmetb
Copy link
Member Author

ahmetb commented Apr 7, 2020

yeah. I think it would be reusable by public indexes out of the box. private indexes have different requirements (no LICENSE etc).

@ahmetb
Copy link
Member Author

ahmetb commented Apr 7, 2020

tl;dr I'm thinking we should rename the existing tool to krew_index_manifest_validator and extend its capabilities to more cosmetics validation.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 5, 2020
@corneliusweig
Copy link
Contributor

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 15, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 13, 2020
@corneliusweig
Copy link
Contributor

/remove-lifycycle stale
/freeze
I think this is important long-term and we should work on this eventually.

@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 15, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

@corneliusweig
Copy link
Contributor

/reopen
/remove-lifecycle rotten
/good-first-issue

@k8s-ci-robot k8s-ci-robot reopened this Jan 24, 2021
@k8s-ci-robot
Copy link
Contributor

@corneliusweig: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle rotten
/good-first-issue

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.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 24, 2021
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 24, 2021
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 24, 2021
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/proposal lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

4 participants