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.

Sign up for GitHub

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

Jump to bottom

suborgproperties is matching too many repositories #748

Open
grossag opened this issue Jan 23, 2025 · 4 comments · May be fixed by #750
Open

suborgproperties is matching too many repositories #748

grossag opened this issue Jan 23, 2025 · 4 comments · May be fixed by #750
Labels
bug Something isn't working

Comments

@grossag
Copy link

grossag commented Jan 23, 2025

Problem Description

I have a suborg file:

suborgproperties:
  - sub-org: horizon-cart

repository:
  topics:
    - horizon-cart

This rule matches 11 repositories in my organization, one of which is euc-vdi/cart. So we end up with this entry in settings.js this.subOrgConfigs:

"cart": {
  "suborgproperties": [{"sub-org":"horizon-cart"}],
  "repository":{"topics":["horizon-cart"]},
  "source":".github/suborgs/horizon-cart.yml"
}

That is correct, but this function implementation does too loose of matching:

  getSubOrgConfig(repoName) {
    if (this.subOrgConfigs) {
      for (const k of Object.keys(this.subOrgConfigs)) {
        const repoPattern = new Glob(k)
        if (repoName.search(repoPattern) >= 0) {
          return this.subOrgConfigs[k]
        }
      }
    }
    return undefined
  }

It causes any repository name starting with cart to match this rule!

Can this be fixed by changing the function to simply be this:

  getSubOrgConfig(repoName) {
    return this.subOrgConfigs[repoName]
  }

Context

Are you using the hosted instance of probot/settings or running your own?

Running our own.

If running your own instance, are you using it with github.com or GitHub Enterprise?

GitHub Enterprise Cloud, so github.com

Version of probot/settings

This issue happens when deploying either a692dbf or 774b2e5

Version of GitHub Enterprise

Using cloud.
@grossag grossag added the bug Something isn't working label Jan 23, 2025
@PendaGTP
Copy link
Contributor

@grossag I'm working on it and will submit a pull request soon that maintains the glob functionality

@grossag
Copy link
Author

grossag commented Jan 24, 2025 •
edited
Loading

@grossag I'm working on it and will submit a pull request soon that maintains the glob functionality

Great! Here is a patch that fixed it for me and also added more trace logging to help diagnose future issues: safe-settings-patch.txt. It did drop the glob though, so it is not consistent with what you are trying to do.

Can you explain why the glob functionality is needed? Each usage of getSubOrgConfig() appears to be passing in a full repository name. And each entry in this.subOrgConfigs appears to have a key which is a full repository name. What functionality is lost when a glob is dropped? I am unable to connect the dots between this implementation and the various features mentioned in the README that support globs.

@PendaGTP PendaGTP linked a pull request Jan 24, 2025 that will close this issue
Open
@grossag
Copy link
Author

grossag commented Jan 24, 2025

Ok reading through your diff it looks like this is also used by include and exclude logic, so that makes sense.

Your change looks good to me. It fulfills my need in that minimatch("cart-air-tools", "cart") returns false and minimatch("cart", "cart") returns true.

@PendaGTP
Copy link
Contributor

PendaGTP commented Jan 24, 2025 •
edited
Loading

@grossag

Thanks for you feedback.

Can you explain why the glob functionality is needed?

From my understanding, one reason is that getSubOrgConfig() process suborgrepos, suborgteams and suborgproperties from e.g suborg.yml (stored in this.subOrgConfigs). This enables the use of glob patterns for all of them.

To maintain consistency across the org configuration and other configuration files, I decided to keep glob pattern "support" for suborgproperties. However, I deliberately did not document this because glob patterns won’t work in this case. On the backend, the implementation uses GET /orgs/{org}/properties/values?repository_query=props.PROPERTY=VALUE: and custom property query doesn't seems to support any form of regex/glob pattern.

Ref:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix!: replace custom glob with minimatch for improved pattern matching PendaGTP/safe-settings
2 participants
@grossag @PendaGTP