Add Results configuration in tekton config #2398

pratap0007 · 2024-11-07T23:37:28Z

This will add the Results configuration to the tekton config and will be
installed by default through tekton config
Docs changes related to this PR is Add TektonResult to TektonConfig doc #2400

Changes

Submitter Checklist

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

Run make test lint before submitting a PR
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

This will add the Results configuration to the tekton config and will be installed by default through tekton config

tekton-robot · 2024-11-07T23:37:31Z

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

khrm

Can you create documentation for this PR?

jkandasa · 2024-11-19T02:13:57Z

@pratap0007 include the doc changes[1] on this PR
[1] - #2400

tekton-robot · 2024-11-25T08:05:01Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	46.8%	-5.2
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

tekton-robot · 2024-11-25T09:26:15Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	46.8%	-5.2
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

tekton-robot · 2024-11-25T09:56:15Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	46.8%	-5.2
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

tekton-robot · 2024-11-25T14:06:18Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	46.8%	-5.2
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

pratap0007 · 2024-11-26T04:48:58Z

/test pull-tekton-operator-integration-tests
/test pull-tekton-operator-build-tests

tekton-robot · 2024-11-26T05:51:37Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	46.8%	-5.2
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

khrm

@pratap0007 Please address the review comments, It looks fine.

tekton-robot · 2025-01-15T13:33:28Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	50.0%	-2.0
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

pratap0007 · 2025-01-15T13:57:57Z

@khrm @l-qing Thanks for review and I have addressed it

l-qing

Your modification might still need a pull request to add end-to-end tests. 😆

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

l-qing · 2025-01-16T08:13:47Z

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

+
+	_, err := r.kubeClientSet.CoreV1().Secrets(tr.Spec.TargetNamespace).Create(ctx, secret, metav1.CreateOptions{})
+	if err != nil {
+		logger.Error(err)


Same as above.

l-qing · 2025-01-16T08:17:23Z

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

+				tr.Status.MarkDependencyMissing(fmt.Sprintf("Default db %s creation is failing", DbSecretName))
+				return err
+			}
+		}


The current writing places a significant mental burden.
It is necessary to consider whether the final return nil is accurate.

Would this be better?

if err == nil { return nil } if !apierrors.IsNotFound(err) { // logger return err } newDBSecret := r.getDBSecret(DbSecretName, tr.Spec.TargetNamespace, tr) ...

pkg/reconciler/shared/tektonconfig/result/result.go

tekton-robot · 2025-01-16T10:38:34Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	50.0%	-2.0
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

l-qing

👍

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

tekton-robot · 2025-01-17T16:17:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: l-qing
To complete the pull request process, please assign concaf after the PR has been reviewed.
You can assign the PR to them by writing /assign @concaf in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

tekton-robot · 2025-01-20T05:48:46Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	50.0%	-2.0
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

tekton-robot · 2025-01-22T13:05:23Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	50.0%	-2.0
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

tekton-robot · 2025-01-23T07:57: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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	50.0%	-2.0
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

pratap0007 · 2025-01-23T11:07:41Z

/test pull-tekton-operator-integration-tests

l-qing · 2025-01-23T15:11:15Z

/retest

l-qing · 2025-01-24T02:29:10Z

/retest

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

jkhelil · 2025-01-28T08:44:32Z

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

+	}
+
+	notBefore := time.Now()
+	notAfter := notBefore.Add(365 * 24 * time.Hour)


We should document the fact that we dont rotate the tls certificate if generated via the operator

jkhelil · 2025-01-28T08:56:28Z

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

-	// TODO: And remove this check in future release.
-	if err := r.validateSecretsAreCreated(ctx, tr); err != nil {
-		return err
+	// If external database is disable then create default database and tls secret


If the external database is disabled, create a default database and a TLS secret.
Otherwise, verify if the default database secret is already created, and ensure the TLS secret is also created.

Are we sure that the code below creates a database ? it seems jsut creating creds

Are we sure that the code below creates a database ? it seems jsut creating creds

yes, it creates the database

tekton-robot · 2025-01-28T14:41:20Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	50.0%	-2.0
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

jkhelil · 2025-02-04T07:28:52Z

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

+		Type:       corev1.SecretTypeOpaque,
+		StringData: map[string]string{},
+	}
+	password, _ := generateRandomBaseString(20)


We should handle the error of generateRandomBaseString

jkhelil · 2025-02-04T07:30:05Z

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

+	// Create Kubernetes TLS secret
+	err = r.createKubernetesTLSSecret(ctx, tr.Spec.TargetNamespace, TlsSecretName, certPEM, keyPEM, tr)
+	if err != nil {
+		logger.Fatalf("failed to create TLS secret %s in namespace %s: %v", TlsSecretName, tr.Spec.TargetNamespace, err)


use Errof instead of Fatal

jkhelil · 2025-02-04T07:34:11Z

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

+	if err != nil {
+		return nil, nil, err
+	}
+	keyPEM = pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: privBytes})


Add constante for RSAPrivateKeyBlockType = "RSA PRIVATE KEY"

You are using RSA type with x509.MarshalECPrivateKey(priv) ? Should be type EC Private KEY , right ?

yes, it should be EC Private KEY

jkhelil · 2025-02-04T07:34:31Z

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

+		return nil, nil, err
+	}
+
+	certPEM = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER})


use constante CertificateBlockType = "CERTIFICATE"

jkhelil · 2025-02-04T07:40:03Z

pkg/reconciler/kubernetes/tektonresult/tektonresult.go

+	}
+	password, _ := generateRandomBaseString(20)
+	s.StringData["POSTGRES_PASSWORD"] = password
+	s.StringData["POSTGRES_USER"] = "result"


Use constant

This will add the Results configuration to the tekton config and will be installed by default through tekton config Signed-off-by: Shiv Verma <[email protected]>

pratap0007 · 2025-02-06T10:46:55Z

/retest

tekton-robot · 2025-02-06T10:52:04Z

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/tektonconfig_validation.go	45.7%	46.2%	0.6
pkg/reconciler/openshift/tektonresult/extension.go	52.0%	50.0%	-2.0
pkg/reconciler/shared/tektonconfig/controller.go	Do not exist	0.0%
pkg/reconciler/shared/tektonconfig/result/result.go	Do not exist	71.4%
pkg/reconciler/shared/tektonconfig/tektonconfig.go	Do not exist	0.0%

pratap0007 · 2025-02-06T13:03:28Z

/retest

pratap0007 · 2025-02-06T15:14:34Z

/test pull-tekton-operator-integration-tests

tekton-robot · 2025-02-06T17:15:10Z

@pratap0007: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-tekton-operator-integration-tests	`e629301`	link	true	`/test pull-tekton-operator-integration-tests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 7, 2024

tekton-robot requested review from jkhelil and PuneetPunamiya November 7, 2024 23:37

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 7, 2024

khrm reviewed Nov 11, 2024

View reviewed changes

pratap0007 force-pushed the default-results branch 2 times, most recently from dbb72d6 to 42f99c8 Compare November 25, 2024 07:47

pratap0007 changed the title ~~[WIP] Automate the results installation process~~ Add Results configuration in tekton config Nov 25, 2024

pratap0007 marked this pull request as ready for review November 25, 2024 07:58

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2024

tekton-robot requested a review from piyush-garg November 25, 2024 07:58

pratap0007 force-pushed the default-results branch from 42f99c8 to a1ce3b9 Compare November 25, 2024 09:17

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 25, 2024

pratap0007 force-pushed the default-results branch 2 times, most recently from 1c5b5ff to 13655eb Compare November 25, 2024 09:46

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 25, 2024

pratap0007 force-pushed the default-results branch from 13655eb to 055a55b Compare November 25, 2024 13:59

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2024

khrm reviewed Jan 15, 2025

View reviewed changes

pratap0007 force-pushed the default-results branch from 00bbc52 to 0260ad4 Compare January 15, 2025 13:25

l-qing reviewed Jan 16, 2025

View reviewed changes

pratap0007 force-pushed the default-results branch from 0260ad4 to 70f1a26 Compare January 16, 2025 10:32

l-qing approved these changes Jan 17, 2025

View reviewed changes

pkg/reconciler/kubernetes/tektonresult/tektonresult.go Outdated Show resolved Hide resolved

pratap0007 force-pushed the default-results branch from 70f1a26 to 71f5652 Compare January 20, 2025 05:33

tekton-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 20, 2025

pratap0007 force-pushed the default-results branch from 71f5652 to 7d696c2 Compare January 22, 2025 12:55

pratap0007 force-pushed the default-results branch from 7d696c2 to 00091f3 Compare January 23, 2025 07:49

jkhelil reviewed Jan 28, 2025

View reviewed changes

pratap0007 force-pushed the default-results branch from 00091f3 to 34b0e4d Compare January 28, 2025 14:30

jkhelil reviewed Feb 4, 2025

View reviewed changes

Add Results configuration in tekton config

e629301

This will add the Results configuration to the tekton config and will be installed by default through tekton config Signed-off-by: Shiv Verma <[email protected]>

pratap0007 force-pushed the default-results branch from 34b0e4d to e629301 Compare February 4, 2025 08:54

Add Results configuration in tekton config #2398

Are you sure you want to change the base?

Add Results configuration in tekton config #2398

Conversation

pratap0007 commented Nov 7, 2024 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Nov 7, 2024

khrm left a comment

Choose a reason for hiding this comment

jkandasa commented Nov 19, 2024

tekton-robot commented Nov 25, 2024

tekton-robot commented Nov 25, 2024

tekton-robot commented Nov 25, 2024

tekton-robot commented Nov 25, 2024

pratap0007 commented Nov 26, 2024

tekton-robot commented Nov 26, 2024

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Jan 15, 2025

pratap0007 commented Jan 15, 2025

l-qing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jan 16, 2025

l-qing left a comment

Choose a reason for hiding this comment

tekton-robot commented Jan 17, 2025

tekton-robot commented Jan 20, 2025

tekton-robot commented Jan 22, 2025

tekton-robot commented Jan 23, 2025

pratap0007 commented Jan 23, 2025

l-qing commented Jan 23, 2025

l-qing commented Jan 24, 2025

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

tekton-robot commented Jan 28, 2025

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

pratap0007 commented Feb 6, 2025

tekton-robot commented Feb 6, 2025

pratap0007 commented Feb 6, 2025

pratap0007 commented Feb 6, 2025

tekton-robot commented Feb 6, 2025

pratap0007 commented Nov 7, 2024 •

edited

Loading