Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.86%. Comparing base (c2402cf) to head (6acc085).
⚠️ Report is 137 commits behind head on main.

@@            Coverage Diff             @@
##             main     #986      +/-   ##
==========================================
- Coverage   86.45%   81.86%   -4.60%     
==========================================
  Files          73       73              
  Lines        3293     3297       +4     
==========================================
- Hits         2847     2699     -148     
- Misses        446      598     +152     

Thanks a very interesting contribution, @Anedaar, thank you! Have you perhaps compared it to our analysis of the alternative approach described here https://github.com/gitlabform/gitlabform/wiki/Current-and-proposed-way-of-selecting-targets ?

I feel this PR is introducing breaking change. But more importantly, I don't understand the problem being solved based on the description. It just jumped into solutioning. For example in the proposed config, now it's not clear at all whether a given section is a subgroup or a project. It adds extra cognitive load to interpret the config and it can lead to lots of issues/confusions.

I suggest opening a new issue or engage in an existing issue. Make the problem statement as clear as possible. We already have feature for breaking inheritance.

We also cover this in https://github.com/gitlabform/gitlabform/wiki/Configuration-Syntax-v5-Draft#alternatives-for-includeexclude

I think we shall handle an improved syntax for project/group selection in one go.

Hey everyone,

Apologies for catching you a bit off guard with this PR. Let me provide some context:

I work for a small, but rapidly growing startup, and we expect our team of fewer than 10 developers to manage around 100 projects in the next six months, as we can build upon an existing codebase. Tasked with structuring our GitLab setup, I discovered GitLabForm—and I absolutely love it! No more clicking through hundreds of projects to check if all branches are protected as they should be or if all features are enabled/disabled correctly. However, since I’m new to GitLabForm, I may have missed some important configuration options.

Here’s what we want to achieve: For external partners, we’ve created a group called external. Each partner will have their own subgroup, e.g. external/companyA, external/companyB. We don’t have any public repositories or groups, but we want the external group to have visibility: internal, while all child groups should default to visibility: private unless explicitly specified otherwise, to meet customer confidentiality agreements.
(Similar setups will exist for a group internal with different "private" projects, but lets skip this here)

So, I created a config like this:

    "external/*":
      group_settings:
        visibility: internal
    "external/companyA/*":
      group_settings:
        visibility: private
    "external/companyB/*":
      group_settings:
        visibility: private

Now, if someone adds companyC with visibility: private, GitLabForm will mistakenly set it to internal if I don’t update the config—ugh! The only option I saw was to make the external group also private, which is bearable, but not ideal.

Next, I created a config like this:

    "external/*":
      group_settings:
        visibility: internal
        description: Projects with external partners.
      group_members:
        users:
          anedaar:
            access_level: Owner
    "external/CompanyA/*":
      group_settings:
        visibility: private
        description: Projects with CompanyA.

This works well for external and external/CompanyA, as they get the correct description and visibility. However, external/companyB ends up with the wrong description ("Projects with external partners") and the wrong visibility ("internal"). It’s frustrating—better no description than an incorrect one! Additionally, I’m listed as an “Owner” in all groups and subgroups, meaning all groups say "Anedaar is a direct member," instead of just having me as the owner of external and having all subgroups show "inherited from external" (which is issue #541).

While users and descriptions are specific settings where I don’t think inheritance makes sense, visibility usually does, just not in this case. The issue arises because external/* applies to all subgroups unless explicitly specified otherwise (or using inherit: false), while external is interpreted as a project (even though it’s a group). I can’t specify a setting in the config that applies only to the group and not the subgroups.

Since the change seemed simple enough, I decided to skip the issue and go straight for a PR.

With that background:

@gdubicki

I didn’t see that proposal initially. When I first read it, I thought it only applied to running GitLabForm, not to the projects_and_groups config section. However, my PR is essentially this version of your proposal (config part only):

Proposed behavior:

• ALL_DEFINED and ALL: No changes.
• rootgroup/subgroup/: Applies only to this subgroup and its projects.
• rootgroup/: Applies only to this group and its projects.
• rootgroup/*: Applies to the group and all of its descendants. (No changes)

Details:

• *: Refers to all groups and projects.
• <A>, when A:
  .... is a project: Refers only to this project (no changes).
  .... is a group: Invalid reference (no changes, see below to not break running GitLabForm).
• <A>/, when A:
  .... is a project: Invalid reference (no changes—well, sort of, currently it’s not just an "invalid reference").
  .... is a group: Refers to this group and its projects, but NOT subgroups.
• <A>/*, when A:
  .... is a project: Invalid reference (no changes).
  .... is a group: Refers to this group, its projects, its subgroups, and their projects (no changes).

The “no changes” here refer to the config only. I think this version has the advantage of not altering any existing behavior or invalidating existing configurations.

This PR/syntax addresses your "more than one" issue:

foo/bar/* applies to the group and all subgroups, while foo/bar/ only applies to the group itself. The latter is more specific and takes precedence.

It also solves the "same syntax" issue:

• Keys not ending in / or /* still refer to projects, as they do now.
• Keys ending in / or /* refer to groups (including their projects).

For the running GitLabForm part:

The current behavior is that <A> refers only to the project/group itself, while only ALL or ALL_DEFINED triggers recursion (as I understand from the linked issues). I propose that for projects, <A> remains the correct syntax, while for groups:

• <A>/ runs only the group and its projects.
• <A>/* runs the group and all its subgroups, including all their projects.
• <A> is an alias for <A>/ to avoid breaking current behavior, but might add a hint to switch to <A>/ or <A>/* for clarity.

I personally don't see a use case to distinguish between running group settings or project settings for projects in a group. I think what matters is recursion (or lack thereof). But I trust your judgment on that.

@amimas

Regarding the reasoning: see above.
Regarding the breaking change: I feel differently.

One of the most unintuitive things for me when I first encountered GitLabForm was that it doesn’t distinguish between groups and their projects in the config. A config might look like this:

    foo/*:
      group_settings:
        <...>
      project_settings:
        <...>
      members:
        <...>
       labels:
        <...>

Here, group_settings apply to the group, while project_settings apply to the projects. The members and labels apply to projects as well, with their group counterparts named group_members and group_labels. This mixed design makes it difficult to tell what applies to what just by looking at the top-level key. We know foo is a group, not a project, because the key ends in /* (not just foo). This PR keeps the current syntax and introduces the new option foo/, which refers only to the group. This ensures that existing, valid configs remain unchanged.

@jimisola:

I don’t fully grasp all the details of this v5 draft yet, but it definitely sounds like a huge improvement. I love the idea of separating group-related and project-related settings. I think this will offer even more options than this PR. This PR could become part of 4.X but may be superseded by the linked v5 draft.

Hi @Anedaar . Thanks for all of that details. There's a lot to unpack in there. It's possible I might have missed a few things from your reply.

Regarding whether this is a breaking change or not - I'm still leaning towards this being a breaking change. The updated config syntax changes how we would read the config and what we'd expect to happen or interpret the config.

As for your first example: I think it might be a matter of expectation. For example: if all subgroups under example group should have private visibility, then the config should be following (as you already noted), to avoid config duplication:

    "external/*":
      group_settings:
        visibility: private

    "external/CompanyA/*":
      group_settings:
        description: Projects with CompanyA.

    "external/CompanyB/*":
      group_settings:
        description: Projects with CompanyB.

To be fair, I do understand some of the limitations you've mentioned. GitLabForm is not perfect. For me personally, config readability is extremely important. A common proposal on this topic I've seen makes it impossible to infer what the config is doing from readability perspective. For example, the following maybe fine for automation but as I said, when reading the config, how do I know what's a project and what's a group?

<A>, when A:
.... is a project: Refers only to this project (no changes).
.... is a group: Invalid reference (no changes, see below to not break running GitLabForm).
<A>/, when A:
.... is a project: Invalid reference (no changes—well, sort of, currently it’s not just an "invalid reference").
.... is a group: Refers to this group and its projects, but NOT subgroups.
<A>/*, when A:
.... is a project: Invalid reference (no changes).
.... is a group: Refers to this group, its projects, its subgroups, and their projects (no changes).

Currently the purpose of <group>/* is to be used for common settings for all resources under this group including the host group itself. If something is not common across the board (e.g. group/project description), it shouldn't be there or if there's an exception inherit: false is to be used for that specific resource. It's clear and concise (in my opinion) and I'm not sure if this is clearly understood by gitlabform users.

Also, there are other proposal about expanding on the wildcard usage for selecting a resource (e.g. group/project) to be updated. See #398 and #325 . That's why we probably should think about overall changes to config syntax, in terms of resource selection, so that all of these use cases are properly evaluated and then come up with a proposal/design, and then implementation. I think it's a huge undertaking that will require lots of time and effort. Will you be able to help lead this effort?

What do you think @jimisola @gdubicki @TimKnight-DWP ?

@amimas I think we have definetly fallen afoul of specifying some "group" settings under the group/* heading, perhaps we need to be clearer in the documentation and feedback, some kind of linter?

@amimas @Anedaar
One example I can think of which would be fixed by this is integrations

group/*:
  integrations:
      gitlab-slack-application:
        use_inherited_settings: false
        merge_requests_events: true
        merge_request_channel: maintainers

In this scenario all projects get updated with the specific merge_request_channel etc, however what I want, is to set that integration at the group level, and to have the projects set use_inherited_settings: true -> which there is currently? no way to achieve in GLF