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 stdin_open, tty support, add tests, fix #344 #350

Merged
merged 1 commit into from
Dec 28, 2016

Conversation

concaf
Copy link
Contributor

@concaf concaf commented Dec 27, 2016

This adds supports for stdin_open: bool and
tty: bool support for kubernetes and openshift
providers in kompose. This maps to the
template.Spec.Containers[0].Stdin and
template.Spec.Containers[0].TTY in Kubernets
world.

Also, added tests.

Fixes #344

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

Coverage Status

Coverage increased (+0.2%) to 45.953% when pulling 21a4421 on containscafeine:stdin_tty into 7341831 on kubernetes-incubator:master.

@@ -428,6 +434,8 @@ func (k *Kubernetes) InitPod(name string, service kobject.ServiceConfig) *api.Po
{
Name: name,
Image: service.Image,
Stdin: service.Stdin,
TTY: service.Tty,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@@ -60,6 +62,15 @@ func TestInitDeploymentConfig(t *testing.T) {
if spec.Spec.Triggers[1].ImageChangeParams.ContainerNames[0] != "myfoobarname" {
t.Errorf("Expected myfoobarname for name, actual %s", spec.Spec.Triggers[1].ImageChangeParams.ContainerNames[0])
}

if spec.Spec.Template.Spec.Containers[0].Stdin != true {
Copy link
Member

Choose a reason for hiding this comment

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

Can we create a separate test? Because, this test gets hidden and never shows on output as this was tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added another test instead of this!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 45.682% when pulling f219cbd on containscafeine:stdin_tty into 0370d66 on kubernetes-incubator:master.

func TestOpenShiftUpdateKubernetesObjects(t *testing.T) {
var object []runtime.Object
o := OpenShift{}
serviceConfig := newServiceConfig()
Copy link
Member

Choose a reason for hiding this comment

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

A log saying what this test runs will help.

Copy link
Member

Choose a reason for hiding this comment

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

Similar comments are missing in other tests!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed for TestOpenShiftUpdateKubernetesObjects, log messages for other tests should go in another PR. Tracked at #351

@surajssd
Copy link
Member

apart from those two comments LGTM!

This adds supports for stdin_open: bool and
tty: bool support for kubernetes and openshift
providers in kompose. This maps to the
template.Spec.Containers[0].Stdin and
template.Spec.Containers[0].TTY in Kubernets
world.

Also, added tests.

Fixes kubernetes#344
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 45.682% when pulling 6a151c6 on containscafeine:stdin_tty into 0370d66 on kubernetes-incubator:master.

@surajssd surajssd added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 28, 2016
@surajssd
Copy link
Member

@containscafeine awesome work 👍

@surajssd surajssd merged commit c80735c into kubernetes:master Dec 28, 2016
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. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants