Starting a refactoring around RunContext and Docker local/remote Api #2497
Conversation
This file contains 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
dgageot
requested review from
balopat,
briandealwis,
chanseokoh,
loosebazooka,
nkubala,
priyawadhwa,
TadCordle 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) { |
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)There was a problem hiding this comment.
Yeah, I could try that
| @@ -197,11 +191,13 @@ func getDeployer(runCtx *runcontext.RunContext) (deploy.Deployer, error) { | |||
| } | |||
| } | |||
|
|
|||
| func getTagger(t latest.TagPolicy, customTag string) (tag.Tagger, error) { | |||
| func getTagger(runCtx *runcontext.RunContext) (tag.Tagger, error) { | |||
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
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 <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Once those options are set by command line flags, they are immutable. Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
dgageot
force-pushed
the
refactoring-start
branch
from
7cb0410
to
916c7d9
Compare
|
@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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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