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

pkg/index: ignore non-.yaml files during scanning #413

Merged
merged 2 commits into from Dec 2, 2019

Conversation

@ahmetb
Copy link
Member

ahmetb commented Dec 1, 2019

Turns out if there was a file in index/ directory with a non-.yaml extension,
we still targeted that as a plugin, so we tried to parse its manifest. (But
read/parsing of individual manifests do not cause fatal errors, just printed
to stderr like a warning.)

This creates a helper method that properly scans for .yaml files in an
immediate directory.

Fixes #412
/assign @corneliusweig

Turns out if there was a file in index/ directory with a non-.yaml extension,
we still targeted that as a plugin, so we tried to parse its manifest. (But
read/parsing of individual manifests do not cause fatal errors, just printed
to stderr like a warning.)

This creates a helper method that properly scans for .yaml files in an
immediate directory.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb ahmetb force-pushed the ahmetb:index-scan-extension branch from e6c6629 to c273f3b Dec 1, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 1, 2019

Codecov Report

Merging #413 into master will decrease coverage by 0.1%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #413      +/-   ##
==========================================
- Coverage   56.52%   56.41%   -0.11%     
==========================================
  Files          19       19              
  Lines         920      927       +7     
==========================================
+ Hits          520      523       +3     
- Misses        347      349       +2     
- Partials       53       55       +2
Impacted Files Coverage Δ
pkg/index/indexscanner/scanner.go 66.66% <80%> (-3.55%) ⬇️

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 952834f...0a85a15. Read the comment docs.

Copy link
Contributor

corneliusweig left a comment

Mostly
/lgtm
/approve

I also have a minor questions, therefore
/hold

}

pluginName := strings.TrimSuffix(f.Name(), filepath.Ext(f.Name()))
for _, fn := range files {

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Dec 1, 2019

Contributor

Other languages use fn as keyword for functions. Of course this is not Rust, but it makes the reader stumble. Thus can you rename?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Dec 1, 2019

Author Member

I meant filename here :D

pkg/index/indexscanner/scanner.go Outdated Show resolved Hide resolved
return nil, errors.Wrap(err, "failed to open index dir")
}
for _, fi := range files {
if fi.Mode().IsRegular() && filepath.Ext(fi.Name()) == constants.ManifestExtension {

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Dec 1, 2019

Contributor

Would it make sense to be even stricter here? For example, plugin names cannot contain spaces. These kind of errors would be caught later but we could as well do that here.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Dec 1, 2019

Author Member

I don't think this is the right place to act on those things.
It actually doesn't matter a whole lot there's a plugin file with space in the name. If that happens, we already capture it as a validation (parsing) error.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm label Dec 2, 2019
@ahmetb

This comment has been minimized.

Copy link
Member Author

ahmetb commented Dec 2, 2019

/hold cancel

@corneliusweig

This comment has been minimized.

Copy link
Contributor

corneliusweig commented Dec 2, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 2, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link

k8s-ci-robot commented Dec 2, 2019

[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:
  • OWNERS [ahmetb,corneliusweig]

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 merged commit ed324d4 into kubernetes-sigs:master Dec 2, 2019
2 of 3 checks passed
2 of 3 checks passed
tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation ahmetb authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.