Default not readonly & tolerate missing permissions when removing #1062
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tektoncd#926 added a `--readonly` flag for adding readonly user permissions, however it added it with defualt "true" which means that by default when running this script to add or remove permissions, it will only modify the read only permissions and will leave any write permissions as-is. This means that when I went to update the governing board permissions - i.e. adding permissions for new members and removing permissions from old members, the script did very little and I couldn't figure out why. This commit switches the default to the writer roles - defaulting to reader is probably a safer default, however I think it's a confusing change given how the script has been used so far. Can be convinced that instead we should keep the default and add better documentation and examples (but someone else will probably need to pick that up!)
When removing permissions, if the permission to be removed isn't present, the script will stop. However, if the permission isn't there, imo that is okay and if the permissions list changes at all, this error could get it - imo it's better to keep running the script and just report any errors that couldn't be removed. I ran into this especially when I ran the remove script but only for readonly mode - from that point on I could no longer run the script for the broader permissions b/c it would encounter an error immediately since the viewer role had already been removed.
bobcatfish
added a commit
to bobcatfish/community
that referenced
this pull request
Apr 11, 2022
1. Finishing up updates from governing board election (andrew bayer no longer on the board, priya wadhwa joining the board) 2. jerop@ will be taking my place on the governing board while I am on leave (see [leave policy](https://github.com/tektoncd/community/blob/main/governance.md#governing-board-leave-policy)) (thanks jerop@ !!) Went through the list at https://github.com/tektoncd/community/blob/main/governance.md#changes-to-governing-board to make sure jerop and priya have all the governing board permissions and to make the changes required with andrew leaving the board. While running the permissions adding script I ran into a couple of difficulties which I've tried to fix in: * tektoncd/plumbing#1062 * tektoncd/plumbing#1061
bobcatfish
added a commit
to bobcatfish/community
that referenced
this pull request
Apr 11, 2022
1. Finishing up updates from governing board election (andrew bayer no longer on the board, priya wadhwa joining the board) 2. jerop@ will be taking my place on the governing board while I am on leave (see [leave policy](https://github.com/tektoncd/community/blob/main/governance.md#governing-board-leave-policy)) (thanks jerop@ !!) Went through the list at https://github.com/tektoncd/community/blob/main/governance.md#changes-to-governing-board to make sure jerop and priya have all the governing board permissions and to make the changes required with andrew leaving the board. Also added Priti to the governing board + community meeting faciltator rotation because she is listed in the rotation in the doc. While running the permissions adding script I ran into a couple of difficulties which I've tried to fix in: * tektoncd/plumbing#1062 * tektoncd/plumbing#1061
bobcatfish
added a commit
to bobcatfish/community
that referenced
this pull request
Apr 11, 2022
1. Finishing up updates from governing board election (andrew bayer no longer on the board, priya wadhwa joining the board) 2. @jerop will be taking my place on the governing board while I am on leave (see [leave policy](https://github.com/tektoncd/community/blob/main/governance.md#governing-board-leave-policy)) (thanks @jerop !!) Went through the list at https://github.com/tektoncd/community/blob/main/governance.md#changes-to-governing-board to make sure jerop and priya have all the governing board permissions and to make the changes required with andrew leaving the board. Also added Priti to the governing board + community meeting faciltator rotation because she is listed in the rotation in the doc. While running the permissions adding script I ran into a couple of difficulties which I've tried to fix in: * tektoncd/plumbing#1062 * tektoncd/plumbing#1061
bobcatfish
added a commit
to bobcatfish/community
that referenced
this pull request
Apr 11, 2022
1. Finishing up updates from governing board election (andrew bayer no longer on the board, priya wadhwa joining the board) 2. @jerop will be taking my place on the governing board while I am on leave (see [leave policy](https://github.com/tektoncd/community/blob/main/governance.md#governing-board-leave-policy)) (thanks @jerop !!) Went through the list at https://github.com/tektoncd/community/blob/main/governance.md#changes-to-governing-board to make sure jerop and priya have all the governing board permissions and to make the changes required with andrew leaving the board. Also added Priti to the governing board + community meeting faciltator rotation because she is listed in the rotation in the doc. While running the permissions adding script I ran into a couple of difficulties which I've tried to fix in: * tektoncd/plumbing#1062 * tektoncd/plumbing#1061
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
tekton-robot
pushed a commit
to tektoncd/community
that referenced
this pull request
Apr 12, 2022
1. Finishing up updates from governing board election (andrew bayer no longer on the board, priya wadhwa joining the board) 2. @jerop will be taking my place on the governing board while I am on leave (see [leave policy](https://github.com/tektoncd/community/blob/main/governance.md#governing-board-leave-policy)) (thanks @jerop !!) Went through the list at https://github.com/tektoncd/community/blob/main/governance.md#changes-to-governing-board to make sure jerop and priya have all the governing board permissions and to make the changes required with andrew leaving the board. Also added Priti to the governing board + community meeting faciltator rotation because she is listed in the rotation in the doc. While running the permissions adding script I ran into a couple of difficulties which I've tried to fix in: * tektoncd/plumbing#1062 * tektoncd/plumbing#1061
|
/lgtm |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
#926 added a
--readonlyflagfor adding readonly user permissions, however it added it with defualt
"true" which means that by default when running this script to add or
remove permissions, it will only modify the read only permissions and
will leave any write permissions as-is.
This means that when I went to update the governing board permissions -
i.e. adding permissions for new members and removing permissions from
old members, the script did very little and I couldn't figure out why.
This commit switches the default to the writer roles - defaulting to
reader is probably a safer default, however I think it's a confusing
change given how the script has been used so far. Can be convinced that
instead we should keep the default and add better documentation and
examples (but someone else will probably need to pick that up!)
When removing permissions, if the permission to be removed isn't
present, the script will stop. However, if the permission isn't there,
imo that is okay and if the permissions list changes at all, this error
could get it - imo it's better to keep running the script and just
report any errors that couldn't be removed.
I ran into this especially when I ran the remove script but only for
readonly mode - from that point on I could no longer run the script for
the broader permissions b/c it would encounter an error immediately
since the viewer role had already been removed.
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.