search

TBD54566975 / tbdex

56 stars 26 forks source link

Consider increasing # of reviewers to 3 #252

Open jiyoonie9 opened 9 months ago

jiyoonie9 commented 9 months ago

Currently the # review required to merge a PR in this repo is 2.

Relevant comments/contexts https://github.com/TBD54566975/tbdex/pull/248#issuecomment-1964484797 https://github.com/TBD54566975/tbdex/pull/248#issuecomment-1964727593

diehuxx commented 9 months ago

How should we decide this? Let's try this: if a majority of current codeowners of this repo react with 👍 to the issue description above AND none of the existing codeowners express strong pushback, we'll move forward with increasing the number of reviewers to 3.

Since this would increase demand on codeowners, I'd prefer to get most of us on board. I'm in favor of the change.

KendallWeihe commented 8 months ago

love it @diehuxx! 👍

jiyoonie9 commented 8 months ago

how do we feel about only scoping the 3 approvals to spec / api changes, and not test vector changes? currently the flow we generally all follow is:

  1. PR for updating spec in tbdex
  2. PR for implementing spec change in appropriate sdks
  3. PR for adding / updating test vectors so tests pass in the sdks from step 2

wondering if we can modify github approval perms to be scoped to specific areas of a repo tho 🤔

phoebe-lew commented 8 months ago

Don't understand the original rationale for why it needs to be 3. Are you aiming for a certain percentage of codeowners?