Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing support for suborg level rulesets #597

Open
stevoland opened this issue Feb 17, 2024 · 11 comments
Open

Implementing support for suborg level rulesets #597

stevoland opened this issue Feb 17, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@stevoland
Copy link
Contributor

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

@stevoland stevoland added the enhancement New feature or request label Feb 17, 2024
@decyjphr
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link

JakubBiel commented Apr 10, 2024

@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
Copy link
Contributor Author

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

It looks like it's supposed to work

safe-settings/index.js

Lines 295 to 304 in 9128ea8

robot.log.debug('Repository Repository edited by a Human')
if (payload.repository_ruleset.source_type === 'Organization') {
// For org-level events, we need to update the context since context.repo() won't work
const updatedContext = Object.assign({}, context, {
repo: () => { return { repo: env.ADMIN_REPO, owner: payload.organization.login } }
})
return syncAllSettings(false, updatedContext)
} else {
return syncSettings(false, context)
}

@JakubBiel
Copy link

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
Copy link
Contributor Author

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

@JakubBiel
Copy link

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
Copy link
Contributor Author

That's correct. We do have some fixes that aren't pushed upstream. This one might be relevant eeveebank@db7ff86.

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

@stevoland
Copy link
Contributor Author

Also, empty diff if there's multiple commits on the PR branch eeveebank@bc83d9f

@beatngu13
Copy link

beatngu13 commented Oct 18, 2024

# This condition only exists at the org level (remove for suborg and repo level rulesets)
repository_name:

This comment in combination with this issue confuses me a bit. Are rulesets for suborgs supported or not? Because given that the above comment, it suggests one only has to leave out rulesets.conditions.repository_name and put the ruleset into a suborg, in order to get a suborg ruleset?

UPDATE: I just tried it myself. When placed in a suborg YAML, then the ruleset is created on the repository (not organization) level.

@paddyroddy
Copy link

I got this working with the following config https://github.com/UCL-MIRSG/.github/blob/main/safe-settings/suborgs/rulesets.yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants