Refactors TektonTriggers to use TektonInstallerSet #408

sm43 · 2021-09-13T05:46:03Z

Updates Triggers reconciler to use TektonInstallerSet

This updates the triggers reconciler to use TektonInstallerSet
for installation, this would allow change of TargetNamespace and
deletion of obsolete resources on upgrades.

Add AllDeploymentsReady Conditions in TektonInstallerSet

If there is any other deployment other than controller and
webhook then it will be missed by installer set for checking the
status so this adds another condition which marks the status of
any other deployment other than controller and webhook.

Signed-off-by: Shivam Mukhade [email protected]

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

tekton-robot · 2021-09-13T05:50:05Z

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/tektoninstallerset_lifecycle.go	62.5%	59.3%	-3.2
pkg/apis/operator/v1alpha1/tektontrigger_lifecycle.go	76.5%	37.5%	-39.0

tekton-robot · 2021-09-13T06:06:49Z

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/tektoninstallerset_lifecycle.go	62.5%	59.3%	-3.2
pkg/apis/operator/v1alpha1/tektontrigger_lifecycle.go	76.5%	37.5%	-39.0
pkg/reconciler/common/deployments.go	94.4%	0.0%	-94.4
pkg/reconciler/common/install.go	61.8%	0.0%	-61.8
pkg/reconciler/common/releases.go	61.8%	32.4%	-29.4
pkg/reconciler/common/stages.go	67.9%	21.4%	-46.4

tekton-robot · 2021-09-13T06:27:09Z

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/tektoninstallerset_lifecycle.go	62.5%	59.3%	-3.2
pkg/apis/operator/v1alpha1/tektontrigger_lifecycle.go	76.5%	37.5%	-39.0

tekton-robot · 2021-09-13T06:53:52Z

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/tektoninstallerset_lifecycle.go	62.5%	59.3%	-3.2
pkg/apis/operator/v1alpha1/tektontrigger_lifecycle.go	76.5%	37.5%	-39.0
pkg/reconciler/common/deployments.go	94.4%	0.0%	-94.4
pkg/reconciler/common/install.go	61.8%	0.0%	-61.8
pkg/reconciler/common/releases.go	61.8%	32.4%	-29.4
pkg/reconciler/common/stages.go	67.9%	21.4%	-46.4

nikhil-thomas · 2021-09-13T14:18:03Z

/approve

pkg/reconciler/common/deployments_test.go

pkg/reconciler/common/install_test.go

pkg/reconciler/common/releases_test.go

pkg/reconciler/kubernetes/tektoninstallerset/install.go

pkg/reconciler/kubernetes/tektontrigger/controller.go

nikhil-thomas · 2021-09-13T14:28:35Z

@sm43 the reconciler is much simpler now 👍
could we remove the commented lines.

vdemeester

We do not have a huge test coverage, and this PR removes quite some too, most likely we should port the test to the new struct to make sure we are still covered.

sm43 · 2021-09-14T04:25:40Z

We do not have a huge test coverage, and this PR removes quite some too, most likely we should port the test to the new struct to make sure we are still covered.

@vdemeester yeah test coverage is less. I was going to address it once refactoring is done, but it make sense to start as if there is a bug we will catch.
I will add test cases for new code then we can refactor rest of the code.
cc @nikhil-thomas

tekton-robot · 2021-09-14T04:34: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/tektontrigger_lifecycle.go	76.5%	34.4%	-42.1

tekton-robot · 2021-09-15T04:43:12Z

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/tektoninstallerset_lifecycle.go	62.5%	59.3%	-3.2
pkg/apis/operator/v1alpha1/tektontrigger_lifecycle.go	76.5%	37.5%	-39.0
pkg/reconciler/common/deployments.go	94.4%	0.0%	-94.4
pkg/reconciler/common/install.go	61.8%	0.0%	-61.8
pkg/reconciler/common/releases.go	61.8%	32.4%	-29.4
pkg/reconciler/common/stages.go	67.9%	21.4%	-46.4
pkg/reconciler/common/utils.go	Do not exist	78.6%
pkg/reconciler/kubernetes/tektoninstallerset/install.go	0.0%	80.9%	80.9

sm43 · 2021-09-15T05:12:00Z

/test pull-tekton-operator-integration-tests

tekton-robot · 2021-09-15T14:01:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,vdemeester]

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

pkg/reconciler/kubernetes/tektontrigger/tektontrigger.go

tekton-robot · 2021-09-16T05:21:11Z

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/tektoninstallerset_lifecycle.go	62.5%	59.3%	-3.2
pkg/apis/operator/v1alpha1/tektontrigger_lifecycle.go	76.5%	37.5%	-39.0
pkg/reconciler/common/deployments.go	94.4%	0.0%	-94.4
pkg/reconciler/common/install.go	61.8%	0.0%	-61.8
pkg/reconciler/common/releases.go	61.8%	32.4%	-29.4
pkg/reconciler/common/stages.go	67.9%	21.4%	-46.4
pkg/reconciler/common/utils.go	Do not exist	78.6%
pkg/reconciler/kubernetes/tektoninstallerset/install.go	0.0%	80.9%	80.9

tekton-robot · 2021-09-16T06:21:40Z

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/tektoninstallerset_lifecycle.go	62.5%	59.3%	-3.2
pkg/apis/operator/v1alpha1/tektontrigger_lifecycle.go	76.5%	37.5%	-39.0
pkg/reconciler/common/deployments.go	94.4%	0.0%	-94.4
pkg/reconciler/common/install.go	61.8%	0.0%	-61.8
pkg/reconciler/common/releases.go	61.8%	32.4%	-29.4
pkg/reconciler/common/stages.go	67.9%	21.4%	-46.4
pkg/reconciler/common/utils.go	Do not exist	78.6%
pkg/reconciler/kubernetes/tektoninstallerset/install.go	0.0%	80.9%	80.9

tekton-robot · 2021-09-20T03:58:12Z

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/tektoninstallerset_lifecycle.go	62.5%	59.3%	-3.2
pkg/apis/operator/v1alpha1/tektontrigger_lifecycle.go	76.5%	37.5%	-39.0
pkg/reconciler/common/deployments.go	94.4%	0.0%	-94.4
pkg/reconciler/common/install.go	61.8%	0.0%	-61.8
pkg/reconciler/common/releases.go	61.8%	32.4%	-29.4
pkg/reconciler/common/stages.go	67.9%	21.4%	-46.4
pkg/reconciler/common/utils.go	Do not exist	78.6%
pkg/reconciler/kubernetes/tektoninstallerset/install.go	0.0%	80.9%	80.9

nikhil-thomas · 2021-09-20T07:44:24Z

pkg/reconciler/common/utils.go

+func FetchVersionFromCRD(manifest mf.Manifest, releaseLabel string) (string, error) {
+	crds := manifest.Filter(mf.CRDs)
+	if len(crds.Resources()) == 0 {
+		return "", fmt.Errorf("failed to find crds to get release version")
+	}
+
+	crd := crds.Resources()[0]
+	version, ok := crd.GetLabels()[releaseLabel]
+	if !ok {
+		return version, fmt.Errorf("failed to find release label on crd")
+	}
+
+	return version, nil
+}


this good.

As a future improvement, how about reading the version from the "info" configMap example.

May be we can set it as unknown, if the configMap is not available.

What do you think.
if you think that could be a useful improvement, then could you create an issue (#good-first-issue, #help-wanted) to capture it.

else, i am curious to know why. and feel free to resolve this 🙂 👍

note: this comment ☝️ is not a blocker.

created #413

nikhil-thomas · 2021-09-20T07:50:53Z

LGTM

If there is any other deployment other than controller and webhook then it will be missed by installer set for checking the status so this adds another condition which marks the status of any other deployment other than controller and webhook. Signed-off-by: Shivam Mukhade <[email protected]>

This updates the triggers reconciler to use TektonInstallerSet for installation, this would allow change of TargetNamespace and deletion of obsolete resources on upgrades. Signed-off-by: Shivam Mukhade <[email protected]>

This adds new test for installer and removes old obsolete tests from common pkg. Signed-off-by: Shivam Mukhade <[email protected]>

Signed-off-by: Shivam Mukhade <[email protected]>

tekton-robot · 2021-09-20T08:47:08Z

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/tektoninstallerset_lifecycle.go	62.5%	59.3%	-3.2
pkg/apis/operator/v1alpha1/tektontrigger_lifecycle.go	76.5%	37.5%	-39.0
pkg/reconciler/common/deployments.go	94.4%	0.0%	-94.4
pkg/reconciler/common/install.go	61.8%	0.0%	-61.8
pkg/reconciler/common/releases.go	61.8%	32.4%	-29.4
pkg/reconciler/common/stages.go	67.9%	21.4%	-46.4
pkg/reconciler/common/utils.go	Do not exist	78.6%
pkg/reconciler/kubernetes/tektoninstallerset/install.go	0.0%	80.9%	80.9

nikhil-thomas · 2021-09-20T09:04:54Z

/lgtm

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

tekton-robot requested review from piyush-garg and pradeepitm12 September 13, 2021 05:46

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 13, 2021

sm43 force-pushed the refactor-triggers branch from 73df580 to 4b5add8 Compare September 13, 2021 06:03

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 13, 2021

sm43 force-pushed the refactor-triggers branch from 4b5add8 to 70fe2a0 Compare September 13, 2021 06:23

sm43 force-pushed the refactor-triggers branch from 70fe2a0 to a9694cc Compare September 13, 2021 06:50

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

nikhil-thomas reviewed Sep 13, 2021

View reviewed changes

pkg/reconciler/common/deployments_test.go Outdated Show resolved Hide resolved

nikhil-thomas reviewed Sep 13, 2021

View reviewed changes

pkg/reconciler/common/install_test.go Outdated Show resolved Hide resolved

nikhil-thomas reviewed Sep 13, 2021

View reviewed changes

pkg/reconciler/common/releases_test.go Outdated Show resolved Hide resolved

nikhil-thomas reviewed Sep 13, 2021

View reviewed changes

pkg/reconciler/kubernetes/tektoninstallerset/install.go Show resolved Hide resolved

nikhil-thomas reviewed Sep 13, 2021

View reviewed changes

pkg/reconciler/kubernetes/tektontrigger/controller.go Outdated Show resolved Hide resolved

vdemeester reviewed Sep 13, 2021

View reviewed changes

sm43 force-pushed the refactor-triggers branch from a9694cc to b52d522 Compare September 14, 2021 04:30

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 14, 2021

sm43 changed the title ~~Refactors TektonTriggers to use TektonInstallerSet~~ [WIP] Refactors TektonTriggers to use TektonInstallerSet Sep 14, 2021

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2021

sm43 force-pushed the refactor-triggers branch from b52d522 to a9694cc Compare September 14, 2021 08:25

sm43 changed the title ~~[WIP] Refactors TektonTriggers to use TektonInstallerSet~~ Refactors TektonTriggers to use TektonInstallerSet Sep 15, 2021

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 Sep 15, 2021

sm43 requested review from nikhil-thomas and vdemeester September 15, 2021 13:00

vdemeester approved these changes Sep 15, 2021

View reviewed changes

PuneetPunamiya reviewed Sep 16, 2021

View reviewed changes

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

sm43 force-pushed the refactor-triggers branch from 1b6fed2 to bbffb93 Compare September 16, 2021 05:17

sm43 force-pushed the refactor-triggers branch from bbffb93 to f0e4446 Compare September 16, 2021 06:18

sm43 force-pushed the refactor-triggers branch from f0e4446 to 4520944 Compare September 20, 2021 03:54

nikhil-thomas reviewed Sep 20, 2021

View reviewed changes

Shivam Mukhade added 4 commits September 20, 2021 13:53

Updates Triggers reconciler to use TektonInstallerSet

4b69799

This updates the triggers reconciler to use TektonInstallerSet for installation, this would allow change of TargetNamespace and deletion of obsolete resources on upgrades. Signed-off-by: Shivam Mukhade <[email protected]>

Adds unit test for installer in TektonInstallerSet

6b47edb

This adds new test for installer and removes old obsolete tests from common pkg. Signed-off-by: Shivam Mukhade <[email protected]>

Cleanup: moves common code for components to common pkg

98def76

Signed-off-by: Shivam Mukhade <[email protected]>

sm43 force-pushed the refactor-triggers branch from 4520944 to 98def76 Compare September 20, 2021 08:43

tekton-robot assigned nikhil-thomas Sep 20, 2021

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

tekton-robot merged commit a3d2802 into tektoncd:main Sep 20, 2021

sm43 deleted the refactor-triggers branch September 20, 2021 09:27

sm43 mentioned this pull request Sep 24, 2021

Refactor TektonComponent with new refactoring changes #424

Closed

nikhil-thomas mentioned this pull request Oct 8, 2021

Delete old resources during upgrade when a component no longer includes them #374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors TektonTriggers to use TektonInstallerSet #408

Refactors TektonTriggers to use TektonInstallerSet #408

sm43 commented Sep 13, 2021 •

edited

Loading

tekton-robot commented Sep 13, 2021

tekton-robot commented Sep 13, 2021

tekton-robot commented Sep 13, 2021

tekton-robot commented Sep 13, 2021

nikhil-thomas commented Sep 13, 2021

nikhil-thomas commented Sep 13, 2021

vdemeester left a comment

sm43 commented Sep 14, 2021 •

edited

Loading

tekton-robot commented Sep 14, 2021

tekton-robot commented Sep 15, 2021

sm43 commented Sep 15, 2021

tekton-robot commented Sep 15, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 20, 2021

nikhil-thomas Sep 20, 2021

nikhil-thomas Sep 20, 2021

sm43 Sep 20, 2021

nikhil-thomas commented Sep 20, 2021

tekton-robot commented Sep 20, 2021

nikhil-thomas commented Sep 20, 2021

Refactors TektonTriggers to use TektonInstallerSet #408

Refactors TektonTriggers to use TektonInstallerSet #408

Conversation

sm43 commented Sep 13, 2021 • edited Loading

Submitter Checklist

Release Notes

tekton-robot commented Sep 13, 2021

tekton-robot commented Sep 13, 2021

tekton-robot commented Sep 13, 2021

tekton-robot commented Sep 13, 2021

nikhil-thomas commented Sep 13, 2021

nikhil-thomas commented Sep 13, 2021

vdemeester left a comment

Choose a reason for hiding this comment

sm43 commented Sep 14, 2021 • edited Loading

tekton-robot commented Sep 14, 2021

tekton-robot commented Sep 15, 2021

sm43 commented Sep 15, 2021

tekton-robot commented Sep 15, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 20, 2021

nikhil-thomas Sep 20, 2021

Choose a reason for hiding this comment

nikhil-thomas Sep 20, 2021

Choose a reason for hiding this comment

sm43 Sep 20, 2021

Choose a reason for hiding this comment

nikhil-thomas commented Sep 20, 2021

tekton-robot commented Sep 20, 2021

nikhil-thomas commented Sep 20, 2021

sm43 commented Sep 13, 2021 •

edited

Loading

sm43 commented Sep 14, 2021 •

edited

Loading