search

github / safe-settings

ISC License
561 stars 137 forks source link

Implementing support for suborg level rulesets #597

Open stevoland opened 4 months ago

stevoland commented 4 months ago

New Feature

We're investigating implementing support for rulesets in suborgs.

It seems that, with this missing piece, rulesets can completely replace branch protections (and their limitations such as the api not supporting branch name patterns)

It would be good to get some more detail on this comment @decyjphr

(deferring the handling of rulesets entries in .yml until ruleset targeting using repo properties is available and we would want to do the same in suborg yml)

I note that repository_property is recently available in org ruleset conditions but am wondering how this might affect the implementation of suborg rulesets?

Wouldn't suborg rulesets be implemented as repo rulesets in the same way as eg: branch protections?

It would be great if you could give a sketch of the requirements here or indicate if this is on the immediate roadmap, thanks

decyjphr commented 4 months ago

If rulesets are defined in the suborg yaml, we could create repo rulesets for each repo in the suborg. But the ruleset team took a different approach by providing a way to create a ruleset at the org level and yet target it to a subset of repos using repo properties.

With that option in mind, first, it would be nice if we could support repository properties as a way to define sub-orgs. Second, if the suborg is defined using repo properties and has a ruleset in the yaml, we could create the ruleset as an org ruleset instead of multiple repo rulesets.

These is what I was thinking but haven't started designing anything yet.

stevoland commented 4 months ago

Thanks @decyjphr, that makes sense although it seems like defining sub-orgs by properties is in tension with the usual business of safe-settings being the source-of-truth for other settings.

I realised after posting that it is currently possible to define rulesets in suborgs which create repo rulesets. Is it best not to rely on this undocumented feature?

JakubBiel commented 2 months ago

@stevoland It doesn't seem to work for me. You can create a ruleset but not modify it and the app won't revert changes that are made manually, even after subscribing to repository_ruleset events. I suppose the app doesn't know what to do with that event.

stevoland commented 2 months ago

@JakubBiel Fair, I haven't tested that webhook myself.

It looks like it's supposed to work https://github.com/github/safe-settings/blob/9128ea8349955b6e0405c7d9a1a4935edf9992b4/index.js#L295-L304

JakubBiel commented 2 months ago

Maybe because it doesn't detect any changes? I wasn't able to modify the ruleset (only create it) after all so something is definitely not working.

stevoland commented 2 months ago

We haven't had any issues modifying rulesets so far fwiw

JakubBiel commented 2 months ago

So just to confirm, you were able to change ruleset under suborg settings and the diff appeared in the PR comment and then it modified the ruleset. If so, would you mind checking if the webhook works for you? Maybe I'm doing something wrong here

stevoland commented 2 months ago

That's correct. We do have some fixes that aren't pushed upstream. This one might be relevant https://github.com/eeveebank/safe-settings-public/commit/db7ff86146e6cca2f96f7dc69fca8e1442ac7a32.

From memory if there's a ruleset that is unchanged an exception is thrown and swallowed and the diff will be empty

stevoland commented 2 months ago

Also, empty diff if there's multiple commits on the PR branch https://github.com/eeveebank/safe-settings-public/commit/bc83d9f68ec4f3557ad31237b0139136f0b27f32