builder: add config var for setting context streaming default #245
Conversation
Codecov Report
@@ Coverage Diff @@
## master #245 +/- ##
==========================================
- Coverage 48.34% 48.32% -0.03%
==========================================
Files 172 172
Lines 11710 11715 +5
==========================================
Hits 5661 5661
- Misses 5691 5696 +5
Partials 358 358 |
cli/command/image/build.go
Outdated
| if v := dockerCli.ConfigFile().BuildStreamContext; v != nil { | ||
| options.stream = *v | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we move this to a new function ?
func setDefaultsFromConfig(dockerCli command.Cli, flags *pflag.FlatSet, options *buildOptions) {
...
}That will make it easy to unit test.
There was a problem hiding this comment.
You mean setDefaultsFromConfig(dockerCli command.Cli, flags *pflag.FlatSet, options *buildOptions) ? Need a pointer of the options to change it. What is the bool return supposed to mean? Also, this wouldn't work because this check needs DockerCli not Cli to check if session is supported.
There was a problem hiding this comment.
Yes, I forgot to remove the bool return (fixed now), and it would need to be a pointer.
It shouldn't need DockerCli. I fixed Cli to include ServerInfo in #233.
| NodesFormat string `json:"nodesFormat,omitempty"` | ||
| PruneFilters []string `json:"pruneFilters,omitempty"` | ||
| Proxies map[string]ProxyConfig `json:"proxies,omitempty"` | ||
| BuildStreamContext *bool `json:"buildStreamContext,omitempty"` |
There was a problem hiding this comment.
@thaJeztah buildStreamContext or streamBuildContext?
There was a problem hiding this comment.
I guess most of these start with the object name (service, images, etc). but buildStreamContext doesn't read too well.
How about buildAlwaysStreamContext (or buildDefaultStreamContext) ? That way it's a little more clear what this is enabling.
There was a problem hiding this comment.
Argh. naming. hm
Do we have other contexts other than for build? For booleans, I always liked, e.g. enableSomething (EnableContextStreaming e.g.), but that may be a bit too verbose
Do we expect more build-related options? If so, possibly have a build key, containing stream-context (in the JSON)
There was a problem hiding this comment.
I'd be good with a build key.
I don't think Enable is right, it's not enabling a feature, it's just changing the default.
Signed-off-by: Tonis Tiigi <[email protected]>
tonistiigi
force-pushed
the
stream-config
branch
from
fc469ae to
34c8dc0
Compare
|
what are the possible values ? any doc on the subject ? |
|
@thaJeztah @tonistiigi @dnephin what is the status here (should we close for now) ? |
|
Nope |
[17.09] back port Close pipe if mountFrom failed.
8 participants
@tiborvass @thaJeztah
Signed-off-by: Tonis Tiigi [email protected]