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

chore: use component_owners #446

Merged
merged 2 commits into from
Jul 12, 2023
Merged

chore: use component_owners #446

merged 2 commits into from
Jul 12, 2023

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Jul 5, 2023

Adds component_owners for all components

@toddbaert toddbaert requested a review from a team as a code owner July 5, 2023 18:44
@toddbaert toddbaert changed the title Chore/update codeowers chore: update CODEOWNERS Jul 5, 2023
@beeme1mr
Copy link
Member

beeme1mr commented Jul 5, 2023

The challenge with using a code owners file is that everyone listed needs write access to the repo. @dyladan created a tool that works around that limitation that we may want to consider.

@toddbaert
Copy link
Member Author

toddbaert commented Jul 5, 2023

The challenge with using a code owners file is that everyone listed needs write access to the repo. @dyladan created a tool that works around that limitation that we may want to consider.

I've simply done this: open-feature/community#221

This extracts the contrib roles from the sdk roles. With the CODEOWNERS in place, this should cover our use cases. CODEOWNERS will be able to approve changes to their own modules and merge them, but not release or make wider changes without SDK maintainer approval. We'll need to add @Mateoc and @sago2k8 to the contribs maintainers to make this all work.

@dyladan
Copy link

dyladan commented Jul 5, 2023

The challenge with using a code owners file is that everyone listed needs write access to the repo. @dyladan created a tool that works around that limitation that we may want to consider.

I've simply done this: open-feature/community#221

This extracts the contrib roles from the sdk roles. With the CODEOWNERS in place, this should cover our use cases. CODEOWNERS will be able to approve changes to their own modules and merge them, but not release or make wider changes without SDK maintainer approval.

You're both right. The issue is that the contrib group can make changes to anything in the contrib repo or merge any PR that satisfies merge checks. If you're ok with that risk then I would recommend using the built in solution as long as it works for you. In otel we have many components with many different owners, some of whom are only occasionally present in the repo or are new to the repo so we wanted to be stricter with controlling those permissions.

@toddbaert
Copy link
Member Author

The challenge with using a code owners file is that everyone listed needs write access to the repo. @dyladan created a tool that works around that limitation that we may want to consider.

I've simply done this: open-feature/community#221
This extracts the contrib roles from the sdk roles. With the CODEOWNERS in place, this should cover our use cases. CODEOWNERS will be able to approve changes to their own modules and merge them, but not release or make wider changes without SDK maintainer approval.

You're both right. The issue is that the contrib group can make changes to anything in the contrib repo or merge any PR that satisfies merge checks. If you're ok with that risk then I would recommend using the built in solution as long as it works for you. In otel we have many components with many different owners, some of whom are only occasionally present in the repo or are new to the repo so we wanted to be stricter with controlling those permissions.

I'd be open to more robust tooling if it's not a lot of overhead. This is just what we'd already talked about. I guess the risk would be a "low trust" CODEOWNER pushing/approving changes that are not carefully reviewed when a maintainer later releases them?

@toddbaert
Copy link
Member Author

I'll mark as draft for now.

@toddbaert toddbaert marked this pull request as draft July 5, 2023 19:41
@dyladan
Copy link

dyladan commented Jul 5, 2023

There are several potential risks of varying severity and likelihood, but most can be mitigated. The issue is that as the repo grows you have tens or hundreds of people with write permissions. Even if you've used branch protection rules and merge checks to lock things down as much as possible you may still run into issues like code owners merging PRs for components they don't own, closing issues that aren't resolved, et cetera. The more people have write permissions, the more likely it is something like this will happen even on accident.

@toddbaert
Copy link
Member Author

There are several potential risks of varying severity and likelihood, but most can be mitigated. The issue is that as the repo grows you have tens or hundreds of people with write permissions. Even if you've used branch protection rules and merge checks to lock things down as much as possible you may still run into issues like code owners merging PRs for components they don't own, closing issues that aren't resolved, et cetera. The more people have write permissions, the more likely it is something like this will happen even on accident.

Ok I'll look at the tooling you're referring to... Assuming I can find it in the otel js contribs?

@beeme1mr
Copy link
Member

beeme1mr commented Jul 5, 2023

@toddbaert toddbaert changed the title chore: update CODEOWNERS chore: use component_owners Jul 10, 2023
@toddbaert toddbaert marked this pull request as ready for review July 10, 2023 17:04
@toddbaert
Copy link
Member Author

toddbaert commented Jul 10, 2023

@dyladan @beeme1mr the TC has agreed to give this a go! Thanks Dan!

Please re-review.

.github/workflows/component-owners.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@sago2k8 sago2k8 left a comment

Choose a reason for hiding this comment

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

LGTM, I'm ok being pinned for maintaining the provider.

thank you

Comment on lines +5 to +8
permissions:
contents: read # to read changed files
issues: read|write # to read/write issue assignees
pull-requests: read|write # to read/write PR reviewers
Copy link
Member Author

@toddbaert toddbaert Jul 11, 2023

Choose a reason for hiding this comment

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

@kinyoklion I've added the permissions as you suggested, good idea.

@dyladan I looked through the source and cross-referenced the REST calls I saw there with the GH doc, and came up with the permissions above. If this looks good to you, and it works, I can open a PR to document this in your sample workflow.

EDIT: read|write is invalid. I changed this to write, which implies read. After that this worked well.

@toddbaert toddbaert requested a review from dyladan July 11, 2023 15:03
@toddbaert toddbaert merged commit 15c6c51 into main Jul 12, 2023
5 checks passed
@toddbaert toddbaert deleted the chore/update-codeowers branch July 12, 2023 12:52
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.

7 participants