-
Notifications
You must be signed in to change notification settings - Fork 771
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
Conversation
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 |
+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. |
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
63f44be
to
5703942
Compare
@ngtuna @kadel So this seems to be failing due to:
appearing.. Going to investigate this more.. But looks like an update to libcompose added that warning. |
There is a check that was testing root level networks before It looks like new libcompose version is doing something little bit different with networks :-( |
@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. |
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. |
@kadel Done. I've added another commit to this PR that fixes the test. |
There was a problem hiding this 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.
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.
7cc7824
to
39fbc4a
Compare
@cdrage LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdrage please uncomment this https://github.com/kubernetes-incubator/kompose/blob/master/script/test/cmd/tests.sh#L25
before we merge this!
Done. Have a quick look and give me the LGTM 👍 Then we can merge! |
@containscafeine I'll have to open up a separate PR in regards to updating libcompose as for some odd reason you can't do Mind doing a quick LGTM so I can press that merge button? |
@cdrage sure, lookin' gooood to me!!! |
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