Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add --exec option #6584

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@orisano
Copy link

commented Mar 15, 2019

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

This PR is prototype. please give me feedback.

@GordonTheTurtle

This comment has been minimized.

Copy link

commented Mar 15, 2019

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.

@orisano orisano force-pushed the orisano:feat-add-exec-build branch from 86b5fa7 to 6c78203 Mar 15, 2019

@GordonTheTurtle GordonTheTurtle removed the dco/no label Mar 15, 2019

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

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

This comment has been minimized.

Copy link
@AkihiroSuda

This comment has been minimized.

Copy link
@orisano

orisano Mar 15, 2019

Author

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 comment has been minimized.

Copy link
@AkihiroSuda

AkihiroSuda Mar 15, 2019

Member

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

This comment has been minimized.

Copy link
@orisano

orisano Mar 15, 2019

Author

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

This comment has been minimized.

Copy link
@AkihiroSuda

AkihiroSuda Mar 15, 2019

Member

--iidfile?

This comment has been minimized.

Copy link
@orisano

orisano Mar 15, 2019

Author

I implemented to use an iidfile method.

@docteurklein

This comment has been minimized.

Copy link

commented Mar 18, 2019

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

This comment has been minimized.

Copy link

commented Mar 30, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

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

@AkihiroSuda AkihiroSuda referenced this pull request Apr 3, 2019

Open

Support BuildKit #2230

@@ -267,6 +267,7 @@ def build(self, options):
--build-arg key=val Set build-time variables for services.
--parallel Build images in parallel.
-q, --quiet Don't print anything to STDOUT
--exec Run docker build command

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Apr 4, 2019

Member

could you keep this sorted alphabetically?

This comment has been minimized.

Copy link
@orisano

orisano Apr 5, 2019

Author

currently, this codebase is not sorted alphabetically.
should I do it?

@@ -290,7 +291,8 @@ def build(self, options):
build_args=build_args,
gzip=options.get('--compress', False),
parallel_build=options.get('--parallel', False),
silent=options.get('--quiet', False)
silent=options.get('--quiet', False),
_exec=bool(options.get('--exec', False)),

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Apr 4, 2019

Member

here as well (I think)

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Apr 4, 2019

Member

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

orisano added some commits Mar 15, 2019

feat: add --exec option
Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>
refactor: change image_id detection approach
Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>

@orisano orisano force-pushed the orisano:feat-add-exec-build branch from 20c41c1 to 36ed2fa Apr 5, 2019

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

@orisano orisano force-pushed the orisano:feat-add-exec-build branch from 36ed2fa to 19b5db9 Apr 5, 2019

@GordonTheTurtle GordonTheTurtle removed the dco/no label Apr 5, 2019

@@ -1695,3 +1699,135 @@ def rewrite_build_path(path):
path = WINDOWS_LONGPATH_PREFIX + os.path.normpath(path)

return path


def exec_build(path=None, tag=None, quiet=False, fileobj=None,

This comment has been minimized.

Copy link
@ulyssessouza

ulyssessouza Apr 5, 2019

Collaborator

I think that like in docker build path should be mandatory.

This comment has been minimized.

Copy link
@ulyssessouza

ulyssessouza Apr 5, 2019

Collaborator

Also, that would be great if the order here was alphabetic to reflect docker build --help options list behavior.

This comment has been minimized.

This comment has been minimized.

Copy link
@ulyssessouza

ulyssessouza Apr 9, 2019

Collaborator

The point is that in this API path is optional because fileobj can be used instead and fileobj is not used in your PR. Checked in https://github.com/docker/docker-py/blob/37e096f6add7e26ada3d6840ce9a9ce341bbdf23/docker/api/build.py#L125

This comment has been minimized.

Copy link
@orisano

orisano Apr 11, 2019

Author

I understand it.
can i use incompatible signature?

This comment has been minimized.

Copy link
@orisano

orisano Apr 16, 2019

Author

@ulyssessouza I fixed it

Returns:
A generator for the build output.
"""
command_builder = _CommandBuilder()

This comment has been minimized.

Copy link
@ulyssessouza

ulyssessouza Apr 5, 2019

Collaborator

Same order comment here

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

@orisano orisano force-pushed the orisano:feat-add-exec-build branch from 4b6ead0 to dc13745 Apr 17, 2019

@GordonTheTurtle GordonTheTurtle removed the dco/no label Apr 17, 2019

Show resolved Hide resolved compose/service.py Outdated
refactor: avoid shadowing
Signed-off-by: Nao YONASHIRO <yonashiro@r.recruit.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.