Decision making and governance for included checks

[felixarntz]

In this issue, I'd like for us to brainstorm and discuss a bit on how we would like this plugin to evolve in the future, in particular regarding additional checks to be added.

There probably needs to be some sort of process, especially as more people may contribute to the project in the future (hopefully!). We certainly shouldn't overcomplicate this, but we may need more than just "any maintainer approving any PR including a new check" as a "decision" to include a new check.

While I'm sure none of us has any malicious intentions, the purpose of this discussion is to find a way to ensure the "integrity" of the plugin checker long-term. We probably want to avoid that, for example, an extremely opinionated check would make it into the plugin checker and then cause confusing errors for tons of plugin developers.

I'm curious to get your perspective on this. Some questions / ideas to discuss:

• Should there be specific CODEOWNERS whose approval is required to merge a new check?
• Should PRs that add a new check require having a "check proposal" issue that was formally approved, in some way?
• Should there be specific people or groups assigned to "own" specific "check categories"? (For instance, ownership between different verticals like plugin review, performance, accessibility etc. could differ, based on expertise and involvement.)
• Should there be additional requirements for new checks, such as having a documentation URL attached about how to fix the respective problem?
• Or maybe, is the simple approach of any maintainer approving a PR with a new check sufficient?

cc @bordoni @barrykooij @swissspidy @mukeshpanchal27

[barrykooij]

Hey @felixarntz,

I think this would be the most suitable:

Should there be specific people or groups assigned to "own" specific "check categories"? (For instance, ownership between different verticals like plugin review, performance, accessibility etc. could differ, based on expertise and involvement.)

I can only speak from the plugin review part of course, but for us it would be ideal if we can change and merge our own checks. These checks will reflect our guidelines, which we discuss within the team and publicly already. Since checks in PCP only reflect our guidelines, having discussion here regarding adding them makes no sense to me.

If we need to change structural things that affect other parts of the plugin, we should do this via issues that all related parties can discuss in, in my opinion.

This all is just my personal opinion of course, happy to discuss this further.

[chriscct7]

I strongly agree with @barrykooij

I think what makes most sense is:

• Performance checks: merged only by Performance team
• Security checks: merged only by Plugins Team Security members (or users that team designates as needed) as these are currently enforced on plugin re-reviews manually currently by that team
• Guidelines checks: merged only by Plugins Team members (any) as these would have to follow the guideline discussions made by that team and will soon be enforced on new plugin submissions
• For all other categories (general, accessibility, etc) we should allow those with merge access to merge them.

If there's a discussion required about a check, we can do so on the PR, and its easy enough to revert them, but this will allow for quick iteration ability which will be crucial because once PCP is required and enforced on submission on .org soon, the activity of this repo will go way up, and there's not a ton of people entrusted with merge on this repo currently, so we should be fine there. We can always have further discussions as needed on this if an issue arises.

And to be clear, the above is just specific to implementation or alteration of checks. As Barry mentioned re-architecture or larger stuff should be more broadly discussed beforehand so I'd consider that a seperate issue

[felixarntz]

Thank you @barrykooij @chriscct7, I'd be onboard with your suggested approach as well.

+1 to your concrete suggestion @chriscct7 on who "owns" which check categories, except with a caveat: Ideally, I'd say performance checks, security checks, and accessibility checks should be responsibility of the respective teams.

At the moment I don't think anyone from the security or accessibility teams has been involved in the plugin checker much, but it would be great to have those teams be aware, to review relevant proposals from the community. Potentially some contributors from the teams are even interested in getting more actively involved.

Pinging @aaronjorbin @joedolson @peterwilsoncc for visibility due to their involvement in the security and accessibility teams.

[joedolson]

I'd love to be involved in helping to develop and manage accessibility checks in this.

[chriscct7]

That makes sense to me. I've been watching the development of PCP as Robert and I use it during the security re-reviews. As we've started having more and more plugin authors use it on security re-review we'll be writing more checks or we may give the requirements of those checks to other members of the plugin team to implement them since some of those might be more broadly applicable on the plugin initial submission

[davidperezgar]

Yes, each team knows how checks works and what they need for helping their area. So, we could put categories in the PR and make it available for the team and ready to merge. As I suppose, it would be needed to be merged to trunk, right?

[swissspidy]

All makes sense to me so far; each team owns their own checks. We can group these checks in different folders so we can more easily enforce CODEOWNERS on GitHub.

Regarding security checks: the core security team probably doesn't have capacity to help with plugin security checks, and it's not really an area the team usually works on. There are dedicated Plugins Team Security members, as @chriscct7 mentions, who should be owning these.

I think we should encourage cross-team code reviews though to keep coding style consistent throughout the project and improve feedback loop.

Should there be additional requirements for new checks, such as having a documentation URL attached about how to fix the respective problem?

For similar consistency reasons we should establish some minimum requirements when it comes to documentation. Already now we have a lack of documentation for existing checks, so we should add that first and then enforce it for any new checks as well.

[felixarntz]

Should there be additional requirements for new checks, such as having a documentation URL attached about how to fix the respective problem?

For similar consistency reasons we should establish some minimum requirements when it comes to documentation. Already now we have a lack of documentation for existing checks, so we should add that first and then enforce it for any new checks as well.

Big +1 to that. Ideally, I think every check should have a documentation URL attached to it which provides information on how to address the respective problem. Otherwise we just identify problems without suggesting solutions.

We definitely have to first catch up on this with the existing checks, but I think this should be baked into the code, e.g. every check requiring a documentation_url value for instance.

[barrykooij]

I agree with CODEOWNERS and having a required documentation_url per check.

I've addressed this issue in the last plugin review team meeting, and we want some but not all team members as CODEOWNERS for the plugin part, so there are designated people from the plugin team that can focus on what gets merged. I'd like to take this role on me, more will most likely follow but we haven't discussed this yet.

What would be the next step to move forward. Create new (separate) issues for the CODEOWNERS and documentation_url tasks, and close this issue?

If all agree, I'd like to set both these new issues on a milestone after 1.1 as we're trying to focus on getting 1.1 ready as soon as possible. Version 1.1 will be used as the first integration on meta, to be used on new plugin upload process. Our expectation is this will help significantly with the new plugin queue.

[swissspidy]

New issues sound good, we can keep this one open though just in case.

These will be minor changes anyway, so personally I don't mind when these will be picked up earlier :)

[swissspidy]

Just opened #460 and #461 for those two items.