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

Clean up stale krew installations on windows #314

Merged
merged 3 commits into from
Aug 26, 2019

Conversation

ahmetb
Copy link
Member

@ahmetb ahmetb commented Aug 22, 2019

This introduces a preRun check to root cmd. On windows, we determine the
current version of krew from the installation receipt and clean all other
directories in $KREW_ROOT/store/krew that aren't the current version,
since we don't delete the install directory on post-upgrade if plugin
name=="krew" (due to inability of deleting currently executed binary on
win32).

Fixes #309.
/assign @corneliusweig

This introduces a preRun check to root cmd. On windows, we determine the
current version of krew from the installation receipt and clean all other
directories in $KREW_ROOT/store/krew that aren't the current version,
since we don't delete the install directory on post-upgrade if plugin
name=="krew" (due to inability of deleting currently executed binary on
win32).

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 22, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2019
Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Looks good overall, but can you add a test for CleanupStaleKrewInstallations?

}
} else if f.Name() != newVersion {
glog.V(1).Infof("Unlink krew installation under %q", pluginVersionPath)
// TODO(ahmetb,lbb) is this part implemented???
Copy link
Contributor

Choose a reason for hiding this comment

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

finally this cryptic todo is gone 🎉

return nil
}

func cleanupStaleKrewInstallations() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad we can't cover this with integration tests, because it only targets windows where we don't run tests...

Copy link
Member Author

Choose a reason for hiding this comment

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

we are able to emulate windows very easily by doing KREW_OS=windows here.
I just didn't think it was worth adding integration tests, because the upgrade path is fairly complex.

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Looks good overall, but can you add a test for CleanupStaleKrewInstallations?

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb
Copy link
Member Author

ahmetb commented Aug 22, 2019

Done.

@corneliusweig
Copy link
Contributor

Oops.. CI failed

@ahmetb
Copy link
Member Author

ahmetb commented Aug 23, 2019

2: Consider preallocating `got` (prealloc)
336	var got []string

I hate this lint check 😄

@codecov-io
Copy link

Codecov Report

Merging #314 into master will increase coverage by 1.63%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #314      +/-   ##
=========================================
+ Coverage   55.66%   57.3%   +1.63%     
=========================================
  Files          19      19              
  Lines         918     904      -14     
=========================================
+ Hits          511     518       +7     
+ Misses        355     335      -20     
+ Partials       52      51       -1
Impacted Files Coverage Δ
pkg/environment/environment.go 88.23% <ø> (+9.06%) ⬆️
pkg/installation/upgrade.go 0% <0%> (ø) ⬆️
pkg/installation/install.go 42.42% <68.75%> (+3.44%) ⬆️
pkg/download/downloader.go 67.3% <0%> (-0.74%) ⬇️
pkg/index/indexscanner/scanner.go 70.21% <0%> (+0.64%) ⬆️

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 82e6899...ea16786. Read the comment docs.

@ahmetb
Copy link
Member Author

ahmetb commented Aug 26, 2019

This should be good to go.

@corneliusweig
Copy link
Contributor

Yeah, sorry for the delay.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2019
@k8s-ci-robot k8s-ci-robot merged commit 5a29484 into kubernetes-sigs:master Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrading krew on windows fails while removing old krew
4 participants