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

{LINTER_PREFIX}_ENABLED configs for more flexible extension of (base) configs #3490

Open
pjungermann opened this issue Apr 15, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@pjungermann
Copy link
Contributor

pjungermann commented Apr 15, 2024

Is your feature request related to a problem? Please describe.
Assuming, you have a base configuration that is used by a lot of different repositories (e.g., as an organization). At each repository, you have customizations specific to this repository.

Additionally, you could consider having different base configs per flavor and a root config for these, etc.

For these cases, you can use EXTENDS to extend another config. Each key you provide at your local config will overwrite the config value from the extended config(s). Configs are extended in order and the same overwrites get applied.

At the MegaLinter config, you have multiple array type config properties like ENABLE, DISABLE, ENABLE_LINTERS, DISABLE_LINTERS, and more.
When you provide a value at the extending config, you will overwrite the whole value, replacing one array with the other one.

.mega-linter-base.yml

ENABLE_LINTERS:
  - LINTER_A

.mega-linter.yml

EXTENDS: .mega-linter-base.yml
ENABLE_LINTERS:
  - LINTER_B

merged:

ENABLE_LINTERS:
  - LINTER_B

desired outcome:

ENABLE_LINTERS:
  - LINTER_A
  - LINTER_B

Esp. for the config properties related to enabling/disabling linters, this can be a challenge. It requires the extending config to be aware of the value it's overwriting as well as changes to this value over time.

E.g., if I add LINTER_C to the base config, it will not be enabled until all places overwriting ENABLE_LINTERS add it to their config as well.

Describe the solution you'd like
I propose to use linter-specific configs as an alternative to the array-typed config properties like

REPOSITORY_CHECKOV_ENABLED: true
REPOSITORY_TRIVY_ENABLED: true

instead of

ENABLE_LINTERS:
  - REPOSITORY_CHECKOV
  - REPOSITORY_TRIVY

This allows to selectively enable or disable linters at each level without fully replacing the whole configuration part.

Similarly, you could support

REPOSITORY_ENABLED: true

to enable a whole category.

Config properties like REPOSITORY_CHECKOV_ENABLED would default to the value of REPOSITORY_ENABLED.

Describe alternatives you've considered
Alternatively, extending configs need to be adjusted for every change and changes cannot be rolled out centrally.

Additional context

Other config properties?
There are more array-typed config properties like {LINTER_PREFIX}_FILE_EXTENSIONS, {LINTER_PREFIX}_FILE_NAME_REGEX, {LINTER_PREFIX}_PRE_COMMANDS, etc.

However, in contrast to the config properties above, they usually represent a value that forms a unit while at the ENABLE/DISABLE config properties, it is rather an enumeration.

What to do with the previous configuration?
I would suggest deprecating them and removing them as part of the next major release.

Until then, there needs to be a clear hierarchy to achieve deterministic outcomes.
Hereby, I would suggest giving precedence to {LINTER_PREFIX}_ENABLED over whatever is configured at ENABLE, DISABLE, ENABLE_LINTERS, and DISABLE_LINTERS and use an empty array as their default value.

@nvuillam
Copy link
Member

The idea is interesting, but 99% of .mega-linter.yml current configs are using ENABLE , DISABLE, ENABLE_LINTERS and DISABLE_LINTERS, and do not need EXTENDS capabilities, so such update should be incremental to the current behaviour, not deprecate it :)

Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label May 18, 2024
@pjungermann
Copy link
Contributor Author

remove stale

@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label May 22, 2024
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jun 21, 2024
@pjungermann
Copy link
Contributor Author

remove stale

@bdovaz
Copy link
Collaborator

bdovaz commented Jun 24, 2024

@pjungermann I would say that this need I solved in #3469 via CONFIG_PROPERTIES_TO_APPEND:

https://megalinter.io/latest/config-variables/

@pjungermann
Copy link
Contributor Author

@bdovaz While I already use it as a workaround - and I'm very happy that it exists -, I would prefer this solution here.

Of course, CONFIG_PROPERTIES_TO_APPEND can be used as a workaround and for other array-based use cases. As written above, I'm using it already.
Still, it is an overly complex solution for the problem and still requires this setting to be introduced and maintained at local config files. This cannot be controlled easily and requires education for people using it.
On the other hand, it is more compatible with previous design decisions, it's more backward compatible.

E.g., if I want to disable linter LINTER_A on org level I would configure

DISABLE_LINTERS:
  - LINTER_A

Other places may already use DISABLE_LINTERS and hence, overwrite it. Until they introduce

CONFIG_PROPERTIES_TO_APPEND:
  - DISABLE_LINTERS

DISABLE_LINTERS:
  - LINTER_B

If you have many places to adjust that does not scale well. Of course, you could argue this is an initial effort only.
Also, it is not very intuitive.

Alternatively, having an approach like

LINTER_A_ENABLED: false

and at other places

LINTER_B_ENABLED: true

It would even allow changing the default.

More importantly, though, it does not require any awareness of the org-level defaults and changes to it over time.

@pjungermann
Copy link
Contributor Author

PS: I wanted to suggest a change eventually. I didn't get to it yet.

@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants