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

Level up markdown content in all repos in this organization #8

Closed
dblock opened this issue May 20, 2021 · 22 comments
Closed

Level up markdown content in all repos in this organization #8

dblock opened this issue May 20, 2021 · 22 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@dblock
Copy link
Member

dblock commented May 20, 2021

Problem: We say different things in different repositories for items, such as code of conduct, for no good reason.

https://github.com/opensearch-project/job-scheduler

Screen Shot 2021-05-20 at 11 27 16 AM

https://github.com/opensearch-project/common-utils

Screen Shot 2021-05-20 at 11 28 00 AM

etc.

Let's level up READMEs across the org to at least include standard things.

Feel free to take freedoms in these files that are specific to your projects where it makes sense. As a rule of thumb, make links for common things across the org into the .github repo, and add the custom things that are only relevant for your project in your own .md's. When in doubt, consider reducing the amount of mental overhead contributors have to have when making PRs across multiple repos in our org.

Example: opensearch-project/common-utils#32

@ahopp
Copy link

ahopp commented May 25, 2021

@dblock personally, I like the second version. Not only does the latter provide all the available context but it provide a direct path to resolution (linked email) which is hidden on the bottom of the page via the ingress click on the former.

@dblock
Copy link
Member Author

dblock commented May 25, 2021

@dblock personally, I like the second version. Not only does the latter provide all the available context but it provide a direct path to resolution (linked email) which is hidden on the bottom of the page via the ingress click on the former.

Want to take on this issue @ahopp ?

@dblock dblock self-assigned this Jun 10, 2021
@dblock
Copy link
Member Author

dblock commented Jun 22, 2021

I PRed opensearch-project/common-utils#32 as a sample of what we would like each repo to look like.

@dblock dblock changed the title Add README with standard sections such as CoC Standardize README, ADMINS, MAINTAINERS, CODE_OF_CONDUCT, etc. across all repos in this organization Jun 23, 2021
@dblock dblock changed the title Standardize README, ADMINS, MAINTAINERS, CODE_OF_CONDUCT, etc. across all repos in this organization Level up README, ADMINS, MAINTAINERS, CODE_OF_CONDUCT, etc. across all repos in this organization Jun 23, 2021
@dblock dblock changed the title Level up README, ADMINS, MAINTAINERS, CODE_OF_CONDUCT, etc. across all repos in this organization Level up markdown content in all repos in this organization Jun 23, 2021
@cliu123
Copy link
Member

cliu123 commented Jun 23, 2021

@dblock Is the MAINTAINERS.md really necessary? Plugins have to make a PR to change the file everytime when people leave the team or new people join. How does this file help us?

@cliu123
Copy link
Member

cliu123 commented Jun 23, 2021

@dblock MAINTAINERS.md and ADMINS.md look exactly same?

@cliu123
Copy link
Member

cliu123 commented Jun 23, 2021

@dblock Could SECURITY.md and RELEASING.md be sections in README.md? Is it necessary to have separate files for them?

@dblock
Copy link
Member Author

dblock commented Jun 23, 2021

@dblock MAINTAINERS.md and ADMINS.md look exactly same?

How are they the same? The people are different and the responsibilities are not the same.

@joshuali925
Copy link
Member

This issue says to

Cleanup TESTING.md if you have one.

In plugin repo issue opensearch-project/observability#58 there is TESTING.md checkbox. What's the expectation? @dblock

@dblock
Copy link
Member Author

dblock commented Jun 23, 2021

@dblock Could SECURITY.md and RELEASING.md be sections in README.md? Is it necessary to have separate files for them?

SECURITY is needed for GitHub to create automatic security links such as when opening issues, so no.

I expect RELEASING to become quite different across projects. The release dance may be quite different between a plugin that needs, for example, testing for compatibility with other plugins, or plugins that have native bits, etc.

@dblock
Copy link
Member Author

dblock commented Jun 23, 2021

This issue says to

Cleanup TESTING.md if you have one.

In plugin repo issue opensearch-project/trace-analytics#58 there is TESTING.md checkbox. What's the expectation? @dblock

That's where you'd put how and what kind of testing is done. It's entirely optional. The OpenSearch TESTING is quite thorough if you're looking for ideas: https://github.com/opensearch-project/OpenSearch/blob/main/TESTING.md

@dblock
Copy link
Member Author

dblock commented Jun 23, 2021

@dblock Is the MAINTAINERS.md really necessary? Plugins have to make a PR to change the file everytime when people leave the team or new people join. How does this file help us?

The MAINTAINERS.md doc choice precedes me. That said, I think not all people on your team should be maintainers, and leaving the team shouldn't remove you from being a maintainer. This helps the community see that the project is actively maintained by actual people, and not some dark force.

@dblock
Copy link
Member Author

dblock commented Jun 23, 2021

@dblock for repos with 2 plugins(dashboards and opensearch plugins), such as reporting, notification, notebook, etc. Is there a recommened way to organized markdown docs? I think we'll keep the top level docs, but do we need to have README or other docs(code of conduct, dev guide, liscense) inside each plugin folder?

Generally the practice is to put those docs in each repo because repos get forked and need to retain their license, copyright, etc. Furthermore, some docs, like license and COC, are needed for GitHub to notice and use in messaging, you can't blanket-apply them at org level. We also have repos, such as project-website under a different license, for example. Then, the dev guide is radically different for a Ruby plugin vs. a Java service.

I tried to not dup too much content and create files with links where appropriate, but we also have some amazon OSS-wide standards for including COC entirely that we monitor.

@cliu123
Copy link
Member

cliu123 commented Jun 23, 2021

@dblock MAINTAINERS.md and ADMINS.md look exactly same?

@dblock In the issue description, the links for both MAINTAINERS.md and ADMINS.md link to MAINTAINERS.md
But the other question is: Is it really necessary to have both of the files? MAINTAINERS.md makes perfect sense.
And who is the admin of security plugin. Is that Henri Yandell too?

@joshuali925
Copy link
Member

That's where you'd put how and what kind of testing is done. It's entirely optional. The OpenSearch TESTING is quite thorough if you're looking for ideas: https://github.com/opensearch-project/OpenSearch/blob/main/TESTING.md

I see, thanks. How do we know if an item in the checklist is optional?


I have the question on ADMINS.md and MAINTAINERS.md as well. From the documents, ADMINS.md contains people who have "admin" rights in the repo, and MAINTAINERS.md contains people who have "maintain" rights in the repo.

Maintainers are active and visible members of the community, and have maintain-level permissions on a repository. Use those privileges to serve the community and evolve code as follows.

The previous decision of MAINTAINERS.md was to include people who has write access and will contribute, rather than people who have "maintain" rights (reference opensearch-project/dashboards-visualizations#1 (comment)). Most of the plugin owners only have "write" access to the repo and cannot change settings like repo secrets. Are we updating MAINTAINERS.md to be people who have maintain-level permissions then? Or is the statement not accurate?

Also in ADMINS.md it says

If you're interested in becoming a maintainer, see MAINTAINERS

I checked but couldn't find how external contributors can become maintainers in MAINTAINERS.md?

@dblock
Copy link
Member Author

dblock commented Jun 24, 2021

@dblock MAINTAINERS.md and ADMINS.md look exactly same?

@dblock In the issue description, the links for both MAINTAINERS.md and ADMINS.md link to MAINTAINERS.md

That was obviously a typo, fixed.

But the other question is: Is it really necessary to have both of the files? MAINTAINERS.md makes perfect sense.
And who is the admin of security plugin. Is that Henri Yandell too?

Admins and maintainers are definitely different people, especially for current repositories in this org, and have different responsibilities. Admins maps directly to GitHub users with admin permissions, which most maintainers don't have. Furthermore, we are going to have admins for repos that don't belong to AMZN in this org (see opensearch-project/opensearch-plugin-template-java#4). In terms of who is the admin now for what repo - @hyandell works for OSPO and is definitely one, but let's ask @CEHENKLE whether there are other admins (I can't see)?

@dblock
Copy link
Member Author

dblock commented Jun 24, 2021

That's where you'd put how and what kind of testing is done. It's entirely optional. The OpenSearch TESTING is quite thorough if you're looking for ideas: https://github.com/opensearch-project/OpenSearch/blob/main/TESTING.md

I see, thanks. How do we know if an item in the checklist is optional?

We don't ;) But do use common sense, and push back on anything that doesn't. For TESTING it says "Cleanup TESTING.md if you have one".

I have the question on ADMINS.md and MAINTAINERS.md as well. From the documents, ADMINS.md contains people who have "admin" rights in the repo, and MAINTAINERS.md contains people who have "maintain" rights in the repo.

Maintainers are active and visible members of the community, and have maintain-level permissions on a repository. Use those privileges to serve the community and evolve code as follows.

The previous decision of MAINTAINERS.md was to include people who has write access and will contribute, rather than people who have "maintain" rights (reference opensearch-project/dashboards-visualizations#1 (comment)). Most of the plugin owners only have "write" access to the repo and cannot change settings like repo secrets. Are we updating MAINTAINERS.md to be people who have maintain-level permissions then? Or is the statement not accurate?

The statement is correct. Generally everything in the .github repo is "the current state of truth", but we will continue evolving the truth (open issues, question existing text, etc.). I worked through these trying to simplify our story, anticipating that adding/removing everyone with write access will be a lot of busy work (as pointed out by someone on this thread). You can easily see repo contributors on GitHub, so we don't need to list them in a file IMO. Let's put true maintainers that have maintain rights in maintainers.

Also in ADMINS.md it says

If you're interested in becoming a maintainer, see MAINTAINERS

I checked but couldn't find how external contributors can become maintainers in MAINTAINERS.md?

Currently we don't have a process for this, but we will want to figure it out sooner than later.

@ohltyler
Copy link
Member

Still trying to figure out who should go in ADMINS.md for each of the plugins. For example, I don't see any admin privileges in the anomaly-detection team, or other teams for that matter.

@dblock
Copy link
Member Author

dblock commented Jun 24, 2021

Still trying to figure out who should go in ADMINS.md for each of the plugins. For example, I don't see any admin privileges in the anomaly-detection team, or other teams for that matter.

@CEHENKLE said she'll take this

@joshuali925
Copy link
Member

@dblock Got it, thanks for the clarification.

You can easily see repo contributors on GitHub, so we don't need to list them in a file IMO. Let's put true maintainers that have maintain rights in maintainers.

I would appreciate an explicit callout somewhere so that we are aware the maintainers definition has changed.

The sample MAINTAINERS.md linked in this issue seems to be inconsistent with the maintainers definition. Should the MAINTAINERS.md in common-utils be updated then? (e.g. Sarat has maintain rights according to members page but he is not in the list)

@dblock
Copy link
Member Author

dblock commented Jun 24, 2021

@dblock Got it, thanks for the clarification.

You can easily see repo contributors on GitHub, so we don't need to list them in a file IMO. Let's put true maintainers that have maintain rights in maintainers.

I would appreciate an explicit callout somewhere so that we are aware the maintainers definition has changed.

I didn't realize I was introducing a major change. We didn't have a maintainers definition published on GitHub, so I published one using the best of my knowledge. Not sure how we could have done this better.

The sample MAINTAINERS.md linked in this issue seems to be inconsistent with the maintainers definition. Should the MAINTAINERS.md in common-utils be updated then? (e.g. Sarat has maintain rights according to members page but he is not in the list)

Just because Sarat has maintainer GitHub access doesn't make him a maintainer. The reverse is true, a maintainer has maintainer GitHub access.

@dblock
Copy link
Member Author

dblock commented Apr 18, 2022

This was mostly done and .github templates are up-to-date. Closing, leaving to individual projects to keep this up.

@dblock dblock closed this as completed Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants