w3c
Raising the quality bar for registry changes
@iherman I would like to require mandatory PR approvals from all codeowners before a PR can be merged, and have all editors listed as code owners in the repo.
This will ensure that no PR is merged without an editor performing a code review, and reduces the likelihood that PRs sit unreviewed, or that the review burden is shared unequally amongst editors.
@OR13, at present, there is a restriction on merge to main set as follows:
The way I read it (and I am not really a good github admin) only you and Manu (plus the chairs and the W3C team with admin rights, like yours truly) can merge a PR into the main branch. Isn't it what you propose, except for the request of adding @mprorock, as a third editor, to this list?
I believe this is as simple as a PR against https://github.com/w3c/did-spec-registries/blob/main/CODEOWNERS
yes, certainly all editors should be in CODEOWNERs... however I am asking for unanimous positive review, ( and I am sure it will be harder to achieve than a single editor being able to merge things ).
Basically, it should be a requirement that all codeowners approve changes before it is possible for any of them to merge a PR.
Ah. I don't think there's an automated path to that restriction.
I know reviews can be automatically requested from all CODEOWNERS (this might require a switch to be flipped, as I don't think this happens on all repos), but I don't know of any repo automation that requires approval from all (or a specified subset of) requested reviewers before merge can proceed.
@TallTed https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule
Yeah, that requires a number of approving reviews by people with commit authority on the branch (which I believe is a larger group than CODEOWNERS).
It doesn't let you specify a specific subset of committers.
Let us not over-administer ourselves. At this point, there is a very limited number of people who have the right to merge to the main branch. Essentially @OR13 @msporny and the usual suspects, ie, the chairs and myself. I can add @mprorock without a problem. With the limited number of people around (compared to large projects) I think that is enough control.
@OR13 what triggered this request? Was there a specific incident?
@iherman Im interested in ensuring that process changes related to registry editorial activities are shared equally, and that all editors "feel the burden of the process" equally, so they can contribute constructively to its ongoing definition, and hopefully its minimization.
The current format allows for myself or any editor to approve and then merge things, as the only party reviewing the entry.
Since the registry should basically be a list of entries (and not a bunch of value judgements about them, thats the job of the of the registry, the did rubric)... it should not be a burden to require all editors to perform reviews.
For example, it takes me about 30 seconds to review a new DID method spec:
- does the method define the identifier syntax
- does the method cover operation (create, resolve, update, deactivate)
- does the method have a privacy and security considerations section.
Evaluating methods at a finer grained level is not possible, nor am I or any of the other editors capable of fairly conducting an evaluation regarding the sufficiency of these sections.
We are not reviewing code, we are checking for required language.
My hope is that making this change now, will help us gather better information about how subsequent changes have "made things better or worse"...
@OR13 o.k., I think I understand: you want all editors to review things before one of you merge the PR.
Because I have never done that before... just for check this is what I did
- I have added Mike to the list of users who can merge a PR in the first place
- I have added Mike to the CODEOWNERS file
- I have checked the 'require pull request before merging' in the repo settings for 'main' and I also checked the "Require review from Code Owners". There is also a pull down in that sub-setting on the number of required approvals which I left on '1'; the code owners should probably be enough.
Caveat: this is now valid for all PR-s against the main branch. I do not know whether this is wise in view of the current discussions still ongoing on the repo; you tell me.
Adding pending close as I think this has been addressed with the new team of volunteers reviewing DID method PRs.
