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

Abstract out api.pod spec #434

Merged
merged 1 commit into from
Feb 22, 2017

Conversation

procrypt
Copy link

Fixes: #348
cc: @containscafeine @kadel @cdrage @surajssd

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 21, 2017
@procrypt procrypt force-pushed the abstract_out_api.PodSpec branch from ad7bd5c to d738b34 Compare February 21, 2017 12:59
@kadel
Copy link
Member

kadel commented Feb 21, 2017

PodSpec is also used in OpenShift transformer. It would be great to use podspec function also there.

@@ -104,6 +104,19 @@ func (k *Kubernetes) CheckUnsupportedKey(komposeObject *kobject.KomposeObject, u
return keysFound
}

// podspec creates the pod specification
func (k *Kubernetes) podspec(name string, service kobject.ServiceConfig) api.PodSpec {
Copy link
Member

Choose a reason for hiding this comment

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

should be podSpec (Go name conventions)

Copy link
Member

Choose a reason for hiding this comment

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

and even better might be initPodSpec similar to other init functions ;-)

Copy link
Author

Choose a reason for hiding this comment

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

@kadel @cdrage I'm really bad in picking up names 😉 changing it to initPodSpec

@procrypt
Copy link
Author

@kadel We can't use PodSpec function in OpenShift because PodSpec in OpenShift is different.
PodSpec in Kubernetes

	pod := api.PodSpec{
		Containers: []api.Container{
			{
				Name:  name,
				Image: service.Image,
			},
		},
	}

PodSpec in OpenShift

	pod := api.PodSpec{
		Containers: []api.Container{
			{
				Name:  name,
				Image: " ",
			},
		},
	}

@procrypt procrypt force-pushed the abstract_out_api.PodSpec branch 2 times, most recently from 42de5d7 to f7e5e62 Compare February 22, 2017 09:54
@kadel
Copy link
Member

kadel commented Feb 22, 2017

I think that if we are abstracting podSpec in one place it would be also great to have everywhere else.

If you change your initPodSpec function it can be used also in OpenShift.
You don't have to pass whole service kobject.ServiceConfig struct down to initPodSpec.
All you need is name and image. It could look like this:

func (k *Kubernetes) InitPodSpec(name string, image string) api.PodSpec {
... 

@procrypt procrypt force-pushed the abstract_out_api.PodSpec branch 5 times, most recently from 72f4964 to 4a97cc1 Compare February 22, 2017 14:43
@@ -104,6 +104,19 @@ func (k *Kubernetes) CheckUnsupportedKey(komposeObject *kobject.KomposeObject, u
return keysFound
}

// "InitPodSpec creates the pod specification"
Copy link
Member

Choose a reason for hiding this comment

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

remove " and it will be ok ;-)

Copy link
Author

Choose a reason for hiding this comment

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

@kadel tests are passing locally when I run make test, no idea what is going wrong in Travis.

@procrypt procrypt force-pushed the abstract_out_api.PodSpec branch from 4a97cc1 to 082e235 Compare February 22, 2017 14:56
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

LGTM

@cdrage
Copy link
Member

cdrage commented Feb 22, 2017

LGTM

@cdrage cdrage merged commit 082bd17 into kubernetes:master Feb 22, 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.

4 participants