-
Notifications
You must be signed in to change notification settings - Fork 363
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
Move platform detection to pkg/installation #286
Move platform detection to pkg/installation #286
Conversation
ca57cc6
to
95dfc10
Compare
Codecov Report
@@ Coverage Diff @@
## master #286 +/- ##
==========================================
+ Coverage 54.84% 55.47% +0.63%
==========================================
Files 19 19
Lines 877 876 -1
==========================================
+ Hits 481 486 +5
+ Misses 342 337 -5
+ Partials 54 53 -1
Continue to review full report at Codecov.
|
- moved Platform matching methods to pkg/installation, where: - they make more sense - they can be tested without worrying about cyclic imports due to pkg/testutil. - rewrote some tests in platform_test.go to be shorter. - added a test for "no platforms specified" case in TestValidatePlugin. - added a check to verify-code-patterns that uses a heuristic to detect inline initializations of index.(Plugin|Platform) structs. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
95dfc10
to
bc7357b
Compare
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.
Some small nits, otherwise looks great.
pkg/installation/platform_test.go
Outdated
inOS, inArch := runtime.GOOS, runtime.GOARCH | ||
outOS, outArch := osArch() | ||
if inOS != outOS { | ||
t.Fatalf("returned OS=%q; expected=%q", outOS, inOS) |
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.
Errorf
would do fine here and below.
if inArch != outArch { | ||
t.Fatalf("returned Arch=%q; expected=%q", outArch, inArch) | ||
} | ||
} |
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.
another awkward brace. I know, it's fine ;)
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.
I don't get it :) I realized there was a missing empty line, so I added it now.
hack/verify-code-patterns.sh
Outdated
# TODO(ahmetb): ideally this plugin should cleanly scan all (index\.)(Plugin|Platform) occurrences except slice literals | ||
# (which are prefixed with []). However I could not get negative lookbehind pattern to work with grep to match to ones | ||
# that do not have a [] prefix. | ||
out="$(grep --include '*_test.go' --exclude-dir 'vendor/' -EIrn '\s+(index\.)(Plugin|Platform){' || true)" |
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.
out="$(grep --include '*_test.go' --exclude-dir 'vendor/' -EIrn '\s+(index\.)(Plugin|Platform){' || true)" | |
out="$(grep --include '*_test.go' --exclude-dir 'vendor/' -EIrn '[^]](index\.)(Plugin|Platform){' || true)" |
This works with GNU grep. Can you check?
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.
Can you explain [^]]
? To me it reads like [^]
is a negated set but it's empty, then there's a ]
literal outside that's supposed to be a match.
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.
I didn't know either. But if there is a negated character set, the first char following the caret is always interpreted verbatim. So this is indeed a negated char set of just ]
. Iow it matches anything but a ]
, which is what we need here, right?
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.
try on https://www.regexr.com
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.
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.
I didn't find out what regex standard regexr.com uses, but I can see that regexr interprets my proposed pattern differently. My GNU grep 3.1 does what I described, I just double-checked. Have you checked, if it also works with BSD grep?
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.
I finally found a proper reference: man 7 regex
says
To include a literal ']' in the list, make it the first character (following a possible '^'). To include a literal '-', make it the first or last character, or the
second endpoint of a range.
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.
For gnu grep, [^]]
and [^\]]
are different things:
$ grep --include '*_test.go' --exclude-dir 'vendor/' -EIrn '[^\]](index\.)(Plugin|Platform){'
cmd/validate-krew-manifest/main_test.go:177: err := isOverlappingPlatformSelectors([]index.Platform{p1, p2})
cmd/validate-krew-manifest/main_test.go:192: err := isOverlappingPlatformSelectors([]index.Platform{p1, p2})
$ grep --include '*_test.go' --exclude-dir 'vendor/' -EIrn '[^]](index\.)(Plugin|Platform){'
cmd/validate-krew-manifest/main_test.go:169: p1 := index.Platform{
cmd/validate-krew-manifest/main_test.go:174: p2 := index.Platform{
cmd/validate-krew-manifest/main_test.go:184: p1 := index.Platform{
cmd/validate-krew-manifest/main_test.go:189: p2 := index.Platform{
BSD grep is very dumb. It accepts arguments but then accepts input from STDIN (its cli is not fully compatible with gnu grep).
$ /usr/bin/grep --include '*_test.go' --exclude-dir 'vendor/' -EIrn '[^]](index\.)(Plugin|Platform){' | cat
grep: warning: recursive search of stdin
maybe we should try to fix but I don't care much.
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.
I've incorporated your recommendation.
@ahmetb Off-topic: as long as you don't have to rebase, could you put your review amendments in a separate commit? That makes the second review round easier. |
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
/lgtm |
[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 |
/kind cleanup
moved Platform matching methods to pkg/installation, where:
rewrote some tests in platform_test.go to be shorter.
added a check to verify-code-patterns that uses a heuristic to detect inline
initializations of index.(Plugin|Platform) structs.
Fixes #284.