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

Genericized caching for localkube binary and minikube iso #613

Closed
wants to merge 1 commit into from

Conversation

r2d4
Copy link
Contributor

@r2d4 r2d4 commented Sep 22, 2016

This also adds checksum checking for localkube versions. We will need
to upload an additional localkube-linux-amd64.sha256 file as we do for
the iso releases.

ref #599 #602

@codecov-io
Copy link

codecov-io commented Sep 22, 2016

Current coverage is 32.88% (diff: 60.00%)

Merging #613 into master will increase coverage by 0.06%

@@             master       #613   diff @@
==========================================
  Files            44         45     +1   
  Lines          1929       1919    -10   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            633        631     -2   
+ Misses         1168       1165     -3   
+ Partials        128        123     -5   

Powered by Codecov. Last update 3a5966c...6e20bab

@r2d4 r2d4 force-pushed the localkube-sha branch 2 times, most recently from ba34dcc to 9685362 Compare September 22, 2016 01:53
This also adds checksum checking for localkube versions.  We will need
to upload an additional localkube-linux-amd64.sha256 file as we do for
the iso releases.
Copy link
Contributor

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

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

A few nits. I like the way this is looking!

URL: config.MinikubeISO,
ShaURL: constants.DefaultIsoShaUrl,
}
_, err := cache.GetFile(minikubeISO)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to at least close the file?

ShaURL string
}

type DiskCache struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this level of indirection, but it's a bit awkward for callers to have to make a var cache util.DiskCache before calling any methods.

What do you think about adding a static cache to the top level of this package, and adding a SetCache function to allow swapping it out?

Similar to this pattern from libmachine: https://github.com/docker/machine/blob/master/libmachine/ssh/client.go

@r2d4
Copy link
Contributor Author

r2d4 commented Oct 5, 2016

Closing this for now as #624 has already fixed the main issue of checking localkube SHAs. Might be worth refactoring in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants