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

Kompose will read input from stdin #871

Merged
merged 1 commit into from
Jun 6, 2018
Merged

Kompose will read input from stdin #871

merged 1 commit into from
Jun 6, 2018

Conversation

surajnarwade
Copy link
Contributor

Resolves issue #870

for example,

$ cat docker-compose.yaml | kompose convert -f -
INFO Kubernetes file "frontend-service.yaml" created
INFO Kubernetes file "redis-master-service.yaml" created
INFO Kubernetes file "redis-slave-service.yaml" created
INFO Kubernetes file "frontend-deployment.yaml" created
INFO Kubernetes file "redis-master-deployment.yaml" created
INFO Kubernetes file "redis-slave-deployment.yaml" created

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 14, 2017
@cdrage
Copy link
Member

cdrage commented Nov 14, 2017

Please add WIP and begin work on adding tests 👍

@surajnarwade surajnarwade changed the title Kompose will read input from stdin [WIP] Kompose will read input from stdin Nov 15, 2017
@k8s-ci-robot 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 Nov 15, 2017
@surajnarwade
Copy link
Contributor Author

@cdrage I tried writing functional test but it's not working ?
can you provide me some pointer ?

@surajnarwade
Copy link
Contributor Author

@cdrage , unit tests and command line test didn't work :( so I have added integration test (test-k8s).
Please review

cdrage
cdrage requested changes Nov 28, 2017
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.

I believe we should add cmd tests rather than integration tests. I'll see what I can do to help / investigate this, this week.

script/test_k8s/kubernetes.sh Outdated
@@ -104,6 +104,13 @@ test_k8s() {
echo -e "\n${RED}kompose down -f $f ${NC}\n"
./kompose down -f $f
done

echo -e "\nTesting stdin to kompose\n"
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be adding these tests to integration tests... But more within CMD line tests.

Copy link
Member

Choose a reason for hiding this comment

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

Argh. I saw what we did within #868 , see my comment there too. I'm fine with these tests (for now) but it's a bit of a hack / needs to refactor. But let's put that on our future agenda.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdrage sure, I tried adding CMD line tests for stdin but got no success, so, I added here

@cdrage cdrage mentioned this pull request Nov 28, 2017
Closed
@surajnarwade surajnarwade force-pushed the stdin branch 2 times, most recently from f5c8ff8 to 02da34b Compare December 12, 2017 10:38
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 12, 2017
@surajnarwade surajnarwade force-pushed the stdin branch 6 times, most recently from f957b21 to 36abaea Compare December 13, 2017 10:53
@surajnarwade surajnarwade changed the title [WIP] Kompose will read input from stdin Kompose will read input from stdin Dec 14, 2017
@k8s-ci-robot 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 Dec 14, 2017
@surajnarwade surajnarwade self-assigned this Dec 14, 2017
@surajnarwade
Copy link
Contributor Author

@cdrage all green :)

@surajnarwade
Kompose will read input from stdin
6d4c8f9 
Resolves issue kubernetes#870

for example,

```
$ cat docker-compose.yaml | kompose convert -f -
INFO Kubernetes file "frontend-service.yaml" created
INFO Kubernetes file "redis-master-service.yaml" created
INFO Kubernetes file "redis-slave-service.yaml" created
INFO Kubernetes file "frontend-deployment.yaml" created
INFO Kubernetes file "redis-master-deployment.yaml" created
INFO Kubernetes file "redis-slave-deployment.yaml" created
```

Added integration test for the same.
`
@surajnarwade
Copy link
Contributor Author

cc @cdrage

@hangyan hangyan merged commit 5a37474 into kubernetes:master Jun 6, 2018
@cdrage
Copy link
Member

cdrage commented Jun 6, 2018

@hangyan I would of liked to have reviewed this before merging.. I'm against adding "short mode" to the testing suite. If you look at the code too there's this problem: https://github.com/kubernetes/kompose/pull/871/files#diff-aab6ee46f79d5d6c0c930f2773b8be29R17 where the test actually isn't being tested..

@hangyan
Copy link
Contributor

hangyan commented Jun 6, 2018

@cdrage Sorry, I have seen you approve this, and I have reviewed this too...Didn't seen that.. My bad.

@cdrage
Copy link
Member

cdrage commented Jun 6, 2018

@hangyan Ah! I'm sorry. It looks like I did approve this.. I think I meant to do the request changes button haha. My mistake! It's totally my fault.

I've opened up a PR to reverse the -short changes #1037

@hangyan hangyan mentioned this pull request Aug 9, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdrage cdrage cdrage approved these changes

Assignees

@surajnarwade surajnarwade

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@surajnarwade @cdrage @hangyan @k8s-ci-robot