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

Fixes namespac in clusterrolebinding in getting-started #1106

Merged
merged 1 commit into from May 25, 2021

Conversation

sm43
Copy link
Member

@sm43 sm43 commented May 21, 2021

Previously we use to create ClusterRoleBinding with service account
in default ns as admin-role.yaml is symbolic link with examples/rbac.yaml,
this adds a new clusterrolebinding in docs/getting-started/rbac to
update the namespace.

Closes #990

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

@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, 2021
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 21, 2021
@dibyom
Copy link
Member

dibyom commented May 21, 2021

I think this is the same as #1075?

My preference would be to only duplicate the ClusterRoleBinding instead of the entire role so that we do not have to keep both in sync when we update RBAC configs.

@sm43
Copy link
Member Author

sm43 commented May 21, 2021

I think this is the same as #1075?

My preference would be to only duplicate the ClusterRoleBinding instead of the entire role so that we do not have to keep both in sync when we update RBAC configs.

@dibyom yep. #1075 will fix the issue. Do you mean we keep existing as it is and create a new yaml to add the required clusterrolebinding? or let #1075 merge with updated readme and close this pr?

@dibyom
Copy link
Member

dibyom commented May 21, 2021

yes!

@sm43 sm43 force-pushed the fix-ns-in-getting-started branch from 03c907c to b40dead Compare May 24, 2021 13:48
@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 24, 2021
@sm43 sm43 changed the title Fixes namespace in rbac resources in getting-started Fixes namespac in clusterrolebinding in getting-started May 24, 2021
@sm43
Copy link
Member Author

sm43 commented May 24, 2021

@dibyom Could you please take a look now?

@sm43
Copy link
Member Author

sm43 commented May 24, 2021

/test pull-tekton-triggers-integration-tests

Previously we use to create ClusterRoleBinding with service account
in default ns as admin-role.yaml is symbolic link with examples/rbac.yaml,
this adds a new clusterrolebinding in docs/getting-started/rbac to
update the namespace.

Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
@sm43 sm43 force-pushed the fix-ns-in-getting-started branch from b40dead to 65e85ab Compare May 24, 2021 16:37
@sm43 sm43 requested a review from dibyom May 24, 2021 16:37
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.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 24, 2021
@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 May 24, 2021
@sm43
Copy link
Member Author

sm43 commented May 25, 2021

/test pull-tekton-triggers-integration-tests
/test tekton-triggers-unit-tests

@tekton-robot tekton-robot merged commit baeb631 into tektoncd:main May 25, 2021
@sm43 sm43 deleted the fix-ns-in-getting-started branch May 25, 2021 02:24
@EarthlingDavey
Copy link

EarthlingDavey commented May 26, 2021

@sm43 it would have been considerate to ask about my progress on this same issue. A shafting, no matter how small is still a shafting. Lol.

@dibyom it is quite discouraging to have been in discussion, and come to an agreed upon solution only to find that I have wasted my time.

I understand that you probably have a tonne of work, but if you have time or inclination in the future, it may be nicer, especially for newbie developers, if you managed PR clashes with a little more consideration.

Edit: so you understand my sentiment I was also pretty stoked to be able to contribute to such a new and cutting edge project. One that I'm sure I will be using for years to come. Thanks both for you work on the project. And perhaps I will be able to contribute some other time. Have a good week ✌️

@sm43
Copy link
Member Author

sm43 commented May 26, 2021

@EarthlingDavey Yep. I should have asked you about the status of your PR. Sorry for the mistake :(

@EarthlingDavey
Copy link

Hi, @sm43 thanks for acknowledging :) As I said, I'm sure I'll be using tekton (and k8s) for a long while so I may see you round.

@dibyom
Copy link
Member

dibyom commented May 26, 2021

@EarthlingDavey Yikes, totally my bad. I noticed the PR conflict initially but then it slipped my mind when I was approving it. Sorry about that. I appreciate your effort and hope you will still contribute to the project!

@EarthlingDavey
Copy link

@dibyom thanks. On reflection, I should have really commented here when you tagged my PR. It's all good learnings for me.
Looking forward to contributing again where possible. Cheers :)

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting Started clusterrolebinding namespace is wrong
4 participants