Add 'build' to types.go #481

cdrage · 2017-08-29T20:55:59Z

This adds 'build' to types.go in order for projects that use docker/cli
to parse Docker Compose files to correctly retrieve build keys.

cdrage · 2017-08-29T20:58:36Z

ping @vdemeester @dnephin

This unblocks us on kubernetes/kompose#636

dnephin

Once the other fields are added please add them to the full-example.yml as well.

dnephin · 2017-08-29T20:59:34Z

cli/compose/types/types.go

+type BuildConfig struct {
+	Context    string
+	Dockerfile string
+	Args       map[string]*string


This looks incomplete. It should have all the fields from here:

cli/cli/compose/schema/data/config_schema_v3.4.json

Lines 84 to 90 in e600621

"context": {"type": "string"},

"dockerfile": {"type": "string"},

"args": {"$ref": "#/definitions/list_or_dict"},

"labels": {"$ref": "#/definitions/list_or_dict"},

"cache_from": {"$ref": "#/definitions/list_of_strings"},

"network": {"type": "string"},

"target": {"type": "string"}

@dnephin I'm following @vdemeester 's implementation, but sure, I'll add the other options.

dnephin · 2017-08-29T21:00:59Z

cli/compose/types/types.go

@@ -6,7 +6,6 @@ import (

 // UnsupportedProperties not yet supported by this implementation of the compose file
 var UnsupportedProperties = []string{
-	"build",


We need to keep build in the list of unsupported properties because it's still not used by the cli right now.

@dnephin Okay 👍

cdrage · 2017-08-29T21:08:28Z

Man you're fast, I'm just fixing the tests failing now @dnephin

I've already added the build keys from types.go to the full-example.

Other than that, it's adding the rest of the types from the schema document.

cdrage · 2017-08-29T21:19:57Z

Problem I have @dnephin in regards to adding build to unsupported is that it fails to build / be used... How is it possible to add build to the supported types but still have it "unsupported"? When we parse the YAML it still exits ( see the failing test in circleci)

dnephin · 2017-08-29T21:27:29Z

The test failure is because the example config in that test is using a string for build. If you update the test to be something like this, it should work:

build:
  context: ./web

Unsupported properties are only used as a warning, and that warning comes from the cli, not from the loader package, so it shouldn't affect you. We could probably move that list out of types.go into a different package, but I don't think its necessary right now.

vdemeester · 2017-08-30T13:10:34Z

Design LGTM 🐸

cdrage · 2017-08-30T13:42:38Z

Perfect, thank you @dnephin for the explanation. I wasn't too sure about removing build from the failing test case.

I've gone ahead and removed it, hopefully tests pass this time now!

Also greetings fellow Canadian 🇨🇦

cdrage · 2017-08-31T10:43:24Z

Adding WIP tag so I make sure to do two things:

Have tests pass
Add rest of the schema

cdrage · 2017-09-07T22:48:22Z

Hey @vdemeester @dnephin is it possible to help out with the failing test? Not too sure what's happening TBH.

dnephin

Yes, the test failures are related to changes in this PR.

One problem I notice is that there are tabs in your yaml examples, which are invalid. It must be spaces.

cdrage · 2017-09-08T12:52:10Z

@dnephin Finally passes! But looks like the tests flaked / need rebuilding on circleci. I'm going to update my commit message and see if it fixes it.

codecov-io · 2017-09-08T14:43:38Z

Codecov Report

Merging #481 into master will decrease coverage by 0.44%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
- Coverage   49.06%   48.62%   -0.45%     
==========================================
  Files         200      199       -1     
  Lines       16407    16396      -11     
==========================================
- Hits         8050     7972      -78     
- Misses       7938     8008      +70     
+ Partials      419      416       -3

cdrage · 2017-09-08T15:18:42Z

Ayyy, tests pass 👍

dnephin · 2017-09-08T15:27:12Z

cli/compose/types/types.go

-	CapAdd          []string `mapstructure:"cap_add"`
-	CapDrop         []string `mapstructure:"cap_drop"`
-	CgroupParent    string   `mapstructure:"cgroup_parent"`
+	Build           BuildConfig `mapstructure:"build"`


not a problem, but technically you don't need the mapstructure annotation here, since it defaults to build

dnephin · 2017-09-08T15:29:18Z

cli/compose/types/types.go

+	Dockerfile string
+	Args       MappingWithEquals
+	Labels     Labels
+	CacheFrom  StringList


This one will need a mapstructure: annotation because it defaults to using a dash instead of an underscore to separate the words.

dnephin · 2017-09-08T15:30:09Z

cli/compose/loader/full-example.yml

+      context: ./dir
+      dockerfile: Dockerfile
+      args:
+        foo: bar


Please add the other fields (target, network, cache_from, labels) to this as well.

thaJeztah · 2017-09-13T12:06:58Z

@cdrage can you address the review comments?

This adds 'build' to types.go in order for projects that use docker/cli to parse Docker Compose files to correctly retrieve `build` keys Signed-off-by: Charlie Drage <charlie@charliedrage.com>

cdrage · 2017-09-13T14:55:20Z

@thaJeztah @dnephin

Done! Updated the full-example.yml as well as the loader_test.go file with the aforementioned updates.

dnephin

LGTM

vdemeester

LGTM 🦁

Updated vendoring for getting changes from docker/cli for build key in v3 (since docker/cli#481 is merged now)

Resolves kubernetes#636 This PR will add support for `build` in docker compose v3. As docker/cli#481 got merged now

cdrage requested review from dnephin and vdemeester as code owners August 29, 2017 20:55

GordonTheTurtle added dco/no status/0-triage labels Aug 29, 2017

cdrage force-pushed the add-build-to-service-config branch from cb598dd to 081642f Compare August 29, 2017 20:57

GordonTheTurtle removed the dco/no label Aug 29, 2017

cdrage mentioned this pull request Aug 29, 2017

Add build key support for v3 of Docker Compose kubernetes/kompose#636

Closed

dnephin reviewed Aug 29, 2017

View reviewed changes

cdrage force-pushed the add-build-to-service-config branch 3 times, most recently from be81f80 to e0255ba Compare August 29, 2017 21:18

docker deleted a comment from GordonTheTurtle Aug 30, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Aug 30, 2017

cdrage force-pushed the add-build-to-service-config branch from e0255ba to a8c0201 Compare August 30, 2017 13:43

cdrage changed the title ~~Add 'build' to types.go~~ [WIP] Add 'build' to types.go Aug 31, 2017

cdrage force-pushed the add-build-to-service-config branch from a8c0201 to 26b406c Compare September 7, 2017 22:47

cdrage changed the title ~~[WIP] Add 'build' to types.go~~ Add 'build' to types.go Sep 7, 2017

dnephin reviewed Sep 8, 2017

View reviewed changes

cdrage force-pushed the add-build-to-service-config branch from 26b406c to 0645adf Compare September 8, 2017 12:30

cdrage force-pushed the add-build-to-service-config branch from 0645adf to 7dbd194 Compare September 8, 2017 12:52

dnephin reviewed Sep 8, 2017

View reviewed changes

cdrage force-pushed the add-build-to-service-config branch 2 times, most recently from c38a892 to 452cd21 Compare September 13, 2017 14:47

Add 'build' to types.go

9bdb076

This adds 'build' to types.go in order for projects that use docker/cli to parse Docker Compose files to correctly retrieve `build` keys Signed-off-by: Charlie Drage <charlie@charliedrage.com>

cdrage force-pushed the add-build-to-service-config branch from 452cd21 to 9bdb076 Compare September 13, 2017 14:47

dnephin approved these changes Sep 13, 2017

View reviewed changes

vdemeester added status/2-code-review and removed status/1-design-review labels Sep 13, 2017

vdemeester approved these changes Sep 13, 2017

View reviewed changes

vdemeester merged commit 2eb31e6 into docker:master Sep 13, 2017

vdemeester mentioned this pull request Sep 20, 2017

Fix loader error with different build syntax #544

Merged

surajnarwade mentioned this pull request Sep 26, 2017

Updated vendoring kubernetes/kompose#828

Merged

surajnarwade added a commit to surajnarwade/kompose that referenced this pull request Sep 26, 2017

Updated vendoring

bd53d8b

Updated vendoring for getting changes from docker/cli for build key in v3 (since docker/cli#481 is merged now)

thaJeztah mentioned this pull request Sep 26, 2017

Add unsupported key build #450

Closed

surajnarwade added a commit to surajnarwade/kompose that referenced this pull request Oct 12, 2017

Added support for build key in v3

61d8579

Resolves kubernetes#636 This PR will add support for `build` in docker compose v3. As docker/cli#481 got merged now

surajnarwade added a commit to surajnarwade/kompose that referenced this pull request Oct 25, 2017

Added support for build key in v3

c7d6bc6

Resolves kubernetes#636 This PR will add support for `build` in docker compose v3. As docker/cli#481 got merged now

surajnarwade mentioned this pull request Nov 27, 2017

Added support for build key in v3 kubernetes/kompose#846

Merged

procrypt pushed a commit to procrypt/kompose that referenced this pull request Feb 4, 2018

Added support for build key in v3

71638e7

Resolves kubernetes#636 This PR will add support for `build` in docker compose v3. As docker/cli#481 got merged now

thaJeztah added this to the 17.10.0 milestone Feb 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'build' to types.go #481

Add 'build' to types.go #481

cdrage commented Aug 29, 2017

cdrage commented Aug 29, 2017

dnephin left a comment

dnephin Aug 29, 2017

cdrage Aug 29, 2017

dnephin Aug 29, 2017

cdrage Aug 29, 2017

cdrage commented Aug 29, 2017

cdrage commented Aug 29, 2017

dnephin commented Aug 29, 2017

vdemeester commented Aug 30, 2017

cdrage commented Aug 30, 2017

cdrage commented Aug 31, 2017

cdrage commented Sep 7, 2017 •

edited

Loading

dnephin left a comment

cdrage commented Sep 8, 2017

codecov-io commented Sep 8, 2017 •

edited

Loading

cdrage commented Sep 8, 2017

dnephin Sep 8, 2017

dnephin Sep 8, 2017

dnephin Sep 8, 2017

thaJeztah commented Sep 13, 2017

cdrage commented Sep 13, 2017

dnephin left a comment

vdemeester left a comment

	"context": {"type": "string"},
	"dockerfile": {"type": "string"},
	"args": {"$ref": "#/definitions/list_or_dict"},
	"labels": {"$ref": "#/definitions/list_or_dict"},
	"cache_from": {"$ref": "#/definitions/list_of_strings"},
	"network": {"type": "string"},
	"target": {"type": "string"}

Add 'build' to types.go #481

Add 'build' to types.go #481

Conversation

cdrage commented Aug 29, 2017

cdrage commented Aug 29, 2017

dnephin left a comment

Choose a reason for hiding this comment

dnephin Aug 29, 2017

Choose a reason for hiding this comment

cdrage Aug 29, 2017

Choose a reason for hiding this comment

dnephin Aug 29, 2017

Choose a reason for hiding this comment

cdrage Aug 29, 2017

Choose a reason for hiding this comment

cdrage commented Aug 29, 2017

cdrage commented Aug 29, 2017

dnephin commented Aug 29, 2017

vdemeester commented Aug 30, 2017

cdrage commented Aug 30, 2017

cdrage commented Aug 31, 2017

cdrage commented Sep 7, 2017 • edited Loading

dnephin left a comment

Choose a reason for hiding this comment

cdrage commented Sep 8, 2017

codecov-io commented Sep 8, 2017 • edited Loading

Codecov Report

cdrage commented Sep 8, 2017

dnephin Sep 8, 2017

Choose a reason for hiding this comment

dnephin Sep 8, 2017

Choose a reason for hiding this comment

dnephin Sep 8, 2017

Choose a reason for hiding this comment

thaJeztah commented Sep 13, 2017

cdrage commented Sep 13, 2017

dnephin left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

cdrage commented Sep 7, 2017 •

edited

Loading

codecov-io commented Sep 8, 2017 •

edited

Loading