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 nested owners files to automate labeling pull requests #17979

Merged
merged 7 commits into from
May 12, 2024

Conversation

jmhbnz
Copy link
Member

@jmhbnz jmhbnz commented May 10, 2024

This pull request proposes adding an initial set of nested OWNERS files with labels: populated in order to automate labeling for pull requests now that we have enabled the owners-label prow plugin in kubernetes/test-infra#32589.

I've structured the OWNERS files with the assumption that inheritance works, we need to test/verify this. For example, a pull request for tools/rw-heatmaps should in theory be allocated both the area/tooling and area/performance labels based on the inheritance.

Moving forward we can also use the nested OWNERS files to further delegate approvers and reviewers for specific directories of etcd-io/etcd code.

Fixes: #17970

@jmhbnz jmhbnz requested a review from serathius May 10, 2024 11:44
@jmhbnz jmhbnz changed the title Add owner file labels Add nested owners files to automate labeling pull requests May 10, 2024
# See the OWNERS docs at https://go.k8s.io/owners

labels:
- area/documentation
Copy link
Member

Choose a reason for hiding this comment

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

The documentation directory is no longer used in the original way. Originally it included https://github.com/etcd-io/website. Now the remaining documentation is more of contributor documentation, using "area/documentation" label for this directory might be misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that directory is rather awkward. I think we have a larger decision to make on if we will shift to serving our contributor facing documentation to a new area of etcd-io/website.

To be transparent I would be in favor of this. It's something k8s has been doing for a while https://kubernetes.io/docs/contribute/docs.

I don't want to force that decision in this pr though, happy to come back to it later, perhaps as a topic for one of our upcoming community calls.

Copy link
Member

@serathius serathius May 10, 2024

Choose a reason for hiding this comment

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

Yes that directory is rather awkward. I think we have a larger decision to make on if we will shift to serving our contributor facing documentation to a new area of etcd-io/website.

eee, I was pushing for a reverse direction. etcd-io/website#569

The contributors documentation was moved with website, however I reversed the decision because it immediately became unmaintained and outdated. I don't like K8s direction, I think the contributor documentation should be in the same repo as project and be unit tested to ensure it's up to date.

Example of documentation testing in python https://docs.python.org/3/library/doctest.html#simple-usage-checking-examples-in-docstrings

Copy link
Member Author

@jmhbnz jmhbnz May 10, 2024

Choose a reason for hiding this comment

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

Based on when the linked pr was merged I would guess the unmaintained issue was a reflection of the state the project was in at that point in time around 2022? We are in a better position now in my view with more contributors available for website maintenance.

I like the idea of having better testing for documentation, and actually this isn't limited to contributor docs, I think many of the docs pages for our user facing documentation should have automated testing to ensure they are correct. In the ~2 years since that pr you linked was merged I don't believe we added unit tests for any of the files under our Documentation/ directory. However if we want to do this there is no reason we can't have scheduled prow job for etcd-io/website to let us know if something is broken.

Let's shift this discussion to a community meeting sometime and properly explore some pros/cons. At the end of the day I want our documentation to be the best it can be, irrespective of which git repo it is stored in.

Copy link
Contributor

@liangyuanpeng liangyuanpeng left a comment

Choose a reason for hiding this comment

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

LGTM

@ivanvc
Copy link
Member

ivanvc commented May 10, 2024

/retest

@jmhbnz jmhbnz requested a review from ahrtr May 10, 2024 20:39
tools/etcd-dump-logs/OWNERS Outdated Show resolved Hide resolved
Copy link
Contributor

@liangyuanpeng liangyuanpeng left a comment

Choose a reason for hiding this comment

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

how about add an OWNERs file for .github/workflows, and label can be github_actions,it is also using by dependabot .

Signed-off-by: James Blair <mail@jamesblair.net>
Signed-off-by: James Blair <mail@jamesblair.net>
Signed-off-by: James Blair <mail@jamesblair.net>
Signed-off-by: James Blair <mail@jamesblair.net>
Signed-off-by: James Blair <mail@jamesblair.net>
Signed-off-by: James Blair <mail@jamesblair.net>
Signed-off-by: James Blair <mail@jamesblair.net>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks

@jmhbnz jmhbnz merged commit 333bd7b into etcd-io:main May 12, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Auto label PRs on Github
5 participants