-
Notifications
You must be signed in to change notification settings - Fork 5
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
add suport docker and refresh token #3
Conversation
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.
Thanks so much for the PR @zhaojizhuang, this is super cool!
e611937
to
f6e93ae
Compare
@julz Thanks for your review, PR has been updated! |
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.
looks pretty good, few more nits
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's a couple small nits it'd be good to fix at some point, but let's get this merged and clean those up in another PR. This is really great, thanks @zhaojizhuang!
const ( | ||
RuntimeTypeDocker string = "docker" | ||
RuntimeTypeContainerd string = "contaienrd" | ||
RuntimeTypeCriO string = "cri-o" |
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.
these could be private
var freezeThaw daemon.FreezeThawer | ||
switch runtimeType { | ||
case RuntimeTypeDocker: | ||
freezeThaw, err = docker.NewDockerService() |
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.
might be nice to make both methods be called New
or Connect
so they match. Either way ideally we wouldn't have docker.NewDockerService
because it stutters
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.
Both New
sounds like a good idea
Changes
/kind
Fixes #
Release Note
Docs