Skip to content

build: --iidfile support with buildkit#1176

Merged
tiborvass merged 1 commit into
docker:masterfrom
tiborvass:buildkit-iidfile
Jul 3, 2018
Merged

build: --iidfile support with buildkit#1176
tiborvass merged 1 commit into
docker:masterfrom
tiborvass:buildkit-iidfile

Conversation

@tiborvass
Copy link
Copy Markdown
Collaborator

@tiborvass tiborvass commented Jul 2, 2018

Signed-off-by: Tibor Vass tibor@docker.com

To be tested with moby/moby#37368

This changes buildkit's output to Stdout instead of Stderr, to match the old builder behavior.

return errors.Errorf("buildkit not supported by daemon")
}

if options.imageIDFile != "" {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure I'm missing some context for where this gets run, but this seems odd.
What if the user already has this file from a previous build and doesn't want it removed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpuguy83 this seems to be the behavior for old builder as well. The idea is to say, if we can't remove the file, we can't write to it, so fail early instead of running the whole successful build and then fail at writing the image id file.

Comment thread cli/command/image/build_buildkit.go Outdated

if options.imageIDFile != "" {
if imageID == "" {
return errors.Errorf("Server did not provide an image ID. Cannot write %s", options.imageIDFile)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"cannot write image ID file, server did not provide an image ID"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpuguy83 thanks! Should be fixed now!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpuguy83 as in you need to use it with moby/moby#37368

Comment thread cli/command/image/build_buildkit.go Outdated
}
imageID = strings.TrimSpace(imageID)
if err := ioutil.WriteFile(options.imageIDFile, []byte(imageID), 0666); err != nil {
return err
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`errors.Wrap(err, "error writing image ID file")

@tiborvass tiborvass force-pushed the buildkit-iidfile branch 3 times, most recently from 4d016f1 to 8f43a9c Compare July 2, 2018 21:45
Comment thread cli/command/image/build_buildkit.go Outdated
close(displayCh)
}()
displayStatus(displayCh)
displayStatus(out, displayCh)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/out/os.Stderr/

Comment thread cli/command/image/build_buildkit.go Outdated
})
} else {
displayStatus(t.displayCh)
displayStatus(out, t.displayCh)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/out/os.Stdout/

Signed-off-by: Tibor Vass <tibor@docker.com>
@tonistiigi
Copy link
Copy Markdown
Member

LGTM

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants