-
Notifications
You must be signed in to change notification settings - Fork 192
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
Add API and Reconciler for Tekton Result #322
Conversation
/cc @nikhil-thomas |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/approve |
[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:
Approvers can indicate their approval by writing |
Looks great 👍 A few things needs to be added:
|
@wlynch could you take a look. 🧑💻 short recap:
cc @sm43 |
/hold |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
@nikhil-thomas addressed the comments. /hold cancel |
@sm43 let us keep TektonResults CRD and contrller available only for Kubernetes now. As there is no midstream project for TektonResult, lets avoid adding it to openshift builds. /hold make this patch kubernetes build specific |
/hold |
/hold cancel |
/retest |
1 similar comment
/retest |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
app: tekton-results-api | ||
app.kubernetes.io/name: tekton-results | ||
app.kubernetes.io/component: api | ||
results.tekton.dev/release: "v0.1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's a release version mismatch? (filepath says 0.2.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. i downloaded both but copied wrong one I guess 😅
// 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")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using context.Background/TODO, let's just have the func take in a context as a param.
test/resources/tektonresults.go
Outdated
} | ||
|
||
// TektonResultCRDelete deletes tha TektonResult to see if all resources will be deleted | ||
func TektonResultCRDelete(t *testing.T, clients *utils.Clients, crNames utils.ResourceNames) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: TektonResultCRDDelete?
@@ -0,0 +1,119 @@ | |||
// +build e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: tektonresultdeployment_test.go
- 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 | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
|
||
// AssertTektonResultCRReadyStatus verifies if the TektonResult reaches the READY status. | ||
func AssertTektonResultCRReadyStatus(t *testing.T, clients *utils.Clients, names utils.ResourceNames) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah make sense. It seems the for all components it is similar way. I will address this in a separate pr
The following is the coverage report on the affected files.
|
@wlynch the release.yaml for |
/retest |
tektoncd/results#115 to track. Beyond the recently opened issues, nothing else is jumping out at me, so LGTM. /lgtm |
@wlynch: changing LGTM is restricted to collaborators In response to this:
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>
The following is the coverage report on the affected files.
|
/lgtm |
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:
See the contribution guide for more details.
Release Notes