Skip to content

Conversation

@nikhil-thomas
Copy link
Member

@nikhil-thomas nikhil-thomas commented Jan 25, 2022

Fix occurrence of duplicate installerSets in pre, post and main
recociler logic of TektonPipeline reconciler

Adds a label based querying to identify InstallerSets instead of
querying by there name

Use Operator version instead of Component versions while deciding whether to recreate deployments and other resources during upgrades

Signed-off-by: Nikhil Thomas nikthoma@redhat.com

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

fixes duplicate pipeline (pre, post, main)  installerSets occuring in in TektoncdPipeline reconciler. 

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 25, 2022
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 25, 2022
@nikhil-thomas nikhil-thomas force-pushed the fix/duplicate-pipeline-installersets branch 2 times, most recently from fe9fa9f to 2502a9d Compare January 26, 2022 11:03
@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/reconciler/kubernetes/tektoninstallerset/query.go Do not exist 83.3%

@nikhil-thomas nikhil-thomas force-pushed the fix/duplicate-pipeline-installersets branch from d4d7f9e to 5c7e936 Compare January 26, 2022 12:35
@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/reconciler/kubernetes/tektoninstallerset/query.go Do not exist 83.3%

@nikhil-thomas nikhil-thomas force-pushed the fix/duplicate-pipeline-installersets branch from 5c7e936 to f623c33 Compare January 26, 2022 13:50
Fix occurrence of duplicate installerSets in pre, post and main
recociler logic of TektonPipeline reconciler

Adds a label based querying to identify InstallerSets instead of
querying by there name

Signed-off-by: Nikhil Thomas <nikthoma@redhat.com>
Use Operator version instead of Component versions while deciding
whether to recreate deployments and other resources during upgrades

Signed-off-by: Nikhil Thomas <nikthoma@redhat.com>
@nikhil-thomas nikhil-thomas force-pushed the fix/duplicate-pipeline-installersets branch from f623c33 to cc89c23 Compare January 26, 2022 13: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/reconciler/kubernetes/tektoninstallerset/query.go Do not exist 83.3%

@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/reconciler/kubernetes/tektoninstallerset/query.go Do not exist 83.3%

Add mechanism in TektonTriggers reconciler to make sure that duplicate
InstallerSets doesnot exist.

Move label based InstallerSet querying mechanism to tektoninstallerset
package

Signed-off-by: Nikhil Thomas <nikthoma@redhat.com>
@nikhil-thomas nikhil-thomas force-pushed the fix/duplicate-pipeline-installersets branch from cc89c23 to afe8d85 Compare January 27, 2022 05:09
@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/reconciler/kubernetes/tektoninstallerset/query.go Do not exist 83.3%

@pradeepitm12
Copy link
Contributor

most of the review comments already addressed 👍
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2022
Copy link
Contributor

@savitaashture savitaashture left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: savitaashture

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

The pull request process is described here

Details 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 Jan 27, 2022
ReadOnly bool
}

func OperatorVersion(ctx context.Context) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we read this from the config map ?

Copy link
Member

@sm43 sm43 Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but the version is already in the env of pod so easier that way
why to do an API call.. :)
also configmap is for other external tools to read the version of operator

@sm43
Copy link
Member

sm43 commented Jan 27, 2022

Thank you !
/meow

@tekton-robot
Copy link
Contributor

@sm43: cat image

Details

In response to this:

Thank you !
/meow

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.

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants