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.

Sign up for GitHub

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

Jump to bottom

Packages ClusterRoles with Triggers deployment for eventlistener #1158

Merged
merged 2 commits into from
Jul 23, 2021
Merged

Packages ClusterRoles with Triggers deployment for eventlistener #1158

merged 2 commits into from
Jul 23, 2021

Conversation

sm43
Copy link
Member

@sm43 sm43 commented Jul 21, 2021 •
edited by dibyom
Loading

This packages 2 clusterroles with triggers deployment for el
that can be used by users for their deployments.
User will have to creates following resources:

  • a serviceaccount which would be used with eventlistener
  • a rolebinding with above sa and tekton-triggers-eventlistener-roles clusterrole
  • a clusterrolebinding with above sa and tekton-triggers-eventlistener-clusterroles
    clusterrole

Closes #1119

Signed-off-by: Shivam Mukhade smukhade@redhat.com

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
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Triggers packages 2 clusterroles for eventlisteners which can be used by users for their eventlistener deployment which will provide el required access in a particular namespace.
user will have to create following resources
- a serviceaccount which would be used with eventlistener
- a rolebinding with above sa and `tekton-triggers-eventlistener-roles` clusterrole
- a clusterrolebinding with above sa and `tekton-triggers-eventlistener-clusterroles`
  clusterrole

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 21, 2021
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 21, 2021
@sm43
Copy link
Member Author

sm43 commented Jul 21, 2021

/cc @dibyom

@tekton-robot tekton-robot requested a review from dibyom July 21, 2021 17:04
dibyom
dibyom reviewed Jul 22, 2021
View reviewed changes
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

Looks good...my only comment is to see if we can make the names a bit smaller but that's minor

config/200-clusterrole.yaml Outdated
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: tekton-triggers-eventlistener
Copy link
Member

Choose a reason for hiding this comment

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

what do you think of calling this simply eventlistener-roles and the other one eventlistener-clusterroles

Copy link
Member Author

Choose a reason for hiding this comment

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

end users might have many things installed on the cluster so if we keep tekton-triggers- as prefix so wouldn't it be easier to understand where the clusterrole came from.

Copy link
Member Author

Choose a reason for hiding this comment

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

and also it would be uniform with other resources we package

Copy link
Member

Choose a reason for hiding this comment

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

fair enough!

@dibyom
Copy link
Member

dibyom commented Jul 23, 2021

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2021
@dibyom
Copy link
Member

dibyom commented Jul 23, 2021

/approve

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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 Jul 23, 2021
@sm43
Copy link
Member Author

sm43 commented Jul 23, 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 Jul 23, 2021
@sm43
Copy link
Member Author

sm43 commented Jul 23, 2021

let me rename it to -roles and -clusterroles sounds better :)

sm43 added 2 commits July 23, 2021 09:30
Packages ClusterRoles with Triggers deployment for eventlistener
4b0adce 
This packages 2 clusterroles with triggers deployment for el
that can be used by users for their deployments.
User will have to creates following resources:
- a serviceaccount which would be used with eventlistener
- a rolebinding with above sa and `tekton-triggers-eventlistener-roles` clusterrole
- a clusterrolebinding with above sa and `tekton-triggers-eventlistener-clusterroles`
  clusterrole

Signed-off-by: Shivam Mukhade smukhade@redhat.com
Update examples to use clusterrole from triggers deployment
a933b6d 
Signed-off-by: Shivam Mukhade smukhade@redhat.com
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2021
@sm43
Copy link
Member Author

sm43 commented Jul 23, 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 Jul 23, 2021
@savitaashture
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2021
@tekton-robot tekton-robot merged commit 3c4e26b into tektoncd:main Jul 23, 2021
@dibyom
Copy link
Member

dibyom commented Jul 23, 2021

@sm43 One thing I forgot -- could you add a release notes section to this PR?

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 23, 2021
@sm43 sm43 deleted the package-roles branch July 23, 2021 15:40
@sm43
Copy link
Member Author

sm43 commented Jul 23, 2021

@sm43 One thing I forgot -- could you add a release notes section to this PR?

/kind feature

@dibyom done. would it be better to add this in docs somewhere? could you point where I can add this in docs

@sm43 sm43 mentioned this pull request Jul 23, 2021
Closed
@dibyom
Copy link
Member

dibyom commented Jul 23, 2021

Yup!
https://github.com/tektoncd/triggers/blob/main/docs/eventlisteners.md#specifying-the-kubernetes-service-account seems like the right place

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dibyom dibyom dibyom left review comments

@dlorenc dlorenc Awaiting requested review from dlorenc

@vtereso vtereso Awaiting requested review from vtereso

Assignees

@dibyom dibyom

@savitaashture savitaashture

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Package Roles for EventListener SA's with Triggers Releases
4 participants
@sm43 @dibyom @tekton-robot @savitaashture