Hi @Chris-V.

I experienced something similar in the #247, but @markjm fixes (I haven't tested it yet) in #262

Probably it's not working for sub-orgs?
Are you using the new version 2.0.0?

:)

I think what you are looking for is to have multiple sub-org configs be applied to a single repo and merge the configs of multiple sub-orgs to that repo config.

Hi @Chris-V.

I experienced something similar in the #247, but @markjm fixes (I haven't tested it yet) in #262

Probably it's not working for sub-orgs? Are you using the new version 2.0.0?

:)

Yes, I'm testing against 2.0.0. Everything is merged properly except all of the suborgs. We are essentially limited to a single suborg per repo.

I think what you are looking for is to have multiple sub-org configs be applied to a single repo and merge the configs of multiple sub-orgs to that repo config.

In a way, but my understanding is that it may not be clear upfront in which order suborg configs will be applied. Hence my suggestion of an explicit "extends" keyword. Although for our usage (merge required_status_checks), a merge of the suborgs would be enough.

What do you think about such an organization of the settings: #370?

I think there's definitely value, but it's somewhat orthogonal to the main use case I'd see from this particular issue. eg. for at my org we'd have both discipline-specific and business-suborganisation specific configuration that it would be nice to be able to intelligently merge together.

A little heads up on merging required_status_checks. I could not apply my checks because the GitHub API rejected the payload defined as:

contexts: []
checks: [ { ... }, { ... } ]

To my surprise it turned out that I was having a suborg file with suborgteams. The suborgteams feature turns out to be like an alias for all the repositories defined on the given team. With multiple uses of suborgteams for the same team it's a bit unclear what config is used. It seems to apply the last config (in alphabetical order).

In my case I had contexts: [] in that config with suborgteams and the schema for the endpoint in the GitHub API accepts either contexts or checks (contexts are deprecated btw). So these two can't be merged and used simultaneously, ie., you have to use contexts (with name and appid) consistently in all safe-settings config files.

An option to #262 could be to allow inclusion of partial configurations. Something similar to this: https://stackoverflow.com/a/9577670/2447296

By using that a user could define behaviors in separate files such as in this example:

• Org has many teams
• Each team has suborg baseline
• Each team works with frontend and backend apps
• Each tech stack (frontend, backend, etc.) of the same kind has the same checks baseline

Example using include for python project checks:

Org-level checks baseline for all Python projects:
.github/behaviors/python_project_checks.yml

- context: Block Autosquash Commits
  app_id: 15368
- context: Commitlint
  app_id: 15368
- context: Black, isort, Ruff, and Pytest
  app_id: 15368
- context: SonarCloud Code Analysis
  app_id: 12526

Team-level baseline:
.github/suborgs/team1.yml

teams:
  - name: team1-developers
    permission: admin
  - name: all-developers
    permission: push

branches:
  - name: default
    protection:
      required_pull_request_reviews:
        required_approving_review_count: 1
        dismiss_stale_reviews: true
      required_linear_history: true
      allow_deletions: false
      block_creations: true
      required_status_checks:
        # Required. Require branches to be up to date before merging.
        strict: true
        checks: []
      enforce_admins: false

.github/repos/team1-python-service123.yml

branches:
  - name: default
    protection:
      required_pull_request_reviews: {}
      required_status_checks:
        checks:
          - !include behaviors/python_project_checks.yml

Is it safe to say that the same repository being part of multiple suborgs is undefined behavior right now? I don't see any documented ordering by which safe-settings will determine which suborg to apply to a repository that matches multiple suborgs.

If it really is undefined behavior, it would be nice if the PR check bot would fail a status check in this case.

@grossag I would say, yes, it is an undefined behavior since, when you look at

I think adding a PR check fail would be an easy solution since we don't have to figure out the rules for precedence, etc.

I'll look into that.

@grossag I would say, yes, it is an undefined behavior since, when you look at

safe-settings/lib/settings.js

Line 684 in f6c8d43

async getSubOrgConfigs () {

, the suborg config associated with a repo depends on the order the suborg files are processed and the last suborg config that applies to a repo is the one that is left assigned as the suborg config for the repo.
I think adding a PR check fail would be an easy solution since we don't have to figure out the rules for precedence, etc.

I'll look into that.

Thanks @decyjphr . It makes sense that it would be easy to check whether a repository matches multiple suborgs via name pattern. But I am not sure about the more advanced cases.

My one point of confusion is this: as far as I can tell, suborg matching is implemented the following way:

1. Suborgs are matched directly by name here -

   safe-settings/lib/settings.js

   Line 297 in f6c8d43

   const subOrgConfig = this.getSubOrgConfig(repo.repo)

2. Suborgs are matched by custom property or team using plugins by calling into this plugins and invoking the sync() function -

   safe-settings/lib/settings.js

   Line 330 in f6c8d43

   return new Plugin(this.nop, this.github, repo, config, this.log, this.errors).sync()

   calls

   safe-settings/lib/plugins/diffable.js

   Line 75 in f6c8d43

   sync () {

   which calls into plugins such as https://github.com/github/safe-settings/blob/f6c8d436457f91308a84e79754b32ec79aa68314/lib/plugins/teams.js and https://github.com/github/safe-settings/blob/f6c8d436457f91308a84e79754b32ec79aa68314/lib/plugins/custom_properties.js

If that is correct, I wasn't able to figure out a way to catch the more advanced suborg matching cases where, for example:

1. One suborg matches via custom property
2. Another suborg matches via team
3. Another suborg matches via name wildcard pattern
   and one repository matched multiple of those patterns. Do you think that is fixable?

@decyjphr your expert comments will certainly help in here!!

• Suborgs are matched directly by name here -

    [safe-settings/lib/settings.js](https://github.com/github/safe-settings/blob/f6c8d436457f91308a84e79754b32ec79aa68314/lib/settings.js#L297)
  
  
       Line 297
    in
    [f6c8d43](/github/safe-settings/commit/f6c8d436457f91308a84e79754b32ec79aa68314)
  
  
  
  
  
      
        
         const subOrgConfig = this.getSubOrgConfig(repo.repo)
  

@grossag

If you look at the lines shown above, we build a dictionary of suborg configs with the repo name as the key. The repo names are obtained using the logic associated with suborgrepos or suborgteams or suborgproperties. So it is possible that a repo could show up in any of these matching scenarios and it would be associated with the matched suborg config.

It could match one or many of these criteria in a suborg config file and then that config will be associated to that repo. And, sometimes, subsequently it could be matched by the criteria in a different suborg config and it would be associated to that suborg config. I feel this is somewhat similar to how a CSS setting works. (Maybe we can use a page from that book and use a !important rule that will override any previous rules. )

For now, I can add logic in the code to check it is in a dryrun mode and if a repo name already exists in the dictionary to throw a PR check error.

I hope this answers your question.

• Suborgs are matched directly by name here -

    [safe-settings/lib/settings.js](https://github.com/github/safe-settings/blob/f6c8d436457f91308a84e79754b32ec79aa68314/lib/settings.js#L297)
  
  
       Line 297
    in
    [f6c8d43](/github/safe-settings/commit/f6c8d436457f91308a84e79754b32ec79aa68314)
  
  
  
  
  
      
        
         const subOrgConfig = this.getSubOrgConfig(repo.repo)
  

@grossag

safe-settings/lib/settings.js

Line 699 in f6c8d43

if (data.suborgrepos) {

If you look at the lines shown above, we build a dictionary of suborg configs with the repo name as the key. The repo names are obtained using the logic associated with suborgrepos or suborgteams or suborgproperties. So it is possible that a repo could show up in any of these matching scenarios and it would be associated with the matched suborg config.

It could match one or many of these criteria in a suborg config file and then that config will be associated to that repo. And, sometimes, subsequently it could be matched by the criteria in a different suborg config and it would be associated to that suborg config. I feel this is somewhat similar to how a CSS setting works. (Maybe we can use a page from that book and use a !important rule that will override any previous rules. )

For now, I can add logic in the code to check it is in a dryrun mode and if a repo name already exists in the dictionary to throw a PR check error.

I hope this answers your question.

Thank you! I hadn't realized that async getSubOrgConfigs() implementation is where the main matching is implemented and has information about all 3 matching formats (data.suborgrepos, data.suborgteams, and data.suborgproperties), which would allow settings.js to definitively determine whether a repository matches two suborgs, regardless of the matching pattern.

That solution sounds great, thank you! That would help us be able to deploy safe-settings.

I am testing the release candidate that was provided to me with the fix for this problem and it's definitely working much better than before. I tested this in an org that has 9 total repos (including the repo which contains the safe-settings configurations). When I raise a PR that deliberatly introduces a suborb collision as I would expect the validation check now fails.

That being said the comment that the probot app leaves on the PR seems a little odd. There are 10 line items in the comment despite there only being 9 repos in the org. Furthermore all 10 line items say the same things for every column. I would actually expect 8 line items since the repo containing the safe-settings configs is exempt from safe-settings by default. The following screenshot shows what I am seeing with the 2.1.12-rc.1 build.

@scottcarter87 I fixed this in 2.1.14-rc.1. Let me know if that works for you.

@scottcarter87 I fixed this in 2.1.14-rc.1. Let me know if that works for you.

Thank you! Downloading the build now and giving it a try.

@decyjphr Looks much better, thanks!