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

add BuildConfig support to kompose down #413

Merged
merged 1 commit into from
Feb 15, 2017

Conversation

procrypt
Copy link
Contributor

@procrypt procrypt commented Feb 6, 2017

kompose --provider openshift up
INFO Successfully created Service: foo            
INFO Successfully created DeploymentConfig: foo   
INFO Successfully created ImageStream: foo        
INFO Successfully created BuildConfig: foo 
kompose --provider openshift down
INFO Buildconfig using https://github.com/kubernetes-incubator/kompose.git::master as source. 
INFO Successfully deleted service: foo            
INFO Successfully deleted DeploymentConfig: foo   
INFO Successfully deleted ImageStream: foo        
INFO Successfully deleted BuildConfig: foo   

docker-compose file used is docker-compose.yml

@kadel @cdrage @containscafeine Review please.
Fixes #382

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2017
@concaf
Copy link
Contributor

concaf commented Feb 6, 2017

@procrypt mind adding unit tests for this?

@procrypt procrypt force-pushed the add_buildConfig_to_kompose_down branch from 196bebd to 8468126 Compare February 6, 2017 11:54
@kadel
Copy link
Member

kadel commented Feb 8, 2017

@procrypt mind adding unit tests for this?

I'm afraid that we still don't have a way how to test this. :-(
As it will require running or mocking cluster :-(

@procrypt
Copy link
Contributor Author

procrypt commented Feb 8, 2017

@kadel I'm trying something don't know if it the right thing to do, will update the WIP later today.

@concaf
Copy link
Contributor

concaf commented Feb 13, 2017

@procrypt - @kadel is correct, we will need to mock out the entire OpenShift client for this :(

I can think of a couple of ways to go about this -

  • mock out just the parts we are testing?
  • see in openshift/origin upstream about how are they mocking out the client, and if possible just use that?
  • not unit test the deploy bits at all, and rely on functional tests to deploy on the cluster?

I don't know what is the right way forward, but yep, mocking out the entire client might take a sprint or two :)

Thoughts on this? - @surajssd @kadel @cdrage @rtnpro

@pradeepto
Copy link

I would recommend either mock required bits of API or see if we can use/reuse bits from openshift.

@cdrage
Copy link
Member

cdrage commented Feb 13, 2017

@pradeepto @containscafeine no need to "mock" it, you can run an Openshift cluster within TravisCI.

@surajssd
Copy link
Member

@pradeepto @containscafeine no need to "mock" it, you can run an Openshift cluster within TravisCI.

@cdrage then that won't be called unit tests right? If we are planning on doing complete e2e test with kompose in golang, which involves bringing up clusters then that should be a complete new thing.
So that will also require writing all the harness code of bringing up clusters.

@pradeepto
Copy link

@cdrage What @surajssd said. If we are bringing up oc cluster, then we are adding a dependency on the unit tests which doesn't sound right to me. If the cluster doesn't come up anytime, then the tests will fail which is technically not a test failure but an infrastructure failure.

@cdrage
Copy link
Member

cdrage commented Feb 14, 2017

@pradeepto Using versioned containers, it's unlikely to fail coming up (from my experience "mocking" the cluster)

@kadel
Copy link
Member

kadel commented Feb 14, 2017

I think that we can discuss this elsewhere.

I don't think we have block this PR on tests.
We currently don't have any solution for testing this and this is not adding any new functionality just fixing what was missing so I would say we can merge this. What do you think?

@surajssd
Copy link
Member

@kadel +1

@kadel kadel merged commit 7dac1a3 into kubernetes:master Feb 15, 2017
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