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.

Sign up for GitHub

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

Jump to bottom

#231 Invoking kompose --bundle X.dab convert --stdout will produce tw… #338

Merged
merged 1 commit into from
Dec 18, 2016
Merged

#231 Invoking kompose --bundle X.dab convert --stdout will produce tw… #338

merged 1 commit into from
Dec 18, 2016

Conversation

cab105
Copy link
Contributor

@cab105 cab105 commented Dec 14, 2016

…o differently ordered results

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 14, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 35.387% when pulling 39488b9 on cab105:dab-ordering into 65e19e3 on kubernetes-incubator:master.

cdrage
cdrage requested changes Dec 15, 2016
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

LGTM other than spelling error

pkg/transformer/kubernetes/kubernetes.go
@@ -332,7 +333,15 @@ func (k *Kubernetes) Transform(komposeObject kobject.KomposeObject, opt kobject.
// this will hold all the converted data
var allobjects []runtime.Object

for name, service := range komposeObject.ServiceConfigs {
// Need to ensure that the kerbenertes objects are in a consistent order
Copy link
Member

Choose a reason for hiding this comment

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

erh meh gerd, kerbenertes! just a small spelling error :)

@cab105 cab105 force-pushed the dab-ordering branch 3 times, most recently from 3c77ade to 47def4b Compare December 15, 2016 20:56
@cab105
Copy link
Contributor Author

cab105 commented Dec 15, 2016

Whoopsie. Knew I needed another cup of coffee before pushing.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 35.891% when pulling 24cbd52 on cab105:dab-ordering into 78845d3 on kubernetes-incubator:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 35.891% when pulling 01f7e89 on cab105:dab-ordering into 78845d3 on kubernetes-incubator:master.

cdrage
cdrage approved these changes Dec 16, 2016
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

LGTM. Although should we add a unit test for this?

@cab105
Copy link
Contributor Author

cab105 commented Dec 16, 2016

Normally yes, however this bug was the result of adding in unit tests for dab/dsb support (See #241)

@cdrage
Copy link
Member

cdrage commented Dec 16, 2016

@cab105 ah k, merge away then? 👍

@cab105
Copy link
Contributor Author

cab105 commented Dec 16, 2016

@cdrage. Unfortunately blocked. Will need @ngtuna to bless this.

@ngtuna
Copy link
Contributor

ngtuna commented Dec 18, 2016

@cab105 Done. Go ahead man :-)

@cab105 cab105 merged commit 072d481 into kubernetes:master Dec 18, 2016
@cab105 cab105 deleted the dab-ordering branch December 18, 2016 14:02
@cab105
Copy link
Contributor Author

cab105 commented Dec 18, 2016

Thanks @ngtuna!

@cdrage cdrage mentioned this pull request Dec 22, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ngtuna ngtuna ngtuna approved these changes

@cdrage cdrage cdrage approved these changes

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cab105 @coveralls @cdrage @ngtuna @k8s-ci-robot