Add Devfile Kube Struct Library Functions #44

maysunfaisal · 2020-11-09T23:26:47Z

Signed-off-by: Maysun J Faisal [email protected]

What does this PR do?

This PR copies over the refactored generator code from the Odo repo (redhat-developer/odo#4134)

What issues does this PR fix or reference?

Fixes devfile/api#184

Is your PR tested? Consider putting some instruction how to test your changes

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal · 2020-11-16T20:43:26Z

@andrewballantyne this is the PR for the Library functions which should cover those 4 shortlisted ones from https://github.com/openshift/console/pull/6321/files#diff-f8f657472eec48f6e5d2eb27f345b88687919d40900828812362ad5148d29683R35-R38

pkg/devfile/generator/utils.go

pkg/devfile/generator/generators_test.go

Signed-off-by: Maysun J Faisal <[email protected]>

adisky

@maysunfaisal Mostly looks good to me, just had two concerns.

adisky · 2020-11-18T12:40:19Z

pkg/devfile/generator/generators.go

+		}
+		container := getContainer(containerParams)
+
+		// If `mountSources: true` was set, add an empty dir volume to the container to sync the source to


Are we sure that all projects needs this? emptyDir volume?

This is just a comment, we are not doing any empty volume mounting. If mountSources is true, we just set PROJECTS_ROOT & PROJECT_SOURCE for the container.

I'll go through the comments to make sure they're updated and clean.

adisky · 2020-11-18T12:52:43Z

pkg/devfile/parser/data/v2/components.go

+
+// GetPortExposure iterate through all endpoints and returns the highest exposure level of all TargetPort.
+// exposure level: public > internal > none
+func (d *DevfileV2) GetPortExposure() map[int]v1.EndpointExposure {


not sure about this one in this package? is it intended to be directly consumed by library users? i saw generators code using this

True, it is used by the generator code atm. But the reasoning behind why it was moved to the parser pkg is because the func returns a map with the devfile type v1.EndpointExposure. We wanted to keep the generator pkg for only Kubernetes type functions.

Any thoughts?

exposed function should return kube objects, but if it is not intended to be used by consumer, we can move this into generator/utils

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal · 2020-11-18T19:24:34Z

@adisky @mik-dass I've added more generator funcs to the list like - GetDeployment, GetService, GetIngress, etc... odo currently calls and uses GetDeploymentSpec, GetServiceSpec, GetIngressSpec for its usage and wraps it around with Deployment, Service, Ingress, etc. to create them. I think if we have these funcs it will help odo have less redundant code because there may multiple places where we are creating these kubernetes objects with objectMeta and Spec definitions.. Let me know, what you think.

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal · 2020-11-23T15:11:42Z

@adisky @mik-dass Can this be LGTM'd?

Signed-off-by: Maysun J Faisal <[email protected]>

adisky

this way users need to make two calls

GetDeploymentSpec()

and then

GetDeployment()

for simplicity and consistency i think it is better to have one call and all exposed function names be like

GetDeploymentSpec()
GetPodSpec()
GetServiceSpec()
GetIngressSpec()

adisky · 2020-11-24T05:15:12Z

pkg/devfile/generator/generators.go

+func GetDeploymentSpec(deploySpecParams DeploymentSpecParams) *appsv1.DeploymentSpec {
+	deploymentSpec := &appsv1.DeploymentSpec{
+		Strategy: appsv1.DeploymentStrategy{
+			Type: appsv1.RecreateDeploymentStrategyType,
+		},
+		Selector: &metav1.LabelSelector{
+			MatchLabels: deploySpecParams.PodSelectorLabels,
+		},
+		Template: deploySpecParams.PodTemplateSpec,
+	}
+
+	return deploymentSpec
+}
+
+// DeploymentParams is a struct that contains the required data to create a deployment object
+type DeploymentParams struct {
+	TypeMeta       metav1.TypeMeta
+	ObjectMeta     metav1.ObjectMeta
+	DeploymentSpec appsv1.DeploymentSpec
+}
+
+// GetDeployment gets a deployment object
+func GetDeployment(deployParams DeploymentParams) *appsv1.Deployment {
+
+	deployment := &appsv1.Deployment{
+		TypeMeta:   deployParams.TypeMeta,
+		ObjectMeta: deployParams.ObjectMeta,
+		Spec:       deployParams.DeploymentSpec,
+	}
+
+	return deployment


this is getting confusing, can we have a single function instead?

GetDeployment GetDeploymentSpec

Currently odo uses GetDeploymentSpec and wraps the spec with deployment when odo goes to create it. There is 3-4 lines redundant code in odo for every kube resources. How do you feel about GetDeployment() being the exposed func and it takes inputs for all the spec generation? Because, we're also creating pod template spec with GetPodTemplateSpec and then creating a deployment spec with GetDeploymentSpec. So we have to do multiple func call anyways and this was how the generator code was reviewed and written when it was first put into odo, they wanted it very modular and granular.

If we have one main function GetDeployment/GetService where the func would take all the necessary inputs, this would mean, odo would need to be modified a bit more. I've also mentioned this in the PR here #44 (comment)

For example, like this https://github.com/openshift/odo/blob/master/pkg/kclient/deployments.go#L169

Or we can just merge this PR in with the GetDeploymentSpec, GetPodSpec, GetServiceSpec being the exposed functions and leave it upto the consumers to wrap the spec and object meta with the respective kubernetes resources in their own repo

Updated to have GetDeployment, GetService, GetIngress, etc as exposed func

adisky · 2020-11-24T05:16:49Z

pkg/devfile/generator/generators.go

+}
+
+// GetService gets the service
+func GetService(serviceParams ServiceParams) *corev1.Service {


same for

GetService GetServiceSpec

mik-dass · 2020-11-24T08:16:05Z

@adisky @mik-dass Can this be LGTM'd?

Please have a look at @adisky 's concerns. Rest looks OK to me.

Signed-off-by: Maysun J Faisal <[email protected]>

adisky · 2020-11-25T13:13:48Z

/lgtm

yangcao77

Adding lgtm label, since it's already approved by requested reviewers

openshift-ci-robot · 2020-11-25T15:07:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maysunfaisal, yangcao77

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maysunfaisal,yangcao77]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the do-not-merge/work-in-progress label Nov 9, 2020

openshift-ci-robot requested a review from johnmcollier November 9, 2020 23:26

maysunfaisal changed the title ~~Add first set of parser utils~~ Add Devfile Kube Struct Library Functions Nov 9, 2020

maysunfaisal marked this pull request as ready for review November 12, 2020 15:35

openshift-ci-robot removed the do-not-merge/work-in-progress label Nov 12, 2020

maysunfaisal requested review from adisky, mik-dass and reginapizza November 12, 2020 15:35

openshift-ci-robot added the approved label Nov 12, 2020

maysunfaisal mentioned this pull request Nov 12, 2020

(POC) Add devfile v2 outerloop flow to console openshift/console#6321

Closed

maysunfaisal added 3 commits November 12, 2020 17:23

Add first set of parser utils

5c438c3

Signed-off-by: Maysun J Faisal <[email protected]>

Add the generators code

ccd9d18

Signed-off-by: Maysun J Faisal <[email protected]>

Cleanup 1

8475ebc

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal force-pushed the 184-1 branch from e23b252 to 8475ebc Compare November 12, 2020 22:25

go mod tidy

dc31d1c

Signed-off-by: Maysun J Faisal <[email protected]>

mik-dass reviewed Nov 17, 2020

View reviewed changes

pkg/devfile/generator/utils.go Outdated Show resolved Hide resolved

mik-dass reviewed Nov 17, 2020

View reviewed changes

pkg/devfile/generator/generators_test.go Outdated Show resolved Hide resolved

PR feedback cleanup

5caf9fe

Signed-off-by: Maysun J Faisal <[email protected]>

adisky reviewed Nov 18, 2020

View reviewed changes

maysunfaisal added 2 commits November 18, 2020 12:17

Update and cleanup comments

d9eb318

Signed-off-by: Maysun J Faisal <[email protected]>

Add more generator funcs for kubernetes specs

8c7f6fe

Signed-off-by: Maysun J Faisal <[email protected]>

Update util func

1f3ac26

Signed-off-by: Maysun J Faisal <[email protected]>

andrewballantyne mentioned this pull request Nov 23, 2020

Devfile Add Flow openshift/console#7299

Merged

4 tasks

Add get schema ver func

f44cce9

Signed-off-by: Maysun J Faisal <[email protected]>

adisky reviewed Nov 24, 2020

View reviewed changes

Update exposed and unexposed generator func

7d80322

Signed-off-by: Maysun J Faisal <[email protected]>

openshift-ci-robot assigned adisky Nov 25, 2020

openshift-ci-robot added the lgtm label Nov 25, 2020

yangcao77 self-requested a review November 25, 2020 15:06

yangcao77 approved these changes Nov 25, 2020

View reviewed changes

openshift-ci-robot assigned yangcao77 Nov 25, 2020

maysunfaisal merged commit 6eabeab into devfile:master Nov 25, 2020

maysunfaisal mentioned this pull request Nov 25, 2020

Use the generators from devfile/library redhat-developer/odo#4209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Devfile Kube Struct Library Functions #44

Add Devfile Kube Struct Library Functions #44

maysunfaisal commented Nov 9, 2020

maysunfaisal commented Nov 16, 2020

adisky left a comment

adisky Nov 18, 2020

maysunfaisal Nov 18, 2020

adisky Nov 18, 2020

maysunfaisal Nov 18, 2020 •

edited

Loading

adisky Nov 19, 2020

maysunfaisal Nov 23, 2020

maysunfaisal commented Nov 18, 2020 •

edited

Loading

maysunfaisal commented Nov 23, 2020

adisky left a comment •

edited

Loading

adisky Nov 24, 2020

maysunfaisal Nov 24, 2020 •

edited

Loading

maysunfaisal Nov 24, 2020 •

edited

Loading

maysunfaisal Nov 24, 2020

adisky Nov 24, 2020

mik-dass commented Nov 24, 2020

adisky commented Nov 25, 2020

yangcao77 left a comment

openshift-ci-robot commented Nov 25, 2020

Add Devfile Kube Struct Library Functions #44

Add Devfile Kube Struct Library Functions #44

Conversation

maysunfaisal commented Nov 9, 2020

What does this PR do?

What issues does this PR fix or reference?

Is your PR tested? Consider putting some instruction how to test your changes

maysunfaisal commented Nov 16, 2020

adisky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal commented Nov 18, 2020 • edited Loading

maysunfaisal commented Nov 23, 2020

adisky left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

maysunfaisal Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass commented Nov 24, 2020

adisky commented Nov 25, 2020

yangcao77 left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 25, 2020

maysunfaisal Nov 18, 2020 •

edited

Loading

maysunfaisal commented Nov 18, 2020 •

edited

Loading

adisky left a comment •

edited

Loading

maysunfaisal Nov 24, 2020 •

edited

Loading

maysunfaisal Nov 24, 2020 •

edited

Loading