Repository navigation
Improve logic to determine if receipts migration is necessary - #377
Conversation
| for _, entry := range store { | ||
| if entry.IsDir() { | ||
| for _, entry := range plugins { | ||
| if strings.HasPrefix(entry.Name(), "kubectl-") && 0 != entry.Mode()&os.ModeSymlink { |
There was a problem hiding this comment.
what if we didn't look for kubectl- prefix?
similarly, what if we didn't look for them to be symlinks?
we can just ReadDir?
There was a problem hiding this comment.
That should be fine, unless somebody puts something into the bin directory. Doing these checks makes that much more unlikely. But this should matter in very exceptional cases only.
There was a problem hiding this comment.
Yeah I guess it's especially fine because we'll remove this code ≥0.5 or maybe earlier.
|
mostly lgtm |
|
/lgtm We can have this + #375 go into a new release, as both of them address Windows installation issues. |
7c4fa77 to
7a3d03c
Compare
|
I simplified the logic as suggested. That way, the tests don't need to be modified as much, which is better.
Yes, makes sense. Let's wait for confirmation that the copy/move issue is indeed resolved. |
| { | ||
| name: "One plugin and receipts", | ||
| filesPresent: []string{"store/konfig/konfig.sh", "receipts/present"}, | ||
| filesPresent: []string{"bin/konfig/konfig.sh", "receipts/present"}, |
There was a problem hiding this comment.
this is not very realistic since it created dirs under bin/
let's just add bin/konfig.sh directly.
also if "konfig" the word isn't relevant, let's say "foo", (ditto below)
but it seems like this has simplified the tests a good deal.
There was a problem hiding this comment.
Yes, makes much more sense!
|
/lgtm |
|
/hold cancel |
|
/retitle Improve logic to determine if receipts migration is necessary |
| if err := os.MkdirAll(binPath, 0755); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| tempDir.Write("receipts/krew.notyaml", []byte("must be present for receipts migration check")) |
There was a problem hiding this comment.
This is a bit hacky. But the new logic requires that something is present under receipts/.
3b65a0f to
b169f84
Compare
Codecov Report
@@ Coverage Diff @@
## master #377 +/- ##
==========================================
- Coverage 57.11% 56.92% -0.19%
==========================================
Files 19 19
Lines 914 910 -4
==========================================
- Hits 522 518 -4
Misses 339 339
Partials 53 53
Continue to review full report at Codecov.
|
|
@ahmetb If we merge this, we can craft a bugfix release for windows, right? |
|
I think so. |
|
/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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Let's try to get other low-hanging small PRs in for a release too! 👍 |
Fixes #374