Typed builder commands (updated) #5
Conversation
This is a work base to introduce more features like build time dockerfile optimisations, dependency analysis and parallel build, as well as a first step to go from a dispatch-inline process to a frontend+backend process. Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
…spatch time Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
|
Especially the env var expansion is much easier to understand this way. |
| @@ -0,0 +1,316 @@ | |||
| package typedcommand | |||
There was a problem hiding this comment.
In the docs we call these "instructions" not commands. It would be good to keep the terms consistent. Maybe we can name this package instructions ?
| return parseShell(newParseRequestFromNode(node)) | ||
| } | ||
|
|
||
| buildsFailed.WithValues(metricsUnknownInstructionError).Inc() |
There was a problem hiding this comment.
move this metrics to the caller package
|
|
||
| func Parse(ast *parser.Node, targetStage string, buildsFailed metrics.LabeledCounter) (stages BuildableStages, metaArgs []ArgCommand, err error) { | ||
| for _, n := range ast.Children { | ||
| if n.Value == command.From && stages.IsCurrentStage(targetStage) { |
There was a problem hiding this comment.
Parser pkg should not define stateful public functions like this. If you need helper functions that work on returned types, that is fine.
There was a problem hiding this comment.
Ok, Will move the "Tree shaking" on the caller site
| return nil, fmt.Errorf("unknown instruction: %s", strings.ToUpper(node.Value)) | ||
| } | ||
|
|
||
| func Parse(ast *parser.Node, targetStage string, buildsFailed metrics.LabeledCounter) (stages BuildableStages, metaArgs []ArgCommand, err error) { |
There was a problem hiding this comment.
Either return stages slice or define a type that contains the args.
There was a problem hiding this comment.
BuildableStages is a type alias for []BuildableStage with helper methods. Do you prefer explicit []BuildableStage return type and convertion of methods to free functions ?
| } | ||
| switch c := cmd.(type) { | ||
| case *ArgCommand: | ||
| if len(stages) == 0 { |
There was a problem hiding this comment.
This isn't very clear. You can just parse all args first. After that switch between from and any other command.
| } | ||
|
|
||
| func ParseCommand(node *parser.Node, buildsFailed metrics.LabeledCounter) (interface{}, error) { | ||
| switch node.Value { |
There was a problem hiding this comment.
make the request object before the switch for readability
builder/dockerfile/builder.go
Outdated
| sourceCode = src.SourceCode() | ||
| } | ||
| if len(parseResult) > 1 { | ||
| fmt.Fprintf(b.Stdout, "Stage %v: %v / %v %v", stage.Name, i+1, len(stage.Commands), sourceCode) |
There was a problem hiding this comment.
Maybe avoid doing new logging output for now as it will likely change more in the future. No issue if it changes a bit just because the logic is different.
There was a problem hiding this comment.
I made it so that it is clearer when I debugged multi-stage related issues. I'll rollback it
builder/dockerfile/dispatchers.go
Outdated
| return "", errors.Errorf("invalid name for build stage: %q, name can't start with a number or contain symbols", stageName) | ||
| state := d.state | ||
| state.beginStage(cmd.StageName, image) | ||
| state.runConfig.OpenStdin = false |
There was a problem hiding this comment.
this should be part of/called by beginStage
builder/dockerfile/evaluator.go
Outdated
| return nil, err | ||
| runConfigEnv := d.state.runConfig.Env | ||
| envs := append(runConfigEnv, d.builder.buildArgs.FilterAllowed(runConfigEnv)...) | ||
| if ex, ok := cmd.(typedcommand.SupportsMultiWordExpansion); ok { |
There was a problem hiding this comment.
As this is only allowed for expose, the expose dispatcher should just call this call this function.
builder/dockerfile/evaluator.go
Outdated
| envs := append(runConfigEnv, d.builder.buildArgs.FilterAllowed(runConfigEnv)...) | ||
| if ex, ok := cmd.(typedcommand.SupportsMultiWordExpansion); ok { | ||
| ex.Expand(func(word string) ([]string, error) { | ||
| return d.shlex.ProcessWords(word, envs) |
There was a problem hiding this comment.
Follow up: shlex could be removed from dispatchers.
There was a problem hiding this comment.
This will be needed for "expose" special case and for "from" as well (from has a different buildargs handling than default)
builder/dockerfile/evaluator.go
Outdated
| case *typedcommand.StopSignalCommand: | ||
| return d.dispatchStopSignal(c) | ||
| case *typedcommand.ArgCommand: | ||
| if d.state.hasDispatchedFrom { |
There was a problem hiding this comment.
Shouldn't be required. "meta" should use a different path.
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
… logic Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
…ildable stage Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
This the result of my workday, implementing feedback from #4.
Still have to rewrite equivalent tests, and to run the whole builder integration test suite, but I think it is much better