Skip to content
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

Refactors TektonPipeline Reconciler #385

Merged
merged 4 commits into from
Sep 6, 2021

Conversation

sm43
Copy link
Member

@sm43 sm43 commented Aug 18, 2021

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

Changes

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

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 18, 2021
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 18, 2021
@sm43
Copy link
Member Author

sm43 commented Aug 18, 2021

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2021
@tekton-robot
Copy link
Contributor

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/tektonpipeline_lifecycle.go 76.5% 35.1% -41.3

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2021
@nikhil-thomas
Copy link
Member

The refacotring is great. Now the reconciler is more Readable 👍
I have a few comments apart from that everything else looks great.

@sm43 sm43 force-pushed the refactor-pipelines branch from f261e40 to 811229d Compare September 2, 2021 11:19
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2021
@sm43
Copy link
Member Author

sm43 commented Sep 2, 2021

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2021
@tekton-robot
Copy link
Contributor

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 Do not exist 58.3%
pkg/apis/operator/v1alpha1/tektonpipeline_lifecycle.go 76.5% 35.5% -41.0
pkg/reconciler/kubernetes/tektoninstallerset/controller.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/install.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/transformer.go Do not exist 85.7%

@sm43 sm43 force-pushed the refactor-pipelines branch from 811229d to c3c7d1e Compare September 2, 2021 11:57
@tekton-robot
Copy link
Contributor

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 Do not exist 62.5%
pkg/apis/operator/v1alpha1/tektonpipeline_lifecycle.go 76.5% 35.5% -41.0
pkg/reconciler/kubernetes/tektoninstallerset/controller.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/install.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/transformer.go Do not exist 85.7%

@sm43 sm43 force-pushed the refactor-pipelines branch from c3c7d1e to d8757e8 Compare September 2, 2021 12:17
@sm43 sm43 requested a review from nikhil-thomas September 2, 2021 12:18
@tekton-robot
Copy link
Contributor

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 Do not exist 62.5%
pkg/apis/operator/v1alpha1/tektonpipeline_lifecycle.go 76.5% 35.5% -41.0
pkg/reconciler/common/transformers.go 72.8% 72.6% -0.2
pkg/reconciler/kubernetes/tektoninstallerset/controller.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/install.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/transformer.go Do not exist 85.7%

@sm43 sm43 force-pushed the refactor-pipelines branch from d8757e8 to 6f76b77 Compare September 2, 2021 17:51
@tekton-robot
Copy link
Contributor

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 Do not exist 62.5%
pkg/apis/operator/v1alpha1/tektonpipeline_lifecycle.go 76.5% 36.7% -39.8
pkg/reconciler/common/transformers.go 72.8% 72.6% -0.2
pkg/reconciler/kubernetes/tektoninstallerset/controller.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/install.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/transformer.go Do not exist 85.7%

@tekton-robot
Copy link
Contributor

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 Do not exist 62.5%
pkg/apis/operator/v1alpha1/tektonpipeline_lifecycle.go 76.5% 36.7% -39.8
pkg/reconciler/common/transformers.go 72.8% 72.6% -0.2
pkg/reconciler/kubernetes/tektoninstallerset/controller.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/install.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/transformer.go Do not exist 85.7%

@nikhil-thomas
Copy link
Member

/approve

@tekton-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2021
@sm43 sm43 force-pushed the refactor-pipelines branch from de79b56 to ab0dc03 Compare September 6, 2021 03:25
@tekton-robot
Copy link
Contributor

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 Do not exist 62.5%
pkg/apis/operator/v1alpha1/tektonpipeline_lifecycle.go 76.5% 36.7% -39.8
pkg/reconciler/common/transformers.go 72.8% 72.6% -0.2

@sm43
Copy link
Member Author

sm43 commented Sep 6, 2021

/retest

@sm43 sm43 force-pushed the refactor-pipelines branch from ab0dc03 to a25cbea Compare September 6, 2021 08:29
@tekton-robot
Copy link
Contributor

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 Do not exist 62.5%
pkg/apis/operator/v1alpha1/tektonpipeline_lifecycle.go 76.5% 36.7% -39.8
pkg/reconciler/common/transformers.go 72.8% 72.6% -0.2

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2021
@sm43 sm43 force-pushed the refactor-pipelines branch from a25cbea to cd69d3e Compare September 6, 2021 12:17
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2021
@tekton-robot
Copy link
Contributor

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 Do not exist 62.5%
pkg/apis/operator/v1alpha1/tektonpipeline_lifecycle.go 76.5% 36.7% -39.8
pkg/reconciler/common/transformers.go 72.8% 72.6% -0.2

Shivam Mukhade added 4 commits September 6, 2021 17:58
This adds TektonInstaller which would install the manifest passed
in spec in a certain order defined, TektonPipeline/TektonTrigger and
other components will transform their manifest and pass to TektonInstallerSet
which will create the resources.
The components will create an tekton installer set and pass the manifest.
- while upgrading the version components can delete prev installer set
and create new one which will delete any obsolete resource removed in '
newer version.
- to change namespace of installed components, create a new installer set
and delete prev.

Deleting a installer will delete all resources except crds which would
not delete any user data.

Signed-off-by: Shivam Mukhade <[email protected]>
This updates the reconciler to use TektonInstallerSet to install
the pipelines component. If version is changed or target namespace is
changed then previous TektonInstallerSet will get deleted and a new
one will be created with required spec.

Signed-off-by: Shivam Mukhade <[email protected]>
As we have changed the installation flow for TektonPipelines
some of the test are failing as they use to run with TektonPipeline
so till we refactor all components changing them to run on
TektonTriggers as we refactor those will be updated.

Signed-off-by: Shivam Mukhade <[email protected]>
This enable the installer set for openshift and in extension
update the code to install resources directly instead of using
common pkg which has prev install flow, which would break now
as older condtions are used in common/install which are now
removed for TektonPipeline.

Signed-off-by: Shivam Mukhade <[email protected]>
@sm43 sm43 force-pushed the refactor-pipelines branch from cd69d3e to 82bd3b5 Compare September 6, 2021 12:29
@tekton-robot
Copy link
Contributor

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 Do not exist 62.5%
pkg/apis/operator/v1alpha1/tektonpipeline_lifecycle.go 76.5% 36.7% -39.8
pkg/reconciler/common/transformers.go 72.8% 72.6% -0.2
pkg/reconciler/kubernetes/tektoninstallerset/controller.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/install.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go Do not exist 0.0%
pkg/reconciler/kubernetes/tektoninstallerset/transformer.go Do not exist 85.7%

@nikhil-thomas
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2021
@tekton-robot tekton-robot merged commit 805be35 into tektoncd:main Sep 6, 2021
@sm43 sm43 deleted the refactor-pipelines branch October 10, 2021 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants