Skip to content

Support negation globs #116

Closed
Closed

Description

@mscrivo

Negation globs seemingly don't work. For example, I can't do something like this:

- '!(workspaces)/**/sourcing/**/*'

to say that I want this team to own any folder that has a "sourcing" subfolder in the path, but not within any "workspaces" folders.

This works just fine when tested with compgen. code_ownership doesn't seem to throw an error when validating, but when checking a path that should be owned under that glob, say lib/sourcing/some_file.rb, it incorrect states that it's not owned. Interestingly, it still correctly updates the CODEOWNERS file with the negation glob.

So now, instead of having single glob that covers most of this team's ownership, I will have to explicitly map out each folder instead.

Activity

  1. mscrivo commented on Feb 7, 2025

    @mscrivo
    ContributorAuthor

    It seems this may be more a limitation of Dir.glob rather than this gem. It doesn't seem to support glob negations

  2. ashleywillard commented on Feb 7, 2025

    @ashleywillard
    Contributor

    Hi @mscrivo! Thanks for filing this issue. You're correct, currently code_ownership does not support this syntax, and there is that limitation with Dir.glob. One way to add support for excluding globs would be to add an unowned_globs array for teams. We would welcome this contribution if you'd like to make a pull request!

  3. added
    enhancementNew feature or request
    and removed
    triageA new issue that needs review by the core team
    on Feb 7, 2025
  4. mscrivo commented on Feb 10, 2025

    @mscrivo
    ContributorAuthor

    This is trickier than I thought. Making it so that it understands unowned_globs is not that hard for validate for example. But the tricky part is how to translate it to CODEOWNERS file. At first I thought, ok we can just list the globs in unowned_globs in the CODEOWNERS with no team specified, but what if another team's config does own them? How to reconcile all the possibilities into a valid CODEOWNERS is not clear to me.

    EDIT: Actually, I think what we need to do is for any overlapping globs where one team owns it and another team has it in the unowned_globs, we need to make sure the glob for the team that owns it comes, comes last. For example, say you have:

    name: Bar
      owned_globs:
        - app/services/bar_stuff/**/**
        - '**/team_thing/**/*'
      unowned_globs:
        - shared/**/team_thing/**/*

    and

    name: Foo
      owned_globs:
        - shared/**/*

    Then the CODEOWNERS should look like this:

    app/services/bar_stuff/**/** @bar
    **/team_thing/**/* @bar
    # needs to come last to take precedence
    shared/**/* @foo

    with this, team bar would own: lib/team_thing/some_file but team foo would own shared/team_thing/some_file which is the desired outcome I believe?

  5. mscrivo commented on Feb 11, 2025

    @mscrivo
    ContributorAuthor

    Digging more into this, it seems that CODEOWNERS entries are already sorted by least specific to most specific paths, so we may be able to make the assumption that if you have an unowned_glob defined on a team, you're doing it because both of these are true:

    1. your team has a glob that covers a wide swath of things (ie. is not very specific and will be near the top of the CODEOWNERS)
    2. the other team has a more targeted glob that overlaps (ie. will be lower in the CODEOWNERS).

    If those assumptions hold, then we don't have to change any logic for the CODEOWNERS generation. Or even if they may not, we can add a validation error an unowned_glob that is less specific then it's owned counterpart?

    @ashleywillard does that make sense to you?

    btw I have a PR in progress here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions