ADD reconciler for pipeline and task resources - step 6 on operator-pipelines docs #13

acmenezes · 2021-11-02T18:36:05Z

In a nutshell this PR solves the bare minimum on issue #5.

Added on new the dependencies.go file in package controllers:

Clones the repo operator-pipelines to the tmp directory in the container.
Reads and applies both pipelines and tasks found on the paths described in the instructions;
Removes the repo clone from the tmp directory in the container.

OBS:
On Step 2:

The apply part is done via dynamic client-go with unstructured objects "rest mapped" to known GVKs such as pipelines and tasks that belong to tekton.dev apigroup; That requires that the openshift-pipeline operator aka tekton be installed before. This task doesn't seem to require any other to complete therefore I put it right after the pipeline operator subscription one on the resources.go file.

I'm assuming those will be deployed on the same namespace as the CR here.

The image I used for testing this on my AWS OCP environment is on my own repo for now as I don't know yet what tagging strategy for testing images I should use on opdev. You can find it here: quay.io/acmenezes/operator-certification-operator:0.0.1-2.

We may need unit tests, comments and more logging on it (maybe on the next iteration). Glad to discuss what is the level of preference on each one of those that this team prefers.

This commit implements decoding of yaml manifests into unstructured objs for client to apply. It doesn't use the controller-runtime client instead uses a client-go based client. For now it uses a local test.yaml as input to be replaced with operator-pipelines yaml manifests.

The applyManifests function is used within the reconcilePipelineDependencies to create both pipelines and tasks from the operator-pipeline project after executing a plain clone from the project.

This commit includes: 1 - few adjustments and constants on func reconcilePipelineDependencies 2 - kubebuilder marker with tekton.dev api group rbac permissions 3 - other changes due to make manifests commands such as role.yaml with tekton permissions.

Tekton pipelines and tasks are being created on the same namespace as the namespace where the CR is created. Just by catching the meta.Namespace on the specific func for reconciling the pipeline dependencies.

acornett21 · 2021-11-02T19:59:53Z

controllers/dependencies.go

+	// yaml manifests that need to be applied beforehand
+	// ref: https://github.com/redhat-openshift-ecosystem/certification-releases/blob/main/4.9/ga/ci-pipeline.md#step-6---install-the-certification-pipeline-and-dependencies-into-the-cluster
+
+	repo := "https://github.com/redhat-openshift-ecosystem/operator-pipelines.git"


Could this be a constant as well?

Fair enough. My bad. I think I passed too fast my eyes on that. I just changed it.

controllers/dependencies.go

samira-barouti · 2021-11-03T01:06:57Z

controllers/dependencies.go

+	}
+
+	// Removing cloned project
+	if err = os.RemoveAll(PIPELINE_MANIFESTS_PATH); err != nil {


I think removing the manifests should be a deferred function after git.PlainClone. Otherwise, if any error encounters, the program execution will never reach here.
My other question is why we don't remove the files under TASK_MANIFEST_PATH?

That's fair. My mistake. It's not PIPELINE_MANIFESTS_PATH it's the whole REPO_CLONE_PATH. And the defer is a good suggestion already done.

samira-barouti · 2021-11-03T01:07:28Z

controllers/dependencies.go

+
+func (r *OperatorPipelineReconciler) applyManifests(fileName string, Namespace string) error {
+
+	b, err := ioutil.ReadFile(fileName)


ioutil is going to be deprecated soon (I think). It would be better to use os package instead.

Not sure about ioutil. But no problem os is now the one on the code. They have exactly the same signature.

samira-barouti · 2021-11-03T01:09:24Z

controllers/dependencies.go

+	}
+
+	// Decoding yaml files to resource objects and apply to cluster
+	decoder := yamlutil.NewYAMLOrJSONDecoder(bytes.NewReader(b), 100)


I was having some fun today to resolve the same issue, and I think using yaml.Unmarshal is cleaner. This is the snippet of my code to read a csv file:

yfile, err := os.ReadFile("csv.yaml") if err != nil { log.Error(err) } var data operatorv1alpha1.ClusterServiceVersion err = yaml.Unmarshal(yfile, &data)

I checked the pipeline and task folders and we need to handle two CRDs (Task and Pipeline). I think this could be simplified. Please take a look at how we manage a similar situation in preflight (but it is the opposite! we try to dump go struct into yaml manifest). https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/1a2ef8a3829ea911356cd41a0171130d568379cd/certification/internal/policy/operator/deployable_by_olm.go#L378

Please let me know if we need to discuss further.

As it is it's working. Are we aiming for refactoring here? Wouldn't it be interesting having another iteration with another issue to simplify it? I would suggest that we unblock other tasks that depends on this step first and take the code refactoring and new approaches on another issue. Glad to jump in and make the changes there. What do you think?

Yes refactoring.. I understand this is working, but I think it is too complex to follow and some of these lines could be removed. I'd personally like to push a code that is easy to maintain the first time. The refactoring is usually more complicated as you need to have/add unit tests to ensure the refactored code is not breaking, and we don't have unit tests yet.

BTW, have you seen this?
kubernetes/client-go#193 (comment)

I agree with @samira-barouti here, I'd rather see this clearly defined structs (or partial structs) and use yaml.Unmarshal to simplify the code and make it easier to read/maintain.

Hi Samira, nope. I didn't see this one. But just realized that going the generic approach with client-go was not necessary there. So I went on the specific approach which is actually way easier. I just flipped to controller runtime client and that's much better now I think. Please check it out.

bcrochet

Two things...

Unit tests?
Please collapse all the commits. Unless a commit can stand on its own, it shouldn't be a separate commit. I see some of the individual commits have a bunch of commented out code. That definitely doesn't need to be in the repo as a part of the history.

bcrochet · 2021-11-03T14:40:21Z

controllers/dependencies.go

+
+func (r *OperatorPipelineReconciler) applyManifests(fileName string, Namespace string) error {
+
+	b, err := ioutil.ReadFile(fileName)


bcrochet

A couple more comments.

bcrochet · 2021-11-03T14:53:47Z

controllers/dependencies.go

+	defer r.RemovePipelineDependencyFiles(REPO_CLONE_PATH)
+
+	// Reading pipeline manifests and applying to cluster
+	root := REPO_CLONE_PATH + PIPELINE_MANIFESTS_PATH


These two blocks could be a loop.

Can you clarify? What do you mean?

paths := []string{PIPELINE_MANIFESTS_PATH, TASKS_MANIFESTS_PATH}
for _, path := range paths {
...
}

The blocks are identical. The only difference is the two paths.

bcrochet · 2021-11-03T14:54:24Z

controllers/dependencies.go

+	return nil
+}
+
+func (r *OperatorPipelineReconciler) RemovePipelineDependencyFiles(filePath string) error {


This doesn't seem like it needs to be exported.

removePipelineDependencyFiles

acmenezes · 2021-11-03T15:05:33Z

Two things...

Unit tests?

Please collapse all the commits. Unless a commit can stand on its own, it shouldn't be a separate commit. I see some of the individual commits have a bunch of commented out code. That definitely doesn't need to be in the repo as a part of the history.

@bcrochet

about the unit tests I also wrote on the PR note. We didn't discuss on our first meeting how it would be done and with what libraries on this project. I suggest we take it on another issue and have it defined with everyone's participation.
About the multiple commits what do you suggest here? Do you mean git rebase -i head n? Or do you apply another method to accomplish this? Sorry about my ignorance here.

bcrochet · 2021-11-03T15:12:19Z

Two things...

Unit tests?

Please collapse all the commits. Unless a commit can stand on its own, it shouldn't be a separate commit. I see some of the individual commits have a bunch of commented out code. That definitely doesn't need to be in the repo as a part of the history.

@bcrochet
1. about the unit tests I also wrote on the PR note. We didn't discuss on our first meeting how it would be done and with what libraries on this project. I suggest we take it on another issue and have it defined with everyone's participation.

We've used ginkgo/gomega on preflight. But we've also accepted plain old Go tests. Use whatever you're comfortable with. It's a bigger debt to have to add a test later, than to possibly migrate a test to a different framework.

2. About the multiple commits what do you suggest here? Do you mean git rebase -i head n? Or do you apply another method to accomplish this? Sorry about my ignorance here.

My typical workflow when working on a PR like this is to git commit --amend whenever I'm fixing up a PR based on feedback. But yes, a git rebase -i HEAD~n and squash them all to a single commit. It's not completely forbidden to have multiple commits in a PR, but unless a commit adds a significant contribution to the git history, it doesn't need to be a part of main, i.e. we don't need to see your development iterations in the final product. :)

1 - fileWalk on loop block 2 - Simplify code on manifest apply

acmenezes · 2021-11-05T01:13:48Z

Two things...

Unit tests?

Please collapse all the commits. Unless a commit can stand on its own, it shouldn't be a separate commit. I see some of the individual commits have a bunch of commented out code. That definitely doesn't need to be in the repo as a part of the history.

@bcrochet
1. about the unit tests I also wrote on the PR note. We didn't discuss on our first meeting how it would be done and with what libraries on this project. I suggest we take it on another issue and have it defined with everyone's participation.
We've used ginkgo/gomega on preflight. But we've also accepted plain old Go tests. Use whatever you're comfortable with. It's a bigger debt to have to add a test later, than to possibly migrate a test to a different framework.
2. About the multiple commits what do you suggest here? Do you mean git rebase -i head n? Or do you apply another method to accomplish this? Sorry about my ignorance here.
My typical workflow when working on a PR like this is to git commit --amend whenever I'm fixing up a PR based on feedback. But yes, a git rebase -i HEAD~n and squash them all to a single commit. It's not completely forbidden to have multiple commits in a PR, but unless a commit adds a significant contribution to the git history, it doesn't need to be a part of main, i.e. we don't need to see your development iterations in the final product. :)

Hi @bcrochet , so on the unit test I believe after we talked yesterday you won't need them anymore now. About the commits agreed. I pass to that practice from now on. I would suggest to squash them on merge when this PR gets approved.

samira-barouti · 2021-11-08T16:10:29Z

controllers/dependencies.go

+			if !info.IsDir() {
+				var pipeline tekton.Pipeline
+				var task tekton.Task
+				fmt.Print(path)


this seems to be a leftover from the debugging. Could be removed.

That's also done.

controllers/dependencies.go

samira-barouti · 2021-11-08T16:15:47Z

controllers/dependencies.go

+		// Progress: os.Stdout,
+	})
+	if err != nil {
+		log.Log.Info("Couldn't clone the repository for operator-pipelines: " + err.Error())


This should be log.Log.Error

samira-barouti · 2021-11-08T16:16:04Z

controllers/dependencies.go

+			return nil
+		})
+		if err != nil {
+			log.Log.Info("Couldn't iterate over operator-pipelines yaml manifest files: " + err.Error())


samira-barouti · 2021-11-08T16:16:11Z

controllers/dependencies.go

+
+func (r *OperatorPipelineReconciler) removePipelineDependencyFiles(filePath string) error {
+	if err := os.RemoveAll(filePath); err != nil {
+		log.Log.Info("Couldn't remove operator-pipelines directory")


samira-barouti · 2021-11-08T16:16:25Z

controllers/dependencies.go

+
+	b, err := os.ReadFile(fileName)
+	if err != nil {
+		log.Log.Info("Couldn't read manifest file " + err.Error())


controllers/dependencies.go

acornett21 · 2021-11-02T20:04:01Z

controllers/dependencies.go

+		mapper := restmapper.NewDiscoveryRESTMapper(gr)
+		mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+		if err != nil {
+			log.Log.Info("Couldn't the preferred resource mapping for given kind.")


Is this log missing a word?

acornett21 · 2021-11-03T17:40:21Z

controllers/dependencies.go

+	}
+
+	// Decoding yaml files to resource objects and apply to cluster
+	decoder := yamlutil.NewYAMLOrJSONDecoder(bytes.NewReader(b), 100)


I agree with @samira-barouti here, I'd rather see this clearly defined structs (or partial structs) and use yaml.Unmarshal to simplify the code and make it easier to read/maintain.

controllers/dependencies.go

samira-barouti · 2021-11-08T17:11:58Z

controllers/dependencies.go

+		log.Error(err, "Couldn't clone the repository for operator-pipelines")
+		return err
+	}
+	defer r.removePipelineDependencyFiles(REPO_CLONE_PATH)


I think (and I might be wrong), that this method call should be place before the if statement on line 33. If cloning the repo fails in the middle, the control flow will return the error but not delete the created dir.

I overaly recommend 1) creating a tmp folder programmatically for the operator-pipelines (please check https://pkg.go.dev/os#MkdirTemp) to avoid unwanted surprises, and 2) move the defer method to line 33, after the git.PlainClone and before the if err

2- I'm not sure as well if the defer function would get triggered there so just in case I moved it to line 33 as requested.

1- On the other hand temporary files used by programs in Linux are historically stored in the /tmp directory that happens to have, in regular systems, garbage collection in place. This is a single Linux image deployment, single process container, single user with a single purpose. It's pretty straightforward. I don't see the need to add another package/method and more complexity to address this.

No problem.

samira-barouti

/lgtm

acmenezes added 6 commits November 1, 2021 14:50

Clone repository, iterate over manifests and applyManifests added

4618041

The applyManifests function is used within the reconcilePipelineDependencies to create both pipelines and tasks from the operator-pipeline project after executing a plain clone from the project.

ADD removeAll from repository clone after using it

9e0b264

fixes make deploy changes on test

b862911

ADD CR namespace on reconcilePipelineDependencies

ecb656d

Tekton pipelines and tasks are being created on the same namespace as the namespace where the CR is created. Just by catching the meta.Namespace on the specific func for reconciling the pipeline dependencies.

openshift-ci bot requested review from bcrochet and mrhillsman November 2, 2021 18:36

acmenezes mentioned this pull request Nov 2, 2021

Deploy the operator certification pipeline resources #5

Closed

acornett21 reviewed Nov 2, 2021

View reviewed changes

CHANGE repo var to OPERATOR_PIPELINES_REPO const

f0a2da3

samira-barouti requested changes Nov 3, 2021

View reviewed changes

samira-barouti mentioned this pull request Nov 3, 2021

Centralized logging #15

Closed

acmenezes added 2 commits November 3, 2021 10:38

DEFER remove files from operator-pipelines and fix path

44927e8

REPLACING ioutil.ReadFile by os.ReadFile; both have the same signature

e7074be

bcrochet requested changes Nov 3, 2021

View reviewed changes

CHANGE exported function to non exported

2656853

CHANGES requested on PR

4ea6ada

1 - fileWalk on loop block 2 - Simplify code on manifest apply

acmenezes changed the title ~~Resources 5~~ ADD reconciler for pipeline and task resources - step 6 on operator-pipelines docs Nov 8, 2021

samira-barouti requested changes Nov 8, 2021

View reviewed changes

acmenezes added 3 commits November 8, 2021 11:41

Merge branch 'redhat-openshift-ecosystem:main' into resources-5

b575491

REPLACE log.Log by just log on dependencies.go

e755294

ADD log.Error on all return err statements as possible

eb9d742

acornett21 approved these changes Nov 8, 2021

View reviewed changes

samira-barouti reviewed Nov 8, 2021

View reviewed changes

MOVE defer method to line 33

545558c

samira-barouti approved these changes Nov 8, 2021

View reviewed changes

openshift-ci bot assigned samira-barouti Nov 8, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2021

jomkz merged commit 0a911ee into redhat-openshift-ecosystem:main Nov 9, 2021

acmenezes deleted the resources-5 branch November 30, 2021 19:14


		func (r *OperatorPipelineReconciler) applyManifests(fileName string, Namespace string) error {

		b, err := ioutil.ReadFile(fileName)

ADD reconciler for pipeline and task resources - step 6 on operator-pipelines docs #13

ADD reconciler for pipeline and task resources - step 6 on operator-pipelines docs #13

Conversation

acmenezes commented Nov 2, 2021

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

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

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

bcrochet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcrochet 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmenezes commented Nov 3, 2021

bcrochet commented Nov 3, 2021

acmenezes commented Nov 5, 2021 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmenezes Nov 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samira-barouti left a comment

Choose a reason for hiding this comment

acmenezes commented Nov 5, 2021 •

edited

acmenezes Nov 8, 2021 •

edited