Refactor & cleanup kclient generators code before migrating to devfile/parser #4134

maysunfaisal · 2020-10-22T20:15:53Z

What type of PR is this?
/kind cleanup
/kind code-refactoring

What does does this PR do / why we need it:
This PR cleans up kclient generators code so that it can be exported out to devfile/parser repo:

Moved the generator code from kclient/generators.go to kclient/generator/generators.go
Moved some of the adapter/utils.go code to kclient/generator/utils.go
Updated the kube adapter createOrUpdateComponent() logic around kclient refactoring
Added missing unit tests for some of the util functions

Which issue(s) this PR fixes:

Fixes #4131

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

Run the unit and integration tests

openshift-ci-robot · 2020-10-22T20:16:11Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

adisky · 2020-10-30T08:45:55Z

pkg/devfile/adapters/kubernetes/component/adapter.go

-	}
+	objectMeta := generator.CreateObjectMeta(componentName, a.Client.Namespace, labels, nil)
+	supervisordInitContainer := kclient.AddBootstrapSupervisordInitContainer()
+	initContainers := utils.GetPreStartInitContainers(a.Devfile, containers)


why we need to pass containers to get init containers?

containers is all the component containers returned from the devfile. PreStart events reference a command(which in turn reference a container component). So we scan thru all the containers that we have and append to initContainers

adisky · 2020-10-30T08:47:23Z

pkg/devfile/adapters/kubernetes/component/adapter.go

-	// Add PVC and Volume Mounts to the podTemplateSpec
-	err = kclient.AddPVCAndVolumeMount(podTemplateSpec, volumeNameToPVCName, containerNameToVolumes)
+	// Get PVC volumes and Volume Mounts
+	containers, pvcVolumes, err := kclient.GetPVCVolAndVolMount(containers, volumeNameToPVCName, containerNameToVolumes)


can we do the volume mount part while generating the containers?

Probably can, but that should be done with GetVolumes refactoring. I have not touched those volume logic in this PR.

Pls see the commend below regd Volume refactoring

adisky · 2020-10-30T08:53:02Z

pkg/devfile/adapters/kubernetes/utils/utils.go

+				}
+
+				// Get the container info for the given component
+				if container, ok := containersMap[component]; ok {


little confused here, what's the benefit of overriding here? we are not returning containers

So I've cleaned this up a little bit and I removed the map; this is basically the same comment as I have mentioned here #4134 (comment) - we pass in containers that we get from devfile to loop thru and append it to initcontainers.

adisky · 2020-10-30T08:55:17Z

pkg/kclient/generator/generators.go

+
+const (
+	// DevfileSourceVolume is the constant containing the name of the emptyDir volume containing the project source
+	DevfileSourceVolume = "devfile-projects"


do the other projects follows the concept of source volume?

I have asked this question on the forum-devfile Slack channel, lets wait for the outcome. I think all devfile consumers should sync the project somewhere in the pod.

I've moved the odo-projects vol mount code out of the generators and made it odo specific

adisky · 2020-10-30T08:56:27Z

pkg/kclient/generator/generators.go

+}
+
+// GenerateContainer creates a container spec that can be used when creating a pod
+func GenerateContainer(containerParams ContainerParams) *corev1.Container {


we do not need to expose this method

adisky · 2020-10-30T08:57:38Z

pkg/kclient/generator/generators.go

+
+// GeneratePVCSpec creates a pvc spec
+func GeneratePVCSpec(quantity resource.Quantity) *corev1.PersistentVolumeClaimSpec {
+


devfile should be input here and generate pvc specs according to volume component

I left this untouched due to the GetVolume refactoring I was talking about. CreatePVC just takes corev1.PersistentVolumeClaimSpec spec to create the pvc. The pvc name is tied with other logic and would need a separate PR to refactor it. I think thats going to be a bigger change.

adisky · 2020-10-30T08:58:47Z

pkg/kclient/generator/generators.go

+}
+
+// GenerateServiceSpec creates a service spec
+func GenerateServiceSpec(serviceSpecParams ServiceSpecParams) *corev1.ServiceSpec {


this should not be exposed

codecov · 2020-10-31T00:16:23Z

Codecov Report

Merging #4134 (60aaf7f) into master (30d9478) will increase coverage by 0.34%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #4134      +/-   ##
==========================================
+ Coverage   42.23%   42.57%   +0.34%     
==========================================
  Files         155      156       +1     
  Lines       13204    13258      +54     
==========================================
+ Hits         5577     5645      +68     
+ Misses       7020     7012       -8     
+ Partials      607      601       -6

Impacted Files	Coverage Δ
pkg/component/component_full_description.go	`0.00% <0.00%> (ø)`
pkg/devfile/adapters/common/types.go	`50.00% <ø> (ø)`
pkg/devfile/validate/generic/errors.go	`9.09% <0.00%> (-10.91%)`	⬇️
pkg/kclient/kclient.go	`0.00% <0.00%> (-8.46%)`	⬇️
pkg/kclient/supervisord.go	`0.00% <0.00%> (ø)`
pkg/testingutil/devfile.go	`0.00% <0.00%> (ø)`
pkg/kclient/secrets.go	`76.74% <73.33%> (-7.88%)`	⬇️
pkg/devfile/adapters/kubernetes/utils/utils.go	`81.02% <80.48%> (+0.35%)`	⬆️
...g/devfile/adapters/kubernetes/component/adapter.go	`37.54% <81.81%> (+6.42%)`	⬆️
pkg/kclient/generator/generators.go	`86.00% <86.00%> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30d9478...60aaf7f. Read the comment docs.

pkg/kclient/generator/generators.go

pkg/kclient/generator/generators_test.go

pkg/kclient/generator/utils.go

pkg/kclient/supervisord.go

Signed-off-by: Stephanie <[email protected]>

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

Signed-off-by: Stephanie <[email protected]>

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

adisky

/lgtm

openshift-bot · 2020-11-09T08:56:08Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/cleanup labels Oct 22, 2020

openshift-ci-robot requested review from dharmit and girishramnani October 22, 2020 20:16

maysunfaisal force-pushed the ref-kclient-gen-1 branch 3 times, most recently from c781bb5 to c152ca3 Compare October 27, 2020 15:13

maysunfaisal marked this pull request as ready for review October 27, 2020 22:43

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Oct 27, 2020

maysunfaisal changed the title ~~Refactor kclient generators code before migrating to devfile/parser~~ Refactor & cleanup kclient generators code before migrating to devfile/parser Oct 27, 2020

maysunfaisal requested review from adisky and mik-dass October 28, 2020 14:00

maysunfaisal force-pushed the ref-kclient-gen-1 branch 3 times, most recently from 0cb83f8 to 269efe1 Compare October 29, 2020 19:17

adisky reviewed Oct 30, 2020

View reviewed changes

yangcao77 mentioned this pull request Oct 30, 2020

initial Kubernetes object generation piece to unblock openshift/console devfile/api#184

Closed

maysunfaisal force-pushed the ref-kclient-gen-1 branch from bbc7ec6 to d3f2ffa Compare November 2, 2020 17:49

maysunfaisal requested a review from adisky November 2, 2020 20:57

maysunfaisal force-pushed the ref-kclient-gen-1 branch from 6bfc498 to 2908742 Compare November 3, 2020 17:50

yangcao77 mentioned this pull request Nov 3, 2020

Switch to use parser library #4188

Merged

4 tasks