feat: add --cli option by orisano · Pull Request #6584 · docker/compose

orisano · 2019-03-15T11:28:59Z

i implemented --exec option on docker-compose build for to use buildkit.

This PR is prototype. please give me feedback.

GordonTheTurtle · 2019-03-15T11:29:02Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "feat-add-exec-build" git@github.com:orisano/compose.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

AkihiroSuda · 2019-03-15T11:46:22Z

cc @tonistiigi @tiborvass

AkihiroSuda · 2019-03-15T11:47:07Z

compose/service.py

+            line = p.stdout.readline()
+            if line == "":
+                break
+            line = line.replace("writing image sha256:", "Successfully built ")


for bypass this.
https://github.com/docker/compose/blob/master/compose/service.py#L1110

for event in all_events: if 'stream' in event: match = re.search(r'Successfully built ([0-9a-f]+)', event.get('stream', '')) if match: image_id = match.group(1)

This doesn't seem robust.
Can we just append Successfully built... when it didn't appear from docker build CLI?

I do not know how to get image_id from stdout on docker build --progress=plain.
Do you have any idea?

--iidfile?

I implemented to use an iidfile method.

docteurklein · 2019-03-18T13:45:38Z

cool! It's not the first time using the CLI is an option (see https://docs.docker.com/compose/reference/envvars/#compose_interactive_no_cli ). maybe there is a way to normalize this?

mipearson · 2019-03-30T02:21:54Z

I'm keen to contribute here - is there any guidance from the compose team as to how they'd prefer this to be implemented?

tonistiigi · 2019-04-03T05:29:05Z

Not a compose maintainer but this seems ok approach to me. We'll probably take a bit different stab at the same problem at https://github.com/tonistiigi/buildx/issues/13 but the solutions don't seem conflicting and have bit different use cases.

compose/cli/main.py

thaJeztah · 2019-04-04T08:37:10Z

compose/cli/main.py

            parallel_build=options.get('--parallel', False),
-            silent=options.get('--quiet', False)
+            silent=options.get('--quiet', False),
+            _exec=bool(options.get('--exec', False)),


here as well (I think)

Perhaps other locations as well, but I'm not a python coder 😅 and not super-familiar with the codebase (just noticed the PR)

compose/service.py

ulyssessouza · 2019-04-24T13:32:41Z

A general doubt I have is about the name of the option... --exec sounds very internal pythonic for me.
From an user's point of view I see something like --native, --cli or anything more generic.

WDYT @chris-crone @thaJeztah @rumpl @shin-

compose/service.py

ulyssessouza · 2019-04-25T10:50:06Z

Please rebase your PR with upstream master to pass the CI

orisano · 2019-04-26T00:14:13Z

rebased

ulgens · 2019-05-01T09:53:19Z

ping @ulyssessouza The last request change is already done 🙂 Is there something else missing?

ulgens · 2019-05-01T09:54:07Z

Is there anyway to read --exec parameter from environment?

CpuID · 2019-07-21T21:45:30Z

compose currently doesn't handle the "nice" output of the docker cli when buildkit is enabled (because it doesn't detect that a TTY is attached when called through docker compose)

research if we can make it see the TTY, so that the output of the docker cli is printed correctly

+1, would be nice to know it looks the same between both docker and docker-compose

if possible have an option on the docker cli that returns the build output in a machine-parseable format

perhaps the "plain" output is suitable, but if not, another format could be added

offer this as an option during build

as a (future) enhancement; implement code in docker-compose to convert that output into something more "like" the existing output of docker-compose

(this could be a nice project to work on for people from the community)

I think the most common part of the output that would be beneficial would be if a layer is cached or not (cache state).

One other nice to have (but notpossible via Buildkit? and independent of this PR) would be exposing intermediate layer image SHA/digests. I've had to on occasion grab the SHA/digest before a failure and run docker run -it --rm sha sh and poke around at the intermediate image and try interactively resolve why a step of a build failed.

jamesharris-garmin · 2019-07-31T19:01:30Z

Have we addressed the changes needed to merge this PR?

cristicbz · 2019-08-05T16:53:32Z

I'm a little worried this fell into the trap of the perfect being the enemy of the good. This PR offers an opt-in, which will offer a good path for folks who need buildkit and don't need build output. The path forward for using the CLI all the time would be good to determine, but it shouldn't block the simple use-case that this PR supports.

thaJeztah · 2019-08-05T16:58:19Z

This PR adds a new flag, in addition to all flags that the CLI supports, which means that all of those would have to be supported for future releases

kristleifur · 2019-08-05T17:25:18Z

If that’s a problem, then if this is merged and announced and documented as experimental it doesn’t have to be supported.

I’m personally running the fork that has the feature and will do so for the foreseeable future.

I also pledge do my utmost to help do any possible work required to get this merged.

Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

Only handles the arguments already supported by `docker-compose build` Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

ulyssessouza · 2019-08-13T12:05:56Z

Hello @orisano. I'm sorry for the late response. As you can see, I just pushed 3 commits related to the internal meetings with @thaJeztah and the team.
The commits include:

Buildkit's nice TTY output by letting the --progress as "auto"
Replace --cli by and envvar called COMPOSE_NATIVE_BUILDER.
Reduce the arguments passed to docker build to the already present in docker-compose build

PTAL @thaJeztah

Please note this feature is EXPERIMENTAL and could be removed at any new version

changes done

thaJeztah · 2019-08-20T09:47:30Z

Thanks @ulyssessouza - that looks good for a start!

The only thing I'm thinking is if we should add the --progress flag to compose as well, so that people can use --progress=plain in situations where a terminal is present, but you want the output in a more parsable format (I know we've done so in CI, where a TTY may be attached, you don't need the fancy output)

That could be done as a follow-up though (we'll need follow-ups to discuss / implement other flags anyway)

I haven't tried building/testing this PR yet; did you try if setting DOCKER_BUILDKIT=1 / DOCKER_BUILDKIT=0 as (environment) variable works as expected?

DOCKER_BUILDKIT=1 docker-compose build ...

DOCKER_BUILDKIT=0 docker-compose build ...

We'll also need to make sure the docker-compose up --build follows the same behavior, otherwise the cache is not shared between both.

Some other thoughts;

Should we print a warning if someone has this feature enabled, and uses the --compress or --parallel flag? (I think both won't do anything when using the CLI for building?)
Possibly hide them if COMPOSE_NATIVE_BUILDER=1 (but that's a nice to have)

abhaychrungoo · 2019-08-21T04:03:14Z

I have taken this through some limited manual testing on a Linux x86 box

Would it be fair to argue that this PR should produce similar behavior for

docker-compose build && docker-compose up
docker-compose up --build

Currently, the first uses buildkit (w DOCKER_BUILDKIT=1, COMPOSE_NATIVE_BUILDER=1), but the second does not,

thaJeztah · 2019-08-22T08:34:45Z

Would it be fair to argue that this PR should produce similar behavior for

Thanks for testing it! Yes, I think it should work for both (but currently doesn't) (see my comment above yours)

@ulyssessouza is it difficult to make docker compose up --build also call out to the CLI for the actual build(s) ?

orisano · 2019-08-22T13:07:35Z

orisano@aac7193
It supports docker-compose up --build

ulyssessouza · 2019-08-22T14:56:07Z

Hello all!

Now that the requirements of the PR/issue are quite solid, I just opened a new PR to manage this in a clear way.
The new one includes all the previous commits and the support for docker-compose up --build that @orisano just added squashed in a single commit
So to continue the review of feature, please refer to #6865

Once again, thanks a lot for the contribution @orisano !

GordonTheTurtle added the dco/no label Mar 15, 2019

orisano force-pushed the feat-add-exec-build branch from 86b5fa7 to 6c78203 Compare March 15, 2019 11:30

GordonTheTurtle removed the dco/no label Mar 15, 2019

AkihiroSuda reviewed Mar 15, 2019

View reviewed changes

AkihiroSuda mentioned this pull request Mar 15, 2019

Docker compose support moby/buildkit#685

Closed

GordonTheTurtle added the dco/no label Mar 16, 2019

AkihiroSuda mentioned this pull request Apr 3, 2019

Support BuildKit docker/docker-py#2230

Open

thaJeztah reviewed Apr 4, 2019

View reviewed changes

orisano force-pushed the feat-add-exec-build branch 2 times, most recently from 36ed2fa to 19b5db9 Compare April 5, 2019 07:36

GordonTheTurtle removed the dco/no label Apr 5, 2019

ulyssessouza reviewed Apr 5, 2019

View reviewed changes

compose/service.py Outdated Show resolved Hide resolved

compose/service.py Outdated Show resolved Hide resolved

GordonTheTurtle added the dco/no label Apr 16, 2019

orisano force-pushed the feat-add-exec-build branch from 4b6ead0 to dc13745 Compare April 17, 2019 13:26

GordonTheTurtle removed the dco/no label Apr 17, 2019

ulyssessouza suggested changes Apr 17, 2019

View reviewed changes

compose/service.py Outdated Show resolved Hide resolved

ulyssessouza previously requested changes Apr 24, 2019

View reviewed changes

compose/service.py Outdated Show resolved Hide resolved

orisano force-pushed the feat-add-exec-build branch 2 times, most recently from 85a986a to dc846f7 Compare April 24, 2019 14:34

orisano force-pushed the feat-add-exec-build branch from dc846f7 to 69415b1 Compare April 25, 2019 11:22

orisano and others added 10 commits August 13, 2019 13:54

feat: add --exec option

fa7c858

Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>

refactor: change image_id detection approach

5562a86

Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>

feat: add iidfile check

16b6475

Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>

chore: path to required

3864ba1

Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>

refactor: avoid shadowing

dfe0313

Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>

style: to sorted

35696ad

Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>

refactor: rename exec to cli

278ad5d

Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>

Fix buildkit tty output

202fb13

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

Replace --cli by envvar COMPOSE_NATIVE_BUILDER detection

a0a6854

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

Reduce arguments passed to docker build

76e453d

Only handles the arguments already supported by `docker-compose build` Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

ulyssessouza force-pushed the feat-add-exec-build branch from 89cbe40 to 76e453d Compare August 13, 2019 11:57

ulyssessouza self-requested a review August 14, 2019 08:48

rumpl mentioned this pull request Aug 22, 2019

requirements: update jsonschema dependency #6631

Merged

ulyssessouza mentioned this pull request Aug 22, 2019

Add support to CLI build #6865

Merged

3 tasks

ulyssessouza closed this Aug 22, 2019

thaJeztah mentioned this pull request Oct 25, 2019

--progress=json docker/buildx#178

Closed

thaJeztah mentioned this pull request Dec 5, 2019

Proposal: rely on docker CLI #7078

Closed

thaJeztah mentioned this pull request May 10, 2020

Support extra_hosts in _CLIBuilder #7323

Closed

Conversation

orisano commented Mar 15, 2019

Uh oh!

GordonTheTurtle commented Mar 15, 2019

Uh oh!

AkihiroSuda commented Mar 15, 2019

Uh oh!

AkihiroSuda Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

orisano Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

orisano Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

orisano Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

docteurklein commented Mar 18, 2019

Uh oh!

mipearson commented Mar 30, 2019

Uh oh!

tonistiigi commented Apr 3, 2019

Uh oh!

Uh oh!

thaJeztah Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ulyssessouza commented Apr 24, 2019

Uh oh!

Uh oh!

ulyssessouza commented Apr 25, 2019

Uh oh!

orisano commented Apr 26, 2019

Uh oh!

ulgens commented May 1, 2019

Uh oh!

ulgens commented May 1, 2019

Uh oh!

CpuID commented Jul 21, 2019

Uh oh!

jamesharris-garmin commented Jul 31, 2019

Uh oh!

cristicbz commented Aug 5, 2019

Uh oh!

thaJeztah commented Aug 5, 2019

Uh oh!

kristleifur commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulyssessouza commented Aug 13, 2019

Uh oh!

thaJeztah commented Aug 20, 2019

Uh oh!

abhaychrungoo commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Aug 22, 2019

Uh oh!

orisano commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulyssessouza commented Aug 22, 2019

Uh oh!

Reviewers

orisano Mar 15, 2019 •

edited

Loading

orisano Mar 15, 2019 •

edited

Loading

kristleifur commented Aug 5, 2019 •

edited

Loading

abhaychrungoo commented Aug 21, 2019 •

edited

Loading

orisano commented Aug 22, 2019 •

edited

Loading