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

Add support for labels during build #4735

Closed

Conversation

ColinHebert
Copy link

@ColinHebert ColinHebert commented Apr 13, 2017

Signed-off-by: Colin Hebert <hebert.colin@gmail.com>
@ColinHebert
Copy link
Author

ColinHebert commented Apr 13, 2017

I'm not 100% sure that's the best way to write the test. I also used this in my dev environment to make sure it works:

docker-compose.yaml

version: '3.2'

services:
  web:
    build:
      context: .
      labels:
        test: blah
        test2: test

Dockerfile

FROM ubuntu

Execution

root# docker-compose build
Building web
Step 1/2 : FROM ubuntu
 ---> 0ef2e08ed3fa
Step 2/2 : LABEL "test" 'blah' "test2" 'test'
 ---> Using cache
 ---> 6c6c2d57f195
Successfully built 6c6c2d57f195

Signed-off-by: Colin Hebert <hebert.colin@gmail.com>
Signed-off-by: Colin Hebert <hebert.colin@gmail.com>
Signed-off-by: Colin Hebert <hebert.colin@gmail.com>
@shin- shin- requested a review from dnephin April 14, 2017 19:00
@shin-
Copy link

shin- commented Apr 14, 2017

Thanks!

I think if you want to add something to the v3 format, you should submit a PR in the Engine repo first - they're the source of truth as far as v3 is concerned.

@ColinHebert
Copy link
Author

ColinHebert commented Apr 14, 2017

Uh? That sounds a bit.. unwieldy.
In any case, I've opened moby/moby#32632 that updates the docker-compose schema.

Copy link

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

@ColinHebert
Copy link
Author

@dnephin @shin-, with the PR being approved (but not merged) on the moby/moby repo, what are the next steps to get this merged?

@ColinHebert
Copy link
Author

@shin- would it be possible to get this one merged for the next release?

@shin-
Copy link

shin- commented May 5, 2017

Ah yes, sorry - it just needs to be moved to the new 3.3 schema as per moby/moby#32972

@shin- shin- added this to the 1.14.0 milestone May 5, 2017
@ColinHebert
Copy link
Author

ColinHebert commented May 6, 2017

I see, how do you suggest we proceed?
There are other PRs that are attempting to modify the schema (see moby/moby#32743) are we importing the schema each time it's modified?

Also note that the schemas now live in docker/cli due to moby/moby#32694; see docker/cli#32 to get 3.3 in docker-cli

@shin-, @dnephin, should a different approach be devised here, where schemas would be held in a different repository which is "git submoduled" into both the cli and the compose project? To avoid divergence of the schemas between the two projects?
Simple changes like this one (it's literally two lines, one in the schema, one in service.py) should be easy to do. So far it's required 4 PRs across 3 different repository and even now, 23 days after the original PR was opened, it is still unclear how it's going to work :(

@shin- shin- mentioned this pull request May 23, 2017
@shin-
Copy link

shin- commented May 23, 2017

I don't think we are at a point where submodule would actually improve the workflow here. The reorganization at the engine level with the new Moby project and the docker/cli project split has caused this process to be a bit more involved than usual, but it's not the norm.

Either way: Merged via #4858

@shin- shin- closed this May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants