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

Move clustertask and clustertriggerbinding role to component #2164

Merged
merged 1 commit into from
May 22, 2024

Conversation

piyush-garg
Copy link
Contributor

This will move clustertask and clustertriggerbinding view permissions for all system authenticated users on openshift to be installed along with component installation instead of doing it with addon resources.

This will help users to move to basic or lite profile without permission issues, also if user disable default clustertasks installation then also permission issue will not happen

Also clustertask permissions were getting created as part of all installersets like custom and versioned installerset, and later in new versioned installerset also if upgraded. This was creating race between the different installerset reconiler to update resource because of the different installerset name was getting added to ownerReference resulting in different yaml and sha value, so this will also fix that as we moved the permissions out of addon installerset and there will be single instance. I have changed the name of resources to not conflict with old name and we can users to delete old clustertask installerset if they are not using old clustertasks

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

Move clustertask and clustertriggerbinding role to component

This will move clustertask and clustertriggerbinding view permissions for
all system authenticated users on openshift to be installed along with
component installation instead of doing it with addon resources.

This will help users to move to basic or lite profile without
permission issues, also if user disable default clustertasks installation
then also permission issue will not happen

Also clustertask permissions were getting created as part of all
installersets like custom and versioned installerset, and later in new
versioned installerset also if upgraded. This was creating race between
the different installerset reconiler to update resource because of the
different installerset name was getting added to ownerReference resulting
in different yaml and sha value, so this will also fix that as we moved
the permissions out of addon installerset and there will be single instance.
I have changed the name of resources to not conflict with old name and
we can users to delete old clustertask installerset if they are not
using old clustertasks
@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 21, 2024
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 21, 2024
@piyush-garg
Copy link
Contributor Author

/cherry-pick main

@tekton-robot
Copy link
Contributor

@piyush-garg: once the present PR merges, I will cherry-pick it on top of main in a new PR and assign it to you.

In response to this:

/cherry-pick main

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.

@savitaashture
Copy link
Contributor

savitaashture commented May 21, 2024

Should we do this for https://github.com/tektoncd/operator/tree/main/cmd/openshift/operator/kodata/tekton-addon/addons/07-ecosystem ?

I have changed the name of resources to not conflict with old name and we can users to delete old clustertask installerset if they are not using old clustertasks

Should we indicate this in RN ? and is this a breaking change for existing users ?

Copy link
Member

@jkandasa jkandasa left a comment

Choose a reason for hiding this comment

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

LGTM

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkandasa

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 May 22, 2024
@piyush-garg
Copy link
Contributor Author

Should we do this for https://github.com/tektoncd/operator/tree/main/cmd/openshift/operator/kodata/tekton-addon/addons/07-ecosystem ?

these roles are specific to getting task only in openshift-pipelines, while the clustertask and clustertriggerbinding are different case as these are clustertwide resources and end up in breaking profiles, UI etc

I have changed the name of resources to not conflict with old name and we can users to delete old clustertask installerset if they are not using old clustertasks

Should we indicate this in RN ? and is this a breaking change for existing users ?

changing the name of the role/rolebinding which is not exposed to user, should not be a breaking change for users

@PuneetPunamiya
Copy link
Member

Great findings 🙌🏻
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2024
@tekton-robot tekton-robot merged commit a742d9a into tektoncd:release-v0.71.x May 22, 2024
6 checks passed
@tekton-robot
Copy link
Contributor

@piyush-garg: new pull request created: #2165

In response to this:

/cherry-pick main

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.

@jkandasa jkandasa added the kind/bug Categorizes issue or PR as related to a bug. label May 29, 2024
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. kind/bug Categorizes issue or PR as related to a bug. 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.

5 participants