Added support for build key in v3 #846

surajnarwade · 2017-10-11T07:25:32Z

Resolves #636

This PR will add support for build in docker compose v3, as docker/cli#481 got merged now.
also this will update sirupsen, docker/cli, docker/libcompose in glide,
Also changed Sirupsen with sirupsen in all kompose packages as well as in
docker/distribution packages

cdrage · 2017-10-11T13:41:12Z

@surajnarwade
Please update commit description message (more detail, not just resolves X).

Tests would be good too!

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

cdrage · 2017-10-27T16:17:26Z

Hey @surajnarwade I don't see why there is an issue with this. You are only updating Version 3 of Docker Compose which uses docker/cli, you shouldn't be touching / using docker/libcompose.

I also see that docker/cli is already using the most up-to-date logrus here: docker/cli@e3b7700

What issues are you running into? Please investigate and let me know!

cdrage · 2017-10-27T16:20:26Z

Also the error is just a simple renaming of Sirupsen in completion.go:

cmd/completion.go:9:2: cannot find package "github.com/Sirupsen/logrus" in any of:
	/home/travis/gopath/src/github.com/kubernetes/kompose/vendor/github.com/Sirupsen/logrus (vendor tree)
	/home/travis/.gimme/versions/go1.6.linux.amd64/src/github.com/Sirupsen/logrus (from $GOROOT)
	/home/travis/gopath/src/github.com/Sirupsen/logrus (from $GOPATH)
make: *** [test-unit-cover] Error 1

cdrage · 2017-10-27T16:22:08Z

After the rename it seems to work:

github.com/kubernetes/kompose  master ✔                                                                                                                                                                                                                                  22h19m  
▶ git-pull-pr 846
Upstream:  kubernetes/kompose
Already on 'master'
error: branch 'pr_846' not found.
remote: Counting objects: 102, done.
remote: Compressing objects: 100% (22/22), done.
remote: Total 102 (delta 55), reused 83 (delta 55), pack-reused 19
Receiving objects: 100% (102/102), 70.32 KiB | 0 bytes/s, done.
Resolving deltas: 100% (55/55), completed with 49 local objects.
From github.com:kubernetes/kompose
 * [new ref]         refs/pull/846/head -> pr_846
Switched to branch 'pr_846'

github.com/kubernetes/kompose  pr_846 ✔                                                                                                                                                                                                                                      2d  
▶ vim

github.com/kubernetes/kompose  pr_846 ✗                                                                                                                                                                                                                                    2d ⚑  
▶ make bin
go build -ldflags="-w -X github.com/kubernetes/kompose/cmd.GITCOMMIT=efb7552" -o kompose main.go

github.com/kubernetes/kompose  pr_846 ✗                                                                                                                                                                                                                                    2d ⚑  
▶

surajnarwade · 2017-11-08T08:38:39Z

@cdrage after renaming it works locally but not in CI
after investing I found out that, we have to update openshift from vendor, which may cause trouble

cdrage · 2017-11-08T13:05:53Z

Hey @surajnarwade See my previous comment up-above. You don't need to update libcompose to make this work. Only docker/cli, as that's where my commit upstream was merged in for build support. No need to edit / mess around with libcompose vendoring. Version 3 only deals with docker/cli, it doesn't touch docker/libcompose.

cdrage · 2017-11-08T14:12:51Z

glide.yaml

@@ -24,15 +24,15 @@ import:

 # We use libcompose to parse v1 and v2 of Docker Compose
 - package: github.com/docker/libcompose
-  version: 4a647d664afbe05c41455c9d534d8239671eb46a
+  version: 57bd716502dcbe1799f026148016022b0f3b989c


don't update libcompose, you don't need to.

Sirupsen :(

cdrage · 2017-11-09T15:57:37Z

To give an update of what's happening after talking to @surajnarwade

To update docker/cli we had to update vendoring. To update vendoring with the logrus errors, we had to update libcompose. To update libcompose, we had to update OpenShift vendoring due to conflicts, now we have to update and somehow migrate from 1.4 of OpenShift to 3.2 (I believe) in order to get all of this to work. 👍

surajnarwade · 2017-11-20T14:07:18Z

@cdrage @kadel instead of updating openshift, I have replaced Sirupsen with sirupsen in docker/distribution

cdrage · 2017-11-20T16:44:00Z

@surajnarwade Can you please provide details on what you did?

Was it a PR that updated docker/distribution?

Did you manually edit the vendoring files?

Have you updated Makefile to sed replace Sirupsen to sirupsen, or anything else for that matter?

surajnarwade · 2017-11-21T09:01:33Z

@cdrage @kadel ,To give brief idea about this PR,

I have Updated docker/cli to get latest changes that @cdrage pushed for build v3 support

these changes comes with sirupsen case collsion problem, so updated docker/libcompose as well

after fixing sirupsen case collision, it again occured with package docker/distribution which is dependency of openshift/origin, since updating openshift/origin is too much tedious task.

To tackle this situation, I updated imports from kompose manually and docker/distribution (from vendor) by adding line in Makefile and Now kompose seems to be working fine :)

I also added this thing to Makefile with make vendor-update

cdrage · 2017-11-21T13:05:41Z

Awesome 👍 thanks for the detailed explanation!

One last thing, could you put the make vendor-update updates to Makefile into a separate commit so we can see what changes you've made? So in total, there should be three commits. One to the code, one to the Makefile and the other actually updating the vendoring files.

surajnarwade · 2017-11-21T13:24:19Z

@cdrage gotcha

surajnarwade · 2017-11-22T12:16:56Z

@cdrage now you can easily review it :)

cdrage · 2017-11-22T14:28:51Z

@surajnarwade Your makefile commit is messy: c4044b5

Seems like you included some vendor (libcompose) files and there are some code that you're modifying in pkg...

So to go into detail:
Commit 1: Updates to Kedge code (pkg, renaming sirupsen, etc.)
Commit 2: Updates to Makefile (and that's it!)
Commit 3: Any updates to vendoring file.

Remember when you do all this to also update the commit descriptions / titles!

Updated `sirupsen`, `docker/cli`, `docker/libcompose` in `glide`, Also changed `Sirupsen` with `sirupsen` in all kompose packages as well as in `docker/distribution` packages

surajnarwade · 2017-11-27T07:00:27Z

@cdrage , Updated commits, review needed

cdrage · 2017-11-27T14:54:29Z

@surajnarwade

EDIT: Code / commits LGTM so far, awesome job! 👍 Just need to make CI pass now.

surajnarwade · 2017-11-28T06:29:26Z

@cdrage all green :)

cdrage

Few things, I get an error when using the example with just kompose up:

test/fixtures/nginx-node-redis  pr_846 ✗                                                                                                                                                                                                                                   2d ◒  
▶ kompose up -f docker-compose-v3.yml 
FATA Error while deploying application: k.Transform failed: image key required within build parameters in order to build and push service 'nginx'

This is because I'm trying to build this locally.

You are right however... I don't think we are able to test this very well since we're unable to push to Docker Hub without docker login.

test/fixtures/nginx-node-redis  pr_846 ✗                                                                                                                                                                                                                                 2d ⚑ ◒  
▶ kompose up -f docker-compose-v3.yml              
INFO Build key detected. Attempting to build and push image 'cdrage/test-nginx' 
INFO Building image 'cdrage/test-nginx' from directory 'nginx' 
INFO Image 'cdrage/test-nginx' from directory 'nginx' built successfully 
INFO Pushing image 'cdrage/test-nginx:latest' to registry 'docker.io' 
INFO Attempting authentication credentials 'https://index.docker.io/v1/

Above works when I changed it to my username.

Otherwise, LGTM.

cdrage · 2017-11-29T17:16:39Z

script/test/cmd/tests.sh

+# Test BuildConfig v3
+cmd="kompose --provider=openshift -f $KOMPOSE_ROOT/script/test/fixtures/nginx-node-redis/docker-compose-v3.yml convert --stdout -j --build build-config"
+sed -e "s;%VERSION%;$version;g" -e "s;%CMD%;$cmd;g" -e "s;%URI%;$uri;g" -e "s;%REF%;$branch;g" $KOMPOSE_ROOT/script/test/fixtures/nginx-node-redis/output-os-template-v3.json > /tmp/output-os.json
+convert::expect_success_and_warning "kompose --provider=openshift -f $KOMPOSE_ROOT/script/test/fixtures/nginx-node-redis/docker-compose-v3.yml convert --stdout -j --build build-config" "/tmp/output-os.json" "$warning"


There should be another test added for Kubernetes.

cdrage · 2017-11-29T17:23:56Z

Code / implementation LGTM, works on my side perfectly.

After merging, can you please update the documentation (conversion.md document) as well as open an issue that we need to add a test for testing Kubernetes docker image building.

surajnarwade · 2017-11-30T05:46:52Z

Above works when I changed it to my username.

Thanks @cdrage :)

Added test for kubernetes as well
Ready to merge 🎉

cdrage · 2017-11-30T14:06:30Z

Phew. Thanks A LOT for your contribution @surajnarwade that was a lot of trouble-shooting. Merging in!

✓ for build v3 support since we merged in kubernetes#846

surajnarwade · 2017-11-30T14:44:35Z

Thanks @cdrage

✓ for build v3 support since we merged in kubernetes#846

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 11, 2017

surajnarwade changed the title ~~Added support for build key in v3~~ [WIP]Added support for build key in v3 Oct 11, 2017

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2017

surajnarwade force-pushed the buildv3 branch from da22254 to 61d8579 Compare October 12, 2017 13:38

cdrage mentioned this pull request Oct 12, 2017

Add build key support for v3 of Docker Compose #636

Closed

surajnarwade added the blocked label Oct 16, 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 force-pushed the buildv3 branch from 61d8579 to edf5da2 Compare October 25, 2017 09:41

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 25, 2017

surajnarwade force-pushed the buildv3 branch from edf5da2 to efb7552 Compare October 25, 2017 13:33

cdrage requested changes Nov 8, 2017

View reviewed changes

surajnarwade force-pushed the buildv3 branch 4 times, most recently from aab81ae to e8e5a10 Compare November 20, 2017 13:41

surajnarwade removed the blocked label Nov 20, 2017

surajnarwade force-pushed the buildv3 branch 3 times, most recently from 487008b to e6bc23d Compare November 21, 2017 08:45

surajnarwade force-pushed the buildv3 branch from 87aab39 to 9018fe8 Compare November 22, 2017 12:15

surajnarwade changed the title ~~[WIP]Added support for build key in v3~~ Added support for build key in v3 Nov 27, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2017

surajnarwade added 3 commits November 27, 2017 12:28

Updated Makefile with respective vendor-update commands

d02492a

Added sirupsen and updated all occurances

5de4aa8

Updated `sirupsen`, `docker/cli`, `docker/libcompose` in `glide`, Also changed `Sirupsen` with `sirupsen` in all kompose packages as well as in `docker/distribution` packages

Updated Vendoring

b3f2134

surajnarwade force-pushed the buildv3 branch from c4bb0a3 to 34bddc5 Compare November 27, 2017 06:59

surajnarwade force-pushed the buildv3 branch from 34bddc5 to 224da0c Compare November 27, 2017 09:58

surajnarwade force-pushed the buildv3 branch from 224da0c to 90cd6eb Compare November 28, 2017 06:17

cdrage reviewed Nov 29, 2017

View reviewed changes

cdrage approved these changes Nov 29, 2017

View reviewed changes

Added functional test

ada8af2

surajnarwade force-pushed the buildv3 branch from 90cd6eb to ada8af2 Compare November 30, 2017 05:45

cdrage merged commit 84be740 into kubernetes:master Nov 30, 2017

cdrage added a commit to cdrage/kompose that referenced this pull request Nov 30, 2017

n -> ✓ for build v3 support

80f8af0

✓ for build v3 support since we merged in kubernetes#846

cdrage mentioned this pull request Nov 30, 2017

n -> ✓ for build v3 support #878

Merged

cdrage added a commit to cdrage/kompose that referenced this pull request Nov 30, 2017

n -> ✓ for build v3 support

b953844

✓ for build v3 support since we merged in kubernetes#846

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

n -> ✓ for build v3 support

fb6abfe

✓ for build v3 support since we merged in kubernetes#846

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for build key in v3 #846

Added support for build key in v3 #846

surajnarwade commented Oct 11, 2017 •

edited

Loading

cdrage commented Oct 11, 2017

cdrage commented Oct 27, 2017

cdrage commented Oct 27, 2017

cdrage commented Oct 27, 2017

surajnarwade commented Nov 8, 2017

cdrage commented Nov 8, 2017 via email •

edited

Loading

cdrage Nov 8, 2017

surajnarwade Nov 9, 2017

cdrage commented Nov 9, 2017

surajnarwade commented Nov 20, 2017

cdrage commented Nov 20, 2017

surajnarwade commented Nov 21, 2017 •

edited

Loading

cdrage commented Nov 21, 2017

surajnarwade commented Nov 21, 2017

surajnarwade commented Nov 22, 2017

cdrage commented Nov 22, 2017 •

edited

Loading

surajnarwade commented Nov 27, 2017

cdrage commented Nov 27, 2017 •

edited

Loading

surajnarwade commented Nov 28, 2017

cdrage left a comment

cdrage Nov 29, 2017

cdrage commented Nov 29, 2017 •

edited

Loading

surajnarwade commented Nov 30, 2017 •

edited

Loading

cdrage commented Nov 30, 2017

surajnarwade commented Nov 30, 2017

Added support for build key in v3 #846

Added support for build key in v3 #846

Conversation

surajnarwade commented Oct 11, 2017 • edited Loading

cdrage commented Oct 11, 2017

cdrage commented Oct 27, 2017

cdrage commented Oct 27, 2017

cdrage commented Oct 27, 2017

surajnarwade commented Nov 8, 2017

cdrage commented Nov 8, 2017 via email • edited Loading

cdrage Nov 8, 2017

Choose a reason for hiding this comment

surajnarwade Nov 9, 2017

Choose a reason for hiding this comment

cdrage commented Nov 9, 2017

surajnarwade commented Nov 20, 2017

cdrage commented Nov 20, 2017

surajnarwade commented Nov 21, 2017 • edited Loading

cdrage commented Nov 21, 2017

surajnarwade commented Nov 21, 2017

surajnarwade commented Nov 22, 2017

cdrage commented Nov 22, 2017 • edited Loading

surajnarwade commented Nov 27, 2017

cdrage commented Nov 27, 2017 • edited Loading

surajnarwade commented Nov 28, 2017

cdrage left a comment

Choose a reason for hiding this comment

cdrage Nov 29, 2017

Choose a reason for hiding this comment

cdrage commented Nov 29, 2017 • edited Loading

surajnarwade commented Nov 30, 2017 • edited Loading

cdrage commented Nov 30, 2017

surajnarwade commented Nov 30, 2017

surajnarwade commented Oct 11, 2017 •

edited

Loading

cdrage commented Nov 8, 2017 via email •

edited

Loading

surajnarwade commented Nov 21, 2017 •

edited

Loading

cdrage commented Nov 22, 2017 •

edited

Loading

cdrage commented Nov 27, 2017 •

edited

Loading

cdrage commented Nov 29, 2017 •

edited

Loading

surajnarwade commented Nov 30, 2017 •

edited

Loading