search

opensearch-project / .github

Provides templates and resources for other OpenSearch project repositories.
Apache License 2.0
29 stars 71 forks source link

Document code reviews best practices in MAINTAINERS #32

Open dblock opened 3 years ago

dblock commented 3 years ago

Add to MAINTAINERS

AmanKumar6603 commented 11 months ago

Hi @dblock I want to work on this issue, or support on this issue if you are working on it. Please assign me this issue. Also let me know what to do.

dblock commented 11 months ago

Thanks @AmanKumar6603, I am not working on this.

AmanKumar6603 commented 10 months ago

Hi @dblock , can you tell me what to do, as per my understanding, I have to add a document for code review to the Maintainer. Is that right?

dblock commented 10 months ago

I think so. I don't have a better answer to what to do, sorry. I'd like the additions to MAINTAINERS.md to have answers to the above questions and that maintainers in the 100+ repos in this org can agree to.

AmanKumar6603 commented 10 months ago

Whom I should approach for clear understanding over it. @dblock

dblock commented 10 months ago

@AmanKumar6603 Are you asking for an SME on these items? If you're not a subject matter expert on them yourself, consider picking up another work-item.

AmanKumar6603 commented 10 months ago

@dblock No I am saying that where I have to add these changes, in maintainer? As per my understanding I have to write down all answers to the above mentioned question in (https://github.com/opensearch-project/.github/blob/main/MAINTAINERS.md)

dblock commented 10 months ago

Yes. Screenshot 2023-11-08 at 10 46 35 AM

jkowall commented 1 week ago

I'll take this one please, happy to write it up into a PR.

I would also suggest on the security side to use GitHub CodeQL and other free capabilities to do code scanning. Within the CNCF we also use other tools to analyze security and viability: https://clomonitor.io and https://securityscorecards.dev/ I implemented both on Jaeger if interested.

jkowall commented 6 days ago

Was going to work on this, and I don't believe the answers to these questions belong in MAINTAINERS.md but instead in CONTRIBUTING.md you might want a second file for CONTRIBUTING_GUIDELINES.md. Here is how we handle this in Jaeger : https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md and https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING.md

I can incorporate parts of these ideas into the new files for OpenSearch, however I am not as tuned into how some of this works for the project.

If you are okay with me taking a crack at both I can, otherwise let me know. I would also add answers to other questions aside from the list above which is a good start.

Thanks @dblock !

dblock commented 5 days ago

At first, I thought that the only thing I don't love about CONTRIBUTING_GUIDELINES is that it's a very long name ;)

In general we adopted DEVELOPER_GUIDE in the repos in OpenSearch which I think are similar to your CONTRIBUTING.md (example), so probably what you want in guidelines actually does belong in our style contributing (general purpose contributing)?

I am also open to other ideas like a REVIEWING.md and whatever else you want to PR. Maybe start with the non-controversial parts and we can figure out the rest in subsequent PRs.

jkowall commented 4 days ago

So these questions in your initial issue:

  • What is the commit process from the maintainer POV? (e.g. puck-up any of the open PRs, is there an SLA?) I think we need to discuss this in the technical steering committee if we want an SLA. I can tell you we try to answer in 24h, but we also ask people to ping us on Slack if we are not meeting the SLA. We also use Dosu to mark things stale.

  • What's specific about PRs in OpenSearch org (e.g. generally require 1 approval, merged by the contributor if they have contributor rights)? Added (or will when I get my gpg key from home this weekend).

  • What do maintainers look for in PRs (e.g. tests)? Added

  • What's the merge bar? (e.g. code is an improvement over what we have vs. perfect) I am not sure what this means, can you provide a pointer @dblock ?

  • What to look for in a PR? (e.g. ensure quality or security) I think adding scanning into the pipeline is helpful for security and CodeQL. I do not have good examples of what else should be done on this front. We can open a new issue to get those added to the build process if you want me to help there.

  • How do we expect maintainers to behave? (e.g. be clear about what's a must have, should have or nice to have, and lead with empathy) This seems to be covered here : https://github.com/opensearch-project/community/blob/main/CODE_OF_CONDUCT.md I don't think this is great, I like ours in Jaeger better : https://github.com/jaegertracing/jaeger/blob/main/GOVERNANCE.md#maintainer-duties I can add some of these to the existing file if you think it's a good idea. I don't think these are controversial.

  • Links to external resources about doing excellent code reviews. Got any? I don't code as much as you do 🥇

dblock commented 4 days ago

I am pretty sure I was typing these from a brainstorm with someone, so I don't have all the answers :)

I think we need to discuss this in the technical steering committee if we want an SLA. I can tell you we try to answer in 24h, but we also ask people to ping us on Slack if we are not meeting the SLA. We also use Dosu to mark things stale.

Personally I would say maintainers do their best without any SLAs, and that is what should be stated.

What's specific about PRs in OpenSearch org (e.g. generally require 1 approval, merged by the contributor if they have contributor rights)?

Many repos require 2 approvers, most repos require squash only.

What's the merge bar? (e.g. code is an improvement over what we have vs. perfect) I am not sure what this means, can you provide a pointer @dblock ?

I would write something along the lines that say that the PR should make things better, not worse, and should not create tech debt for others (I know @andrross has opinions here).

How do we expect maintainers to behave? This seems to be covered here : https://github.com/opensearch-project/community/blob/main/CODE_OF_CONDUCT.md

I think this should be specific about PRs, not just "act with empathy". One idea is to say that maintainers should be clear about what is a must have vs. should have vs. nice to have in the PR to help the contributor get the PR over the merge line.

Links to external resources about doing excellent code reviews. Got any? I don't code as much as you do 🥇

Do not, but we should ask around ;)

jkowall commented 19 hours ago

Pushed the first fix on this now via #228

Should we discuss these open items on the TSC meeting?