Starting a refactoring around RunContext and Docker local/remote Api#2497
Merged
nkubala merged 8 commits intoGoogleContainerTools:masterfrom Jul 26, 2019
Merged
Starting a refactoring around RunContext and Docker local/remote Api#2497nkubala merged 8 commits intoGoogleContainerTools:masterfrom
nkubala merged 8 commits intoGoogleContainerTools:masterfrom
Conversation
dgageot
requested review from
TadCordle,
balopat,
briandealwis,
chanseokoh,
loosebazooka,
nkubala,
priyawadhwa and
tejal29
as code owners
dgageot
force-pushed
the
refactoring-start
branch
from
18b418d to
d83eec8
Compare
Codecov Report
|
nkubala
reviewed
Jul 18, 2019
| // NewAPIClient guesses the docker client to use based on current kubernetes context. | ||
| func NewAPIClient(forceRemove bool, insecureRegistries map[string]bool) (LocalDaemon, error) { | ||
| // NewAPIClientImpl guesses the docker client to use based on current kubernetes context. | ||
| func NewAPIClientImpl(runCtx *runcontext.RunContext) (LocalDaemon, error) { |
Contributor
There was a problem hiding this comment.
wdyt about having a dockerCLIContext or something that contains just the necessary fields for this constructor? then that can live inside the runcontext and we can just pass that in here.
type DockerCLIContext struct {
insecureRegistries map[string]bool
prune bool
}
type RunContext struct {
dockerCLIContext *DockerCLIContext
...
}
func NewAPIClientImpl(cliContext *docker.DockerCLIContext) (LocalDaemon, error) {
...
}
cli, err := docker.NewAPIClient(runctx.DockerCLIContext)
Contributor
Author
There was a problem hiding this comment.
Yeah, I could try that
| } | ||
|
|
||
| func getTagger(t latest.TagPolicy, customTag string) (tag.Tagger, error) { | ||
| func getTagger(runCtx *runcontext.RunContext) (tag.Tagger, error) { |
Contributor
There was a problem hiding this comment.
so is the end goal here just to make everything consistent? maybe I'm mistaken but I thought that you wanted to move away from the runcontext in these constructors. just trying to understand
Contributor
Author
There was a problem hiding this comment.
:-) For now, I'm aiming at consistency.
dgageot
force-pushed
the
refactoring-start
branch
4 times, most recently
from
774b787 to
7cb0410
Compare
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Once those options are set by command line flags, they are immutable. Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
dgageot
force-pushed
the
refactoring-start
branch
from
7cb0410 to
916c7d9
Compare
Contributor
Author
|
@nkubala could you take another look? That would be awesome because this one is a bit painful to rebase. |
nkubala
approved these changes
Jul 26, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
My goals are:
This is just the beginning of the refactoring but already improves a bit the codebase