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

Merge - Go team can make PRs to this repo #30

Closed
wants to merge 5 commits into from

Conversation

willscott
Copy link
Contributor

Those with Merge - Go should be able to propose PRs changing permissions in this repo without needing to create personal forks

Those with `Merge - Go` should be able to propose PRs changing permissions in this repo without needing to create personal forks
@willscott willscott requested a review from a team as a code owner July 29, 2022 14:26
@github-actions
Copy link
Contributor

Before merge, verify that all the following plans are correct. They will be applied as-is after the merge.

Terraform plans

ipfs

Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # github_team_repository.this["Merge - Go:github-mgmt"] will be created
  + resource "github_team_repository" "this" {
      + etag       = (known after apply)
      + id         = (known after apply)
      + permission = "push"
      + repository = "github-mgmt"
      + team_id    = "3364102"
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Copy link
Contributor

@galargh galargh left a comment

Choose a reason for hiding this comment

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

While I agree it would make for a smoother process I don't think we should broaden the permissions here that much. That's because push access comes not only with the ability to create new branches within the repo but also, among others, with the ability to merge PRs. So with push access you can both propose and apply changes.

However, I think this idea is definitely worth exploring. I think there might be a way to restrict the set of "mergable" PRs to only those reviewed by CODEOWNERs. I created ipdxco/github-as-code#58 to explore it further but it'll require more time to investigate (more details in the linked issue).

@willscott
Copy link
Contributor Author

making it so that only stewards can change permissions in a reasonable way means none of you will directly feel the pain of the extra workflow you're asking for from the rest of the org.

Tighten that permission so that it becomes a pain point. otherwise this is 'it's good enough for us' and won't get fixed.

@galargh
Copy link
Contributor

galargh commented Aug 1, 2022

That's a good idea but I'm afraid it could unintentionally degrade the experience for the rest of the org too since it would mean there are less people that are able to review the proposed changes.

I can definitely start going through fork-based process myself from now on so that I experience how it is.

@willscott
Copy link
Contributor Author

note also thought that the lack of permission means that I can't add reviewers / labels to my PRs, so can't notify relevant people on proposed changes.

willscott added a commit to willscott/github-mgmt-1 that referenced this pull request Aug 1, 2022
Have PRs, but can't tag reviewers.

Would make this as a PR I can tag reviewers against in this repo as requested by stewards, but can't per ipfs#30
@galargh
Copy link
Contributor

galargh commented Aug 1, 2022

note also thought that the lack of permission means that I can't add reviewers / labels to my PRs, so can't notify relevant people on proposed changes.

One thing I'm exploring (with ipdx team for now) to make this part less annoying is setting up CODEOWNERS and setting up participating teams so that their members are auto-assigned to review the PRs.

I know that's not as powerful as being able to ping anyone but I think it makes it at least a little bit better.

@galargh
Copy link
Contributor

galargh commented Aug 4, 2022

Can all the org members have push access to github-mgmt?

I spent some more time exploring the possibility of giving push access to all org members (ipdxco/github-as-code#58). I'm afraid we won't be able to accomplish that because if one has push access to a repository, they can also access repository secrets which, in case of github-mgmt, is as powerful as having org admin permissions.

Having said that, giving push access either to the w3dt-stewards team only or to all the org members are not the only two options.

How did access to github-mgmt change over time?

To begin, let me go through how access levels changed for github-mgmt thus far and why.

  1. We started with only me as a github-mgmt admin because I was the one setting it up.
  2. I gave @ipfs/ipdx maintain access because my other team mate was closest to the GitHub Management project so he knew it the most.
  3. Finally, I also gave w3dt-stewards maintain access because we starter receiving PRs from people other than myself and I didn't want to change requester to have to wait for us to wake up/come back from vacations to get their change requests merged. Stewards were chosen because they had to listen to me going on about GitHub Management during standups and their work often spans multiple initiatives.

Who should have access to github-mgmt?

In my mind, it'd be ideal if the group of users that have push access (and above) to github-mgmt was:

  • familiar with GitHub Management
  • actively involved in triaging/reviewing PRs (at least for their team)
  • trusted as much as org admins are

I don't think that's completely true about the current group of github-mgmt collaborators just yet but that's what we should strive for and we shouldn't shy away from revoking access when it makes sense either.

About Merge - Go, firstly, I'm worried about the name because it doesn't convey the extent of permissions it comes with if it were added as a github-mgmt collaborator. Also, if we were to add it all at once, I think we would be diverging further from some of the criteria I mentioned.

How can we get to the desired group of github-mgmt guardians?

I do have an idea on how we could proceed. What do you think about this?

  1. We create a new team called org-admins with push access to github-mgmt
  2. We decrease w3dt-stewards access level to github-mgmt from maintain to push
  3. We gradually grow org-admins team with users fitting criteria (totally up for discussion what those should be exactly; e.g. familiarity, involvement, trust)
  4. Once org-admins team reaches a good(?) size, we revoke w3dt-stewards access

This is what I have in mind - #37

It might be the case that we end up with all the w3dt-stewards and all the Merge - Go members in this new org-admins team but I think there's value in getting there step by step.

What's my process for introducing changes to github-mgmt?

  1. I go to https://github.com/ipfs/github-mgmt/blob/master/github/ipfs.yml
  2. I hit . which opens up VS Code in the browser.
  3. I hit CMD + SHIFT + P which opens up command prompt, type in Fold All and hit Enter.
  4. Now I introduce my changes and stage them.
  5. Here, those with access have to create a new branch and commit and those without have to hit commit and confirm new branch name (GitHub will automatically create a fork if it doesn't exist yet and prompt for a branch name).
  6. Finally, I create a PR (still in VS Code).

Starting on the master branch of the original repo means I don't have to think about syncing. The only thing I'm missing when working with the repo this way is YAML Schema Server which is not running in the web VS Code.

I know that's not going to work for all but I just wanted to share how I do it for inspiration.

Closing Notes

FYI, I'm yet to look closer into triage access (#33) and if that's something we can be more liberal with. I probably won't get to it before my vacations though (08.08-08.22).

Finally, I want to reiterate that I'm very grateful for the feedback. We do have to be considerate and careful about striking the right balance of UX and security.

@willscott
Copy link
Contributor Author

  • A new group as you propose in Create github-mgmt stewards team with access to github-mgmt #37 seems fine.
  • I do find the need to fork quite suboptimal in terms of my personal workflow: while github makes the fork, it has side effects I am uncomfortable with:
    • it sends a notification of a new repo to my followers
    • It routes emails for notifications on the fork to my github-default personal email rather than my org-default work email

I wonder if there is a more complex two-repo workflow where there's a normal repo with branch protection but without CI secrets, and then a second repo that has secrets and a workflow triggered on releases or updates to the protected branch of the first and applies changes to the org?

@galargh
Copy link
Contributor

galargh commented Aug 5, 2022

A new group as you propose in #37 seems fine.

Cool :) I'll mark it as ready for review and add reviewers today.

I wonder if there is a more complex two-repo workflow where there's a normal repo with branch protection but without CI secrets, and then a second repo that has secrets and a workflow triggered on releases or updates to the protected branch of the first and applies changes to the org?

That's an interesting idea! We haven't explored it in the past. Potentially, we could only have read-only creds in the first repo and read-write ones in the other one. We can try to sketch it out and think of the implications after I'm back from vacations. Whether we can implement it in the most immediate future is another question though because I'd imagine that this would require a pretty big refactor - let's evaluate this once there are more details. We're going to track this in ipdxco/github-as-code#59

github-actions bot pushed a commit that referenced this pull request Sep 12, 2022
@galargh
Copy link
Contributor

galargh commented Nov 14, 2022

I'm going to close this issue for now. We have ipdxco/github-as-code#59 to capture exploration work regarding splitting github management into 2 repos. Feel free to reopen if needed 🙇

@galargh galargh closed this Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants