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

Add repo roles: #611

Merged
merged 1 commit into from
Apr 28, 2022
Merged

Add repo roles: #611

merged 1 commit into from
Apr 28, 2022

Conversation

jacobweinstock
Copy link
Member

@jacobweinstock jacobweinstock commented Apr 28, 2022

Description

This is needed to satisfy the repo roles defined by the community here: tinkerbell/org#10

Why is this needed

Fixes: #

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@jacobweinstock jacobweinstock requested a review from mmlb April 28, 2022 19:19
@jacobweinstock jacobweinstock force-pushed the role-bootstrap branch 2 times, most recently from 0afceb2 to 34833fb Compare April 28, 2022 19:24
@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #611 (687f652) into main (59b0126) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #611   +/-   ##
=======================================
  Coverage   44.39%   44.39%           
=======================================
  Files          61       61           
  Lines        3489     3489           
=======================================
  Hits         1549     1549           
  Misses       1857     1857           
  Partials       83       83           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59b0126...687f652. Read the comment docs.

Comment on lines 3 to 22
collaborators:
# Maintainers
- username: mmlb
permission: maintain
- username: micahhausler
permission: maintain
# Approvers
- username: jacobweinstock
permission: push
- username: tstromberg
permission: push
- username: displague
permission: push
- username: nshalman
permission: push
# Reviewers
- username: tobert
permission: triage
- username: stephen-fox
permission: triage
- username: chrisdoherty4
permission: triage
- username: detiber
permission: triage
Copy link
Contributor

@mmlb mmlb Apr 28, 2022

Choose a reason for hiding this comment

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

What do you think about this form?

collaborators:
  # Maintainers
  - { permission: maintain, username: micahhausler }
  - { permission: maintain, username: mmlb }
  # Approvers
  - { permission: push, username: displague }
  - { permission: push, username: jacobweinstock }
  - { permission: push, username: nshalman }
  - { permission: push, username: tstromberg }
  # Reviewers
  - { permission: triage, username: chrisdoherty4 }
  - { permission: triage, username: detiber }
  - { permission: triage, username: stephen-fox }
  - { permission: triage, username: tobert }

I like it because the groups are more obvious this way.

Copy link
Member

Choose a reason for hiding this comment

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

There are some readability perks, but I think they are marginal (🥁 ).

I'm fine with the original (boring) yaml list, fwiw.

Copy link
Contributor

Choose a reason for hiding this comment

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

no one ever got fired for picking vanilla :) Lets just go with the original.

mmlb
mmlb previously approved these changes Apr 28, 2022
displague
displague previously approved these changes Apr 28, 2022
micahhausler
micahhausler previously approved these changes Apr 28, 2022
Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

all users LGTM, one question about ordering

Comment on lines 5 to 6
- username: mmlb
permission: maintain
- username: micahhausler
permission: maintain
Copy link
Contributor

Choose a reason for hiding this comment

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

Serious question, do we want to alphabetize by GitHub username?

Copy link
Contributor

@mmlb mmlb Apr 28, 2022

Choose a reason for hiding this comment

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

heh actually alphabetizing is why I looked at the one-line-per to begin with 😄

# The permission to grant the collaborator. Can be one of:
# * `pull` - can pull, but not push to or administer this repository.
# * `push` - can pull and push, but not administer this repository.
# * `admin` - can pull, push and administer this repository.
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a note about CODEOWNERS file importance / role in the admin setting.

micahhausler
micahhausler previously approved these changes Apr 28, 2022
Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

LGTM

mmlb
mmlb previously approved these changes Apr 28, 2022
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Apr 28, 2022
@@ -0,0 +1,2 @@
/.github/settings.yml @mmlb @micahhausler
/.github/CODEOWNERS @mmlb @micahhausler
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is /.github/workflows/CODEOWNERS, should it be /.github/CODEOWNERS

Copy link
Member Author

Choose a reason for hiding this comment

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

ah good catch

This is needed to enable the repo roles defined by
the community here: tinkerbell/org#10

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>
Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

LGTM

@mmlb mmlb removed the request for review from displague April 28, 2022 20:47
@mergify mergify bot merged commit d56d3e9 into tinkerbell:main Apr 28, 2022
@jacobweinstock jacobweinstock deleted the role-bootstrap branch April 28, 2022 20:48
@displague displague added this to the 0.7.0 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants