Add API and Reconciler for Tekton Result #322

sm43 · 2021-06-08T08:05:01Z

Closes #163

Signed-off-by: Shivam Mukhade smukhade@redhat.com

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

User can now install Tekton Results by creating a new CR TektonResult. The installation process is documented at  https://github.com/tektoncd/operator/blob/main/docs/tekton_results.md. This is enabled only for Kubernetes Platform.

sm43 · 2021-06-08T08:06:25Z

/cc @nikhil-thomas

tekton-robot · 2021-06-08T08:07:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonresult_lifecycle.go	Do not exist	76.5%
pkg/apis/operator/v1alpha1/tektonresult_types.go	Do not exist	0.0%
pkg/reconciler/common/install.go	65.5%	62.5%	-3.0
pkg/reconciler/common/releases.go	64.6%	63.6%	-1.0
pkg/reconciler/common/transformers.go	64.5%	53.4%	-11.1

config/openshift/role.yaml

tekton-robot · 2021-06-09T04:03:54Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonresult_lifecycle.go	Do not exist	76.5%
pkg/apis/operator/v1alpha1/tektonresult_types.go	Do not exist	0.0%
pkg/reconciler/common/install.go	65.5%	62.5%	-3.0
pkg/reconciler/common/releases.go	64.6%	63.6%	-1.0
pkg/reconciler/common/transformers.go	64.5%	53.4%	-11.1

nikhil-thomas · 2021-06-09T06:41:10Z

/approve

tekton-robot · 2021-06-09T06:41:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhil-thomas

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~~ [nikhil-thomas]

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

nikhil-thomas · 2021-06-09T06:43:18Z

Looks great 👍

A few things needs to be added:

release not in pr description
a doc in docs/ mentioning that TektonResults will not be auton installed like other components, as the TektonResults needs 2 secrets which end user should provide.

nikhil-thomas · 2021-06-09T06:50:25Z

@wlynch could you take a look. 🧑‍💻

short recap:

this pr adds a new clusterScoped CRD TektonResults
user will have to create an instance of this CRD with name result and operator will install latest available (present in operator repo) release.yaml of TektonReults
- operator reconciler will only 1 instance (name: result) of the clusterwide CRD
reconciler logic in short: read TektonResults reelase.yaml, change namespace to spec.targetNamespace, create resources on cluster
at present, unlike other tekton components operator will not auto-install TektonResults, as TektonResults need 2 secrets to be created before install.

cc @sm43

sm43 · 2021-06-09T07:01:14Z

/hold
to add unit tests for transformers

tekton-robot · 2021-06-09T11:31:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonresult_lifecycle.go	Do not exist	76.5%
pkg/apis/operator/v1alpha1/tektonresult_types.go	Do not exist	0.0%
pkg/reconciler/common/install.go	65.5%	62.5%	-3.0
pkg/reconciler/common/releases.go	64.6%	63.6%	-1.0
pkg/reconciler/common/transformers.go	64.5%	67.2%	2.7

tekton-robot · 2021-06-09T11:37:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonresult_lifecycle.go	Do not exist	76.5%
pkg/apis/operator/v1alpha1/tektonresult_types.go	Do not exist	0.0%
pkg/reconciler/common/install.go	65.5%	62.5%	-3.0
pkg/reconciler/common/releases.go	64.6%	63.6%	-1.0
pkg/reconciler/common/transformers.go	64.5%	67.2%	2.7

tekton-robot · 2021-06-09T11:51:38Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonresult_lifecycle.go	Do not exist	76.5%
pkg/apis/operator/v1alpha1/tektonresult_types.go	Do not exist	0.0%
pkg/reconciler/common/install.go	65.5%	62.5%	-3.0
pkg/reconciler/common/releases.go	64.6%	63.6%	-1.0
pkg/reconciler/common/transformers.go	64.5%	67.6%	3.2

sm43 · 2021-06-09T12:04:04Z

Looks great +1

A few things needs to be added:

* release not in pr description

* a doc in `docs/` mentioning that TektonResults `will not be` auton installed like other components, as the TektonResults needs 2 secrets which end user should provide.

@nikhil-thomas addressed the comments.
also added a condition to check if Target Namespace in TektonResult CR is same as TektonPipeline CR, if different then installation would fail.

/hold cancel

nikhil-thomas · 2021-06-10T13:50:47Z

@sm43 let us keep TektonResults CRD and contrller available only for Kubernetes now.
For OpenShift we use payload (release.yaml) created in `github.com/openshift/tektoncd-. eg: (https://github.com/openshift/tektoncd-pipeline/blob/release-v0.22.0/openshift/release/tektoncd-pipeline-v0.22.0.yaml)

As there is no midstream project for TektonResult, lets avoid adding it to openshift builds.

/hold make this patch kubernetes build specific

nikhil-thomas · 2021-06-10T13:51:39Z

/hold

savitaashture · 2021-06-10T16:40:19Z

/hold cancel

sm43 · 2021-06-10T16:45:47Z

/retest

sm43 · 2021-06-14T03:25:18Z

/retest

tekton-robot · 2021-06-16T03:44:26Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonresult_lifecycle.go	Do not exist	76.5%
pkg/apis/operator/v1alpha1/tektonresult_types.go	Do not exist	0.0%
pkg/reconciler/common/install.go	65.5%	62.5%	-3.0
pkg/reconciler/common/releases.go	64.6%	63.6%	-1.0
pkg/reconciler/common/transformers.go	64.5%	67.6%	3.2

tekton-robot · 2021-06-16T04:28:27Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonresult_lifecycle.go	Do not exist	76.5%
pkg/apis/operator/v1alpha1/tektonresult_types.go	Do not exist	0.0%
pkg/reconciler/common/install.go	65.5%	62.5%	-3.0
pkg/reconciler/common/releases.go	64.6%	63.6%	-1.0
pkg/reconciler/common/transformers.go	64.5%	67.6%	3.2

wlynch · 2021-06-17T14:41:55Z

cmd/kubernetes/operator/kodata/tekton-result/0.2.0/release.yaml

+        app: tekton-results-api
+        app.kubernetes.io/name: tekton-results
+        app.kubernetes.io/component: api
+        results.tekton.dev/release: "v0.1.1"


Looks like there's a release version mismatch? (filepath says 0.2.0).

yep. i downloaded both but copied wrong one I guess 😅

wlynch · 2021-06-17T14:47:39Z

test/resources/tektonresults.go

+// is done, returns an error or timeout.
+func WaitForTektonResultState(clients resultv1alpha1.TektonResultInterface, name string,
+	inState func(s *v1alpha1.TektonResult, err error) (bool, error)) (*v1alpha1.TektonResult, error) {
+	span := logging.GetEmitableSpan(context.Background(), fmt.Sprintf("WaitForTektonResultState/%s/%s", name, "TektonResultIsReady"))


Instead of using context.Background/TODO, let's just have the func take in a context as a param.

wlynch · 2021-06-17T14:48:51Z

test/resources/tektonresults.go

+}
+
+// TektonResultCRDelete deletes tha TektonResult to see if all resources will be deleted
+func TektonResultCRDelete(t *testing.T, clients *utils.Clients, crNames utils.ResourceNames) {


nit: TektonResultCRDDelete?

wlynch · 2021-06-17T15:18:46Z

test/e2e/common/tektonresultdeployement_test.go

@@ -0,0 +1,119 @@
+// +build e2e


nit: tektonresultdeployment_test.go

wlynch · 2021-06-17T15:22:56Z

docs/tekton_results.md

+    - namespace: `tekton-pipelin  es`
+    - name: `tekton-results-mysql`
+    - contains the fields:
+        - `user=root`
+        - `password=<your password>`
+
+  If you are not using a particular password management strategy, the following
+  command will generate a random password for you:
+  Update namespace value in the command if Tekton Pipelines is installed in a different namespace..
+
+   ```sh
+   $ kubectl create secret generic tekton-results-mysql --namespace=tekton-pipelines --from-literal=user=root --from-literal=password=$(openssl rand -base64 20)
+   ```
+- Generate cert/key pair. 
+  Note: Feel free to use any cert management software to do this!
+
+  Tekton Results expects the cert/key pair to be stored in a [TLS Kubernetes Secret](https://kubernetes.io/docs/concepts/configuration/secret/#tls-secrets).
+  Update the namespace value in below export command if Tekton Pipelines is installed in a different namespace.
+   ```sh
+   # Generate new self-signed cert.
+   $ export NAMESPACE="tekton-pipelines"
+   $ openssl req -x509 \
+   -newkey rsa:4096 \
+   -keyout key.pem \
+   -out cert.pem \
+   -days 365 \
+   -nodes \
+   -subj "/CN=tekton-results-api-service.${NAMESPACE}.svc.cluster.local" \
+   -addext "subjectAltName = DNS:tekton-results-api-service.${NAMESPACE}.svc.cluster.local"
+   # Create new TLS Secret from cert.
+   $ kubectl create secret tls -n ${NAMESPACE} tekton-results-tls \
+   --cert=cert.pem \
+   --key=key.pem
+   ```


I'm not super familiar with operator best practices, but would it be possible to automate these steps for the user if we find that they are not present at reconcile time?

For the TLS cert, are there existing examples for how we can hook into cert-manager or something similar?

yeah that's the plan to automate this also, To start with we kept the creation of secrets a manual steps. so, user will create secrets and then TektonResult CR.
cc @nikhil-thomas

would it be possible to automate these steps for the user if we find that they are not present at reconcile time?

that is definitely the ideal ux the operator should deliver.
we will definitely handle in in upcoming iterations. 🧑‍💻 👍

i have created an issue to track this: Automate creation prerequisites for TektonResult install #331

wlynch · 2021-06-17T15:25:47Z

test/resources/tektonresults.go

+}
+
+// AssertTektonResultCRReadyStatus verifies if the TektonResult reaches the READY status.
+func AssertTektonResultCRReadyStatus(t *testing.T, clients *utils.Clients, names utils.ResourceNames) {


it's a bit odd to me to have an exported func in a library that requires a testing.T. Can we unexport these and move them to the test file where they are used?

oh yeah make sense. It seems the for all components it is similar way. I will address this in a separate pr

tekton-robot · 2021-06-17T16:48:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonresult_lifecycle.go	Do not exist	76.5%
pkg/apis/operator/v1alpha1/tektonresult_types.go	Do not exist	0.0%
pkg/reconciler/common/install.go	65.5%	62.5%	-3.0
pkg/reconciler/common/releases.go	64.6%	63.6%	-1.0
pkg/reconciler/common/transformers.go	64.5%	67.6%	3.2

sm43 · 2021-06-17T16:52:53Z

@wlynch the release.yaml for v0.2.0 of Results seems to have results.tekton.dev/release: devel and version seems to be missing. Could you please check once?
https://github.com/tektoncd/results/releases/tag/v0.2.0

sm43 · 2021-06-18T07:50:35Z

/retest

wlynch · 2021-06-30T18:37:19Z

@wlynch the release.yaml for v0.2.0 of Results seems to have results.tekton.dev/release: devel and version seems to be missing. Could you please check once?
https://github.com/tektoncd/results/releases/tag/v0.2.0

tektoncd/results#115 to track.

Beyond the recently opened issues, nothing else is jumping out at me, so LGTM.

/lgtm

tekton-robot · 2021-06-30T18:37:26Z

@wlynch: changing LGTM is restricted to collaborators

In response to this:

@wlynch the release.yaml for v0.2.0 of Results seems to have results.tekton.dev/release: devel and version seems to be missing. Could you please check once?
https://github.com/tektoncd/results/releases/tag/v0.2.0

tektoncd/results#115 to track.

Beyond the recently opened issues, nothing else is jumping out at me, so LGTM.

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This adds a new CRD TektonResult to install and manage Tekton Results component only for Kubernetes Platform. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

tekton-robot · 2021-07-05T04:21:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonresult_lifecycle.go	Do not exist	76.5%
pkg/apis/operator/v1alpha1/tektonresult_types.go	Do not exist	0.0%
pkg/reconciler/common/install.go	65.5%	62.5%	-3.0
pkg/reconciler/common/releases.go	64.6%	63.6%	-1.0
pkg/reconciler/common/transformers.go	64.5%	67.6%	3.2

vdemeester · 2021-07-05T09:13:41Z

/lgtm

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 8, 2021

tekton-robot requested review from piyush-garg and pradeepitm12 June 8, 2021 08:05

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 8, 2021

tekton-robot requested a review from nikhil-thomas June 8, 2021 08:06

savitaashture reviewed Jun 8, 2021

View reviewed changes

config/openshift/role.yaml Outdated Show resolved Hide resolved

sm43 force-pushed the add-tekton-results branch from c386b34 to 2998a5d Compare June 9, 2021 04:01

nikhil-thomas added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Jun 9, 2021

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2021

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2021

sm43 force-pushed the add-tekton-results branch from 2998a5d to 2c39775 Compare June 9, 2021 11:29

sm43 force-pushed the add-tekton-results branch from 2c39775 to 942554e Compare June 9, 2021 11:35

sm43 force-pushed the add-tekton-results branch from 942554e to d27fdc9 Compare June 9, 2021 11:49

tekton-robot removed the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Jun 9, 2021

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2021

sm43 requested a review from savitaashture June 9, 2021 12:04

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2021

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2021

sm43 force-pushed the add-tekton-results branch from 373e858 to dcd52f6 Compare June 16, 2021 03:42

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2021

sm43 force-pushed the add-tekton-results branch from dcd52f6 to cbb2eb1 Compare June 16, 2021 04:26

wlynch reviewed Jun 17, 2021

View reviewed changes

sm43 force-pushed the add-tekton-results branch from cbb2eb1 to bd1955d Compare June 17, 2021 16:46

nikhil-thomas mentioned this pull request Jun 18, 2021

Automate creation prerequisites for TektonResult install #331

Open

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2021

sm43 force-pushed the add-tekton-results branch from bd1955d to 5fb211b Compare July 5, 2021 04:12

SM43 added 2 commits July 5, 2021 09:49

Adds API & Reconciler for TektonResult for K8s Platform

1590695

This adds a new CRD TektonResult to install and manage Tekton Results component only for Kubernetes Platform. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

Adds doc explaining steps to enable Tekton Results

087d070

Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

sm43 force-pushed the add-tekton-results branch from 5fb211b to 087d070 Compare July 5, 2021 04:19

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2021

tekton-robot assigned vdemeester Jul 5, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2021

tekton-robot merged commit 63c988b into tektoncd:main Jul 5, 2021

sm43 deleted the add-tekton-results branch July 7, 2021 09:45

Add API and Reconciler for Tekton Result #322

Add API and Reconciler for Tekton Result #322

Conversation

sm43 commented Jun 8, 2021 • edited Loading

Submitter Checklist

Release Notes

sm43 commented Jun 8, 2021

tekton-robot commented Jun 8, 2021

tekton-robot commented Jun 9, 2021

nikhil-thomas commented Jun 9, 2021

tekton-robot commented Jun 9, 2021

nikhil-thomas commented Jun 9, 2021

nikhil-thomas commented Jun 9, 2021 • edited Loading

sm43 commented Jun 9, 2021

tekton-robot commented Jun 9, 2021

tekton-robot commented Jun 9, 2021

tekton-robot commented Jun 9, 2021

sm43 commented Jun 9, 2021

nikhil-thomas commented Jun 10, 2021

nikhil-thomas commented Jun 10, 2021

savitaashture commented Jun 10, 2021

sm43 commented Jun 10, 2021

sm43 commented Jun 14, 2021

tekton-robot commented Jun 16, 2021

tekton-robot commented Jun 16, 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

nikhil-thomas Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jun 17, 2021

sm43 commented Jun 17, 2021

sm43 commented Jun 18, 2021

wlynch commented Jun 30, 2021

tekton-robot commented Jun 30, 2021

tekton-robot commented Jul 5, 2021

vdemeester commented Jul 5, 2021

sm43 commented Jun 8, 2021 •

edited

Loading

nikhil-thomas commented Jun 9, 2021 •

edited

Loading

nikhil-thomas Jun 18, 2021 •

edited

Loading