Add SL as codeowners for snaps-owned controllers

[mcmire]

Explanation

Whenever the version of a package in this monorepo is bumped, all dependencies on that package in other workspace packages are also bumped. This happens frequently in release PRs.

The Snaps team is listed as codeowners of permission-controller, notification-controller, and rate-limit-controller, so when any changes to these packages occur in a PR, the Snaps team must approve that PR. This means that if a dependency on a package is bumped in any of these controllers within a release PR, then the Snaps team must approve that release — even if none of those controllers are included in the release. This creates an obstacle to creating releases quickly.

To fix this, this commit adds the Shared Libraries team as codeowners of these controllers as well. This way, they can ask the Snaps team to approve only if it is necessary.

Changelog

(N/A)

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[Gudahtt]

I'd recommend using this instead:

Suggested change

	* @MetaMask/engineering
	* @MetaMask/extension-devs @MetaMask/mobile-devs @MetaMask/shared-libraries

Generally we avoid referencing the entire engineering group in codeowner rules to avoid pinging the entire organization for each PR review.

The closed equivalent to the purpose of the old devs team is: shared libraries, mobile platform, and extension platform. But we don't have teams yet for the client platforms (AFAIK), so the [client]-devs teams will have to do for now.

[mcmire]

Ah, I didn't know we'd changed that policy. Should we update the module template?

[mcmire]

Also I think there are two teams for Shared Libraries: @MetaMask/shared-libraries and @MetaMask/shared-libraries-engineers. Which one should we use?

[Gudahtt]

Ah right; probably shared-libraries-engineers then

And yes, updating the module template would be great as well!

[mcmire]

Okay, this PR updated here: 68654dd

[FrederikBolding]

Hmm, feels like this would be a significant reduction in permissions for engineers working on controllers that aren't in one of the client teams (we have multiple engineers like this in snaps).

[FrederikBolding]

I think this warrants more discussion. Blocking until then.

[Mrtenz]

I feel like this defeats the purpose of us being codeowner. If the main obstacle is dependency changes, maybe add the shared libraries team to just the package.json?

[FrederikBolding]

I feel like this defeats the purpose of us being codeowner. If the main obstacle is dependency changes, maybe add the shared libraries team to just the package.json?

I'm a little bit fuzzy on the reasoning for not using workspace:^ versions in this repo. Is the upside large enough that it is worth having to consider changes like this?

I will also pose the question though, have we been holding back releases by not reviewing these fast enough? I feel like we've been pretty consistent in keeping up with reviews where we are needed, but willing to be convinced otherwise.

If we have and speed is a necessity here, I would agree with Maartens suggestion. But I will also mention that we have had mistakes being made in the past by exclusively changing package.json (for example flip-flopping between devDeps and deps 😅).

[mcmire]

I'm a little bit fuzzy on the reasoning for not using workspace:^ versions in this repo. Is the upside large enough that it is worth having to consider changes like this?

Using workspace:^ versions made it impossible for clients to test changes locally, because Yarn does not resolve these versions when using yarn link. This error is demonstrated here: #1623. Switching to static versions solved this problem.

I will also pose the question though, have we been holding back releases by not reviewing these fast enough? I feel like we've been pretty consistent in keeping up with reviews where we are needed, but willing to be convinced otherwise.

No — y'all have been doing a good job of responding so far, but I know it is always late over there and so if there is a chance that we have to get a release out more quickly than usual then I was trying to give us a way to work around it in case of you were offline.

That said, I agree that perhaps adjusting the codeowners is not the right approach here. It is not the end of the world if we have to wait an extra day for an approval anyway. And if that does indeed end up becoming an issue maybe there is another approach we can take, such as limiting to certain files. I'll close this, then.