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.

Sign up for GitHub

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

Jump to bottom

Validate manifest file extension and kind field #191

Merged
merged 10 commits into from
May 31, 2019
Merged

Validate manifest file extension and kind field #191

merged 10 commits into from
May 31, 2019

Conversation

corneliusweig
Copy link
Contributor

@corneliusweig corneliusweig commented May 30, 2019 •
edited
Loading

This PR now validates the file extension in the krew manifest validator.

Also, krew now expects kind: Plugin, which was not validated at all.

Last, it adds a test util to create and cleanup temporary directories. Some uses of vanilla ioutil.TempDir were updated with the new utility.

Fix #190

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 30, 2019
@ahmetb
Copy link
Member

ahmetb commented May 30, 2019

Meta-comment: Next time if you can keep the PRs scoped to what’s discussed (in this case, just the file extension validation) it would make the code review more focused as we wouldn't deliberate on other things.

ahmetb
ahmetb reviewed May 30, 2019
View reviewed changes
Copy link
Member

@ahmetb ahmetb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, thanks for doing the cleanup.

pkg/index/validate.go Outdated Show resolved Hide resolved
cmd/validate-krew-manifest/main_test.go Show resolved Hide resolved
pkg/download/downloader_test.go Show resolved Hide resolved
pkg/testutil/TempDir.go Outdated Show resolved Hide resolved
pkg/testutil/TempDir.go Outdated Show resolved Hide resolved
pkg/testutil/TempDir.go Outdated Show resolved Hide resolved
pkg/testutil/TempDir.go Outdated Show resolved Hide resolved
pkg/testutil/TempDir.go Outdated Show resolved Hide resolved
@corneliusweig
Copy link
Contributor Author

Meta-comment: Next time if you can keep the PRs scoped to what’s discussed (in this case, just the file extension validation) it would make the code review more focused as we wouldn't deliberate on other things.

Yeah, sorry about that. I badly wanted to add tests and one thing led to another.

Also, please don't hesitate to ask for splitting up a PR. But I'll also try to be more consistent. 🙏

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 30, 2019
ahmetb
ahmetb reviewed May 30, 2019
View reviewed changes
hack/check-code-patterns.sh Outdated Show resolved Hide resolved
ahmetb
ahmetb reviewed May 30, 2019
View reviewed changes
hack/check-code-patterns.sh Outdated Show resolved Hide resolved
ahmetb
ahmetb reviewed May 30, 2019
View reviewed changes
pkg/testutil/tempdir.go Outdated Show resolved Hide resolved
ahmetb
ahmetb reviewed May 30, 2019
View reviewed changes
pkg/testutil/tempdir.go Outdated Show resolved Hide resolved
@corneliusweig
Improvements from review
6cb2f4f
@corneliusweig
Add code pattern test
3166a9c 
Ensure that `testutil.NewTempDir()` is used in favor of `ioutil.Tempdir`

Ignore error from grep due to no matches

grep exits with error code 1 if no matches are found. This makes it
tricky when `set -eo pipefail` is set. Therefore specifically ignore
error code 123, which results from xargs when a subcommand failed.
@corneliusweig
Fix boilerplate and linter issues
934c2e6
@codecov-io
Copy link

codecov-io commented May 30, 2019 •
edited
Loading

Codecov Report

Merging #191 into master will increase coverage by 2.85%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   50.91%   53.77%   +2.85%     
==========================================
  Files          13       13              
  Lines         711      716       +5     
==========================================
+ Hits          362      385      +23     
+ Misses        297      279      -18     
  Partials       52       52
Impacted Files Coverage Δ
pkg/index/validate.go 94.44% <100%> (+0.32%) ⬆️
cmd/validate-krew-manifest/main.go 58.33% <100%> (+20.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b68ebd...934c2e6. Read the comment docs.

@ahmetb ahmetb changed the title Validate file ending of manifests and require kind: Plugin Validate manifest file extension and kind field May 31, 2019
@ahmetb
Copy link
Member

ahmetb commented May 31, 2019

/lgtm
/approve
thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, corneliusweig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2019
@k8s-ci-robot k8s-ci-robot merged commit 52cbf1e into kubernetes-sigs:master May 31, 2019
ahmetb added a commit to ahmetb/krew that referenced this pull request Jun 14, 2019
@ahmetb
Make corneliusweig a maintainer
18ea1a6 
Adding corneliusweig to owners. He has been consistently helping both with
krew and krew-index repositories in terms of:
- developing plugins himself
- taking a stab at krew machinery with large scale code refactors
- adding integration test suite to the project
- adding more validation and test cases
- increasing developer documentation

Some of his notable work:
- kubernetes-sigs#195
- kubernetes-sigs#183
- kubernetes-sigs#191
- kubernetes-sigs#201
- kubernetes-sigs#202
- kubernetes-sigs#203
- kubernetes-sigs#208

He is familiar with the codebase enough to officially review and approve code.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb ahmetb mentioned this pull request Jun 14, 2019
Merged
ahmetb added a commit that referenced this pull request Jun 18, 2019
@ahmetb
Make corneliusweig a maintainer (#214)
aecfd53 
Adding corneliusweig to owners. He has been consistently helping both with
krew and krew-index repositories in terms of:
- developing plugins himself
- taking a stab at krew machinery with large scale code refactors
- adding integration test suite to the project
- adding more validation and test cases
- increasing developer documentation

Some of his notable work:
- #195
- #183
- #191
- #201
- #202
- #203
- #208

He is familiar with the codebase enough to officially review and approve code.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@corneliusweig corneliusweig mentioned this pull request Jun 19, 2019
Closed
6 tasks
@corneliusweig corneliusweig deleted the validate-fileending branch June 21, 2019 22:46
@corneliusweig corneliusweig mentioned this pull request Jun 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahmetb ahmetb ahmetb left review comments

@juanvallejo juanvallejo Awaiting requested review from juanvallejo

Assignees

@ahmetb ahmetb

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

validate-krew-manifest should check for the correct manifest file ending
4 participants
@corneliusweig @ahmetb @codecov-io @k8s-ci-robot