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

Update vendoring as well as libcompose #356

Merged
merged 3 commits into from
Jan 12, 2017

Conversation

cdrage
Copy link
Member

@cdrage cdrage commented Jan 3, 2017

This commit updates libcompose in order to merge in
docker/libcompose#423 which affected
#92 by not
erroring out when an image name wasn't provided.

Closes #92

As well as knocks out the last required milestone for a 0.2.1 release
https://github.com/kubernetes-incubator/kompose/milestone/2

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 3, 2017
@cdrage
Copy link
Member Author

cdrage commented Jan 3, 2017

ping @ngtuna as this closes your issue.

With this PR, we will have completed all milestones for 0.2.1 which unblocks us for releasing a new version of kompose. https://github.com/kubernetes-incubator/kompose/milestone/2

@coveralls
Copy link

Coverage Status

Coverage remained the same at 46.617% when pulling 63f44be on cdrage:update-vendoring into b059c44 on kubernetes-incubator:master.

@ngtuna
Copy link
Contributor

ngtuna commented Jan 3, 2017

+1 @cdrage.

I was inserting a manual check for image while libcompose has not been updated, but your PR is here so we will work on this. Now I'm considering the test cases failing.

@kadel kadel added this to the v0.3 release milestone Jan 4, 2017
This commit updates libcompose in order to merge in
docker/libcompose#423 which affected
kubernetes#92 by not
erroring out when an image name wasn't provided.

Closes kubernetes#92

As well as knocks out the last required milestone for a 0.2.1 release
https://github.com/kubernetes-incubator/kompose/milestone/2
@coveralls
Copy link

Coverage Status

Coverage remained the same at 46.617% when pulling 5703942 on cdrage:update-vendoring into 6e260ba on kubernetes-incubator:master.

@cdrage
Copy link
Member Author

cdrage commented Jan 5, 2017

@ngtuna @kadel So this seems to be failing due to:

▶ kompose --provider openshift -f /home/wikus/dropbox/dev/go/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/tty-true/docker-compose.yml convert --stdout -j                                                                                                     
etes-incubator/kompose/script/test/fixtures/tty-true/output-oc.json                                                                                                                                                                                                               
WARN[0000] Unsupported root level networks key - ignoring                                                                                                          
{                                                                                                                         
  "kind": "List",                         
  "apiVersion": "v1",                
  "metadata": {},                                                                                                                                                                                                                                                                 
  "items": [                                                                                                                                                                                                                                                                      
    {                               

appearing..

Going to investigate this more.. But looks like an update to libcompose added that warning.

@kadel
Copy link
Member

kadel commented Jan 6, 2017

@cdrage

There is a check that was testing root level networks before
https://github.com/kubernetes-incubator/kompose/blob/4f176b847ec75f61ab69bcc03f2b759a178df991/pkg/loader/compose/compose.go#L87

It looks like new libcompose version is doing something little bit different with networks :-(

@cdrage
Copy link
Member Author

cdrage commented Jan 6, 2017

@kadel yeah :( i saw, seems that it's something putting something into the file even though a network isn't there. I'll open up a separate PR for this to unblock this PR.

@kadel
Copy link
Member

kadel commented Jan 6, 2017

I don't think that we should merge this if it is giving warning about root level network even there is none in docker-compose file. We should include fix in this pr.

@cdrage
Copy link
Member Author

cdrage commented Jan 6, 2017

@kadel Done. I've added another commit to this PR that fixes the test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 46.744% when pulling 7cc7824 on cdrage:update-vendoring into 6e260ba on kubernetes-incubator:master.

Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

LGTM.
But it might be good to have another pair of eyes to check it.

@kadel kadel requested a review from ngtuna January 9, 2017 11:42
@cdrage
Copy link
Member Author

cdrage commented Jan 10, 2017

@ngtuna @surajssd can we get a quick review? :)

Due to changes to libcompose, NetworkConfigs now returns a default
network regardless if one has been provided or not.

An if statement is added that will debug output and ignore the default
network provided that it's the only network available.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 47.042% when pulling 39fbc4a on cdrage:update-vendoring into 7dbf00c on kubernetes-incubator:master.

@ngtuna
Copy link
Contributor

ngtuna commented Jan 11, 2017

@cdrage LGTM

@surajssd surajssd self-requested a review January 12, 2017 05:28
Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

@cdrage
Copy link
Member Author

cdrage commented Jan 12, 2017

@surajssd

Done. Have a quick look and give me the LGTM 👍 Then we can merge!

@cdrage
Copy link
Member Author

cdrage commented Jan 12, 2017

@containscafeine I'll have to open up a separate PR in regards to updating libcompose as for some odd reason you can't do glide update --strip-vendor in a detached HEAD state when you do a git rebase.

Mind doing a quick LGTM so I can press that merge button?

@concaf
Copy link
Contributor

concaf commented Jan 12, 2017

@cdrage sure, lookin' gooood to me!!!

@cdrage cdrage merged commit bf6085d into kubernetes:master Jan 12, 2017
@cdrage cdrage deleted the update-vendoring branch March 30, 2017 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants