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

WIP: Proposal of the v4 (?) config syntax #138

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

gdubicki
Copy link
Member

See #82 for more info.

README.md Show resolved Hide resolved
@gdubicki gdubicki marked this pull request as draft November 24, 2020 15:18
# Here are examples of possible dict key.
#
# This matches all the groups AND all the projects EXCEPT personal projects!
# "*":
Copy link
Contributor

@weakcamel weakcamel Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pattern matching would make the syntax very powerful indeed. The one potential issue I see here is: what if there are 2+ different patterns matching the same project or same group?

  • would this be considered a "fatal error"?
  • if not, which pattern wins? They're defined as dictionary keys so there is no way to predictably order them; they'd need to be a list instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this issue out. We haven't made a decision yet, but we will define some behaviour and write tests to ensure that it will be stable.
And what would be your suggestion?

Copy link
Contributor

@weakcamel weakcamel Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually quite liked the previous 3-level priority: common, overridden on group level, overridden on project level.
This was simple and IMO very effective.

To make it work with pattern matching, I suppose something like this should work:

projects_and_groups:
  - path: '*'   # equivalent of "common_settings"
    group_settings:
      ...
    project_settings:
  - path: 'foogroup/*'   # equivalent of group settings (although it doesn't make it clear whether this includes sub-groups or not)
    group_settings:
      ..
  - path: 'bargroup/quxproject'  # and this is an override per project
    project_settings:
      ..

Those could be processed from top to bottom, with the settings at the bottom "winning" to allow to merge them.

A more powerful option (but also even more complicated):

projects_and_groups:
  - path: '*'   # equivalent of "common_settings"
    continue: true #### <<---- let user select whether this rule should end processing or not
    group_settings:
      ...
    project_settings:
  - path: 'foogroup/*'   # equivalent of group settings (although it doesn't make it clear whether this includes sub-groups or not)
    continue: false #### <<---- let user select whether this rule should end processing or not
    group_settings:
      ..
  - path: 'foogroup/quxproject'  # and this is an override per project which WILL BE IGNORED due to continue=false earlier
    project_settings:
      ..

Copy link

@kowpatryk kowpatryk Dec 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a nice proposal! I believe with tree-like structure of GitLab groups and projects, this could cover most (if not all) of the cases - especially assuming that patterns are defined in the topological order (with patterns affecting more top-level groups/projects defined before the other patterns).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering about pattern matching: unless resorting to "regex", it could be helpful to intrduce a distinction between /* and /** patterns where the former would mean only direct descendents of a group and the latter a recursive match.

Rationale: if recursive were to be default, it would be quite difficult to partition the groups sensibly (make any form of exclusion).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @weakcamel! I think that it would be more efficient to discuss and come up with the best solution using our new Slack workspace. If you (or anyone else reading this and interested in it) want to join it the please use this invite link: https://join.slack.com/t/gitlabform/shared_invite/zt-ljgzvxk0-TubjJFA5lLpRkyivQWI_Qg (valid until ~21 Feb).

# This matches all the groups that have names matching the '(foo)+' regexp AND projects in it.
# "~(foo)+":
#
# This matches all the subgroups of group "my-group" that have names matching the '(bar)+' regexp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subgroups of group "my-group" that have names matching the '(bar)+' regexp AND projects in it.

I'm guessing projects directly under my-group with matching name would be included here as well?

And if not, how could one express it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another good point that makes me think we should really consider going toward recursive search...

# This matches the "my-group" group AND projects in it.
# my-group:
#
# This matches the "my-subgroup" subgroup within the "my-group" group AND projects in it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this be recursive? would this also affect my-group/my-subgroup/my-sub-subgroup?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. What would be expected behaviour for you? I am currently thinking about few possible approaches:

  1. Keep is as it is - no recursion. This means though that all subgroups would have to be explicitly specified (and it weakens the power of pattern matching that we would like to introduce here).
  2. Make it non-recursive by default and allow recursive search to be enabled (either only on single pattern level, globally, or both).
  3. Make it always recursive (with option to disable it? not sure if it makes sense though).

Base automatically changed from master to main January 16, 2021 14:18
gdubicki added a commit that referenced this pull request May 1, 2021
with everything under single key
"projects_and_groups", similarly to
what's described in #138, but simpler
for now (no new features yet).
gdubicki added a commit that referenced this pull request May 2, 2021
with everything under single key
"projects_and_groups", similarly to
what's described in #138, but simpler
for now (no new features yet).
@gdubicki
Copy link
Member Author

gdubicki commented May 4, 2021

The list of issues and PRs that require, or would benefit from the most, introducing backward incompatible changes to v1 got pretty long recently.

That’s why I plan to introduce only some changes to the syntax in v2.0 to release it faster and open the possibility to add more features based on it in v2.1+.

@gdubicki gdubicki changed the title WIP: Proposal of the v2 config syntax WIP: Proposal of the v3 (?) config syntax May 23, 2021
@gdubicki gdubicki changed the title WIP: Proposal of the v3 (?) config syntax WIP: Proposal of the v4 (?) config syntax Jun 25, 2022
@sonarcloud
Copy link

sonarcloud bot commented Nov 22, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

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

Successfully merging this pull request may close these issues.

None yet

4 participants