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

Ensure DownloadPath is always clean #464

Merged
merged 1 commit into from
Jan 27, 2020
Merged

Ensure DownloadPath is always clean #464

merged 1 commit into from
Jan 27, 2020

Conversation

aimbot31
Copy link
Contributor

@aimbot31 aimbot31 commented Jan 13, 2020 •
edited
Loading

Removed DownloadPath function.
Used ioutil.TempDir each time a tmpDir was needed.
Removed old tests on DownloadPath.

Fixes #448.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 13, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @aimbot31!

It looks like this is your first PR to kubernetes-sigs/krew 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/krew has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 13, 2020
@codecov-io
Copy link

codecov-io commented Jan 13, 2020 •
edited
Loading

Codecov Report

Merging #464 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
- Coverage   56.52%   56.47%   -0.05%     
==========================================
  Files          19       19              
  Lines         927      926       -1     
==========================================
- Hits          524      523       -1     
  Misses        350      350              
  Partials       53       53
Impacted Files Coverage Δ
internal/environment/environment.go 87.87% <ø> (-0.36%) ⬇️
internal/installation/install.go 42.1% <0%> (-0.32%) ⬇️
internal/installation/upgrade.go 0% <0%> (ø) ⬆️

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 d3103f3...e9dc907. Read the comment docs.

@aimbot31
Copy link
Contributor Author

/assign @ahmetb

@ahmetb
Copy link
Member

ahmetb commented Jan 13, 2020

/cc @ferhatelmas
can you please do a review?

@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot Jan 13, 2020
@aimbot31
Copy link
Contributor Author

Done, do you see something else @ahmetb ?

@aimbot31
Ensure DownloadPath is always clean kubernetes-sigs#447
e9dc907 
Removed DownloadPath function.
Used ioutil.TempDir each time a tmpDir was needed.
Removed old tests on DownloadPath.
@ahmetb
Copy link
Member

ahmetb commented Jan 13, 2020

Good with me, I'm looking for additional reviews from co-maintainers.
Hope you don't mind the wait.

@corneliusweig
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2020
@corneliusweig
Copy link
Contributor

corneliusweig commented Jan 14, 2020 •
edited
Loading

/retitle Ensure DownloadPath is always clean

@aimbot31 Could you include 'Fixes #448' in your PR description? (not the title 😉)

@k8s-ci-robot k8s-ci-robot changed the title Ensure DownloadPath is always clean #448 Ensure DownloadPath is always clean Jan 14, 2020
@aimbot31
Copy link
Contributor Author

/retitle Ensure DownloadPath is always clean

@aimbot31 Could you include 'Fixes #448' in your PR description? (not the title )

There you go 😉

@aimbot31
Copy link
Contributor Author

aimbot31 commented Jan 24, 2020 •
edited
Loading

Hello, any news ? 😄

ahmetb
ahmetb reviewed Jan 24, 2020
View reviewed changes
internal/installation/install.go
klog.V(3).Infof("Creating download staging directory %q", op.downloadStagingDir)
if err := os.MkdirAll(op.downloadStagingDir, 0755); err != nil {
return errors.Wrapf(err, "could not create staging dir %q", op.downloadStagingDir)
klog.V(3).Infof("Creating download staging directory")
Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to delete this dir after we are done right?

Maybe defer some cleanup?

Copy link
Contributor Author

@aimbot31 aimbot31 Jan 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@ahmetb there's already a defer to clean the dir at the line 100.

Copy link
Member

Choose a reason for hiding this comment

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

sorry for missing :D

@ahmetb
Copy link
Member

ahmetb commented Jan 27, 2020

/lgtm
/approve
Thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Jan 27, 2020
@k8s-ci-robot k8s-ci-robot merged commit da2c84b into kubernetes-sigs:master Jan 27, 2020
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

@corneliusweig corneliusweig Awaiting requested review from corneliusweig

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@ahmetb ahmetb

@corneliusweig corneliusweig

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure archive extraction directory is not used before
5 participants
@aimbot31 @k8s-ci-robot @codecov-io @ahmetb @corneliusweig