WordPress / performance

Performance plugin from the WordPress Performance Group, which is a collection of standalone performance modules.
https://wordpress.org/plugins/performance-lab/
GNU General Public License v2.0
347 stars 94 forks source link

Consider implementing Plugin Release Confirmation approvals #783

Open joemcgill opened 1 year ago

joemcgill commented 1 year ago

The WordPress.org plugin repo has an opt-in feature for releases, called Release Confirmation. That can be used to avoid inadvertent releases. During the release party for 2.5.0, @westonruter suggested that we consider opting into this program for the Performance Team plugins, so I've added this issue for discussion.

Requirements

westonruter commented 1 year ago

The nice thing about this is that even though the release tag is committed to SVN, the release is not made official until the confirmation is made. This allows you to do sanity checks and verification of the code that was committed before making it live to everyone. It also adds an important layer of security.

westonruter commented 2 months ago

This came up again yesterday on Make/Plugins in a post about Keeping Your Plugin Committer Accounts Secure.

joemcgill commented 2 months ago

I think it's a good idea. The only prerequisite, I think, would be for us to make sure we align the access and responsibilities for running a release with those who can approve, so a release doesn't get blocked by lack of approval access.

westonruter commented 2 months ago

The problem right now is the only committer is the core performance team account, the email for which I don't have access to. Once Felix gets back we'll need to make sure that emails sent to that address get forwarded on.

swissspidy commented 1 month ago

The problem right now is the only committer is the core performance team account, the email for which I don't have access to.

@westonruter Actually, we do have access to the email address, it's in the password manager. But would be great to improve the access there.

westonruter commented 1 month ago

@swissspidy I don't think so because, as I recall, when I tried it asked for two-factor auth code as well.

swissspidy commented 1 month ago

I tried it yesterday and I just had to provide the right phone number. But yes ideally 2FA would be set up differently.

felixarntz commented 1 month ago

Since I'm back now, happy to make changes as needed. Though I'm not sure what's the most straightforward approach to shared 2FA.

I don't think we need to do anything about the email access at this point, but of course 2FA should not be limited to myself, so it would be great to change that, at a minimum so that the people with access to that email can get beyond the 2FA.

westonruter commented 1 month ago

I think we can handle this by simply adding more committers to the plugin on WordPress.org. Currently only the performanceteam user on WordPress.org is added as a committer. If we added other users who are responsible for releases to the list of committers, then all of these committers will get the release confirmation email. We wouldn't then have to rely on a single shared performanceteam WordPress.org account and its corresponding Gmail account, bypassing the issue of needing to share MFA. Using our own personal accounts I believe is better for both security and transparency.

felixarntz commented 1 month ago

Adding more committers sounds fair, though we'd need to define by which criteria this should happen. I think the performanceteam account should however continue to be used for all the automated commits that come from this repo (i.e. basically all commits).

westonruter commented 1 month ago

In the past year, only you, me, and @joemcgill have managed releases. So I think our accounts would make sense to be added as the committers. If someone else is to manage a release, either they can be added as a committer or one of us can simply approve the pending release.

felixarntz commented 1 month ago

How about for every plugin we add as committers those accounts specified in the CODEOWNERS file for the respective plugin?

On that note, see related PR #1432 as the file is currently outdated. I would argue if you're a code owner for the plugin, it makes sense that you're also a "code owner" for the code on wordpress.org. For those plugins where there's currently only a single owner, I would suggest we add another owner, so that every plugin has at least 2 people as owners.

I think this would make for a well-aligned approach for handling release confirmations. WDYT?

westonruter commented 1 month ago

How about for every plugin we add as committers those accounts specified in the CODEOWNERS file for the respective plugin?

I'm not sure I understand. Are you saying that for every plugin, the GitHub users in CODEOWNERS should correspond to the WordPress.org users added as committers?

If we add release confirmation for the individual feature plugins this could get complicated if the person doing the PL plugin release isn't also a committer of each plugin on dotorg, as otherwise they wouldn't be able confirm all of the plugin releases and would be dependent on the committers to be available.

So I think that those who are responsible for doing Performance Lab plugin releases, that they should be made committers of all the plugins. Then in addition to this, the CODEOWNERS can also be made committers. An added benefit of this CODEOWNER-committer correspondence is that in the support forum the user would be shown as "author" which would help with credibility for responses as well as recognition for contributions.

Another question then is whether the CODEOWNERS should also be listed among the contributors in each plugin's respective readme.txt.

felixarntz commented 1 month ago

If we add release confirmation for the individual feature plugins this could get complicated if the person doing the PL plugin release isn't also a committer of each plugin on dotorg, as otherwise they wouldn't be able confirm all of the plugin releases and would be dependent on the committers to be available.

Shouldn't the release confirmation act as some sort of second sign-off? I think it makes sense that this is not a 1-person process. I don't think the specific person that does the release needs to be added to all plugins as a code owner, but that it's more like a "pool" of people that are regularly active. I believe the release party chats are typically attended by at least 2 of these people, so I feel like this would work.

To clarify what I mean: Right now, all but one plugin have at least two out of @westonruter @joemcgill @adamsilverstein @mukeshpanchal27 @felixarntz listed as code owners. I think it's fair to assume that this gives good coverage for the release process. We could fix the remaining plugin to also be covered in that way, and of course we can always add more where it makes sense. I just don't think every release coordinator needs to be the owner of every plugin.

Then in addition to this, the CODEOWNERS can also be made committers. An added benefit of this CODEOWNER-committer correspondence is that in the support forum the user would be shown as "author" which would help with credibility for responses as well as recognition for contributions.

👍

Another question then is whether the CODEOWNERS should also be listed among the contributors in each plugin's respective readme.txt.

Maybe, but let's leave that for a separate conversation, as it's not crucial for any of our workflows.

westonruter commented 1 month ago

Shouldn't the release confirmation act as some sort of second sign-off? I think it makes sense that this is not a 1-person process.

Possibly, but I see the release confirmation more as a security enhancement. We already have CODEOWNERS and branch protection in the GitHub repo for second sign-off. If we make release confirmation a third sign-off, that could bog down the release process when multiple plugins are released at once (with differing CODEOWNERS).

I don't think the specific person that does the release needs to be added to all plugins as a code owner, but that it's more like a "pool" of people that are regularly active.

Actually I didn't mean for the release lead to be added as a CODEOWNER but rather as a committer on dotorg for each of the plugins. In this way they could confirm the releases.

I believe the release party chats are typically attended by at least 2 of these people, so I feel like this would work.

I'm not sure I understand. If one person initiates the release, what would prevent that same person also from confirming the release?

felixarntz commented 1 month ago

Possibly, but I see the release confirmation more as a security enhancement. We already have CODEOWNERS and branch protection in the GitHub repo for second sign-off.

Exactly because it's a security enhancement, I wonder why should a single person be able to sign off on the release of all these plugins? I think it makes sense for there to be owners for each plugin that can do that sign off.

If we make release confirmation a third sign-off, that could bog down the release process when multiple plugins are released at once (with differing CODEOWNERS).

Per my previous reply, in theory yes. In practice, the code owners (that would then also be the respective committers) need to be active members of the team, so I don't think the release process would suffer from that requirement. Also, if let's say for one plugin all owners are AFK at some point, I don't see what would be the harm of having that plugin release approved let's say a day later.

Actually I didn't mean for the release lead to be added as a CODEOWNER but rather as a committer on dotorg for each of the plugins. In this way they could confirm the releases.

Sure, I was only using the terms interchangeably since my proposal is to align who's a code owner and who's a committer on .org.

I believe the release party chats are typically attended by at least 2 of these people, so I feel like this would work.

I'm not sure I understand. If one person initiates the release, what would prevent that same person also from confirming the release?

Nothing, but this was referring to my point that the person initiating shouldn't be a committer on every single plugin just because they trigger the release. And if that's not the case, going back to your point of bogging down the release process, this was to emphasize that realistically the release process wouldn't be bogged down because from every plugin someone with commit access will be present in the release party chats.