config: forbid setting core.no_scm outside repo

[ghost]

Raise a ConfigError when setting core.no_scm on a global or system level.

Close #3427

• ❗ I have followed the Contributing to DVC checklist.

• 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.

• ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

[shcheklein]

I think we should be doing this check on the schema level. Even if I edit config manually we should be raising an exception.

[ghost]

@shcheklein , roger that. 🙂
what would be the way to go? I didn't find any similar implementation.

The current validation process happens after merging the configuration files:
https://github.com/iterative/dvc/blob/c1a32ff1bfb0852c575e101668da42229ab810fd/dvc/config.py#L275-L287

We could validate each configuration file separately, something like:

diff --git a/dvc/config.py b/dvc/config.py
index 6e1ccd7f..eba2768a 100644
--- a/dvc/config.py
+++ b/dvc/config.py
@@ -281,10 +281,12 @@ class Config(dict):
         conf = {}
         for level in self.LEVELS:
             if level in self.files:
-                _merge(conf, self.load_one(level))
+                level_conf = self.load_one(level)
 
-        if validate:
-            conf = self.validate(conf)
+                if validate:
+                    level_conf = self.validate(level_conf, level)
+
+                conf = _merge(conf, level_conf)

What do you think?

[shcheklein]

@MrOutis it's a strong assumption. What if we have a section with two required fields and they are defined separately now in two configs. After your change it will start failing, right?

Also, question is how we define this rule on the SCHEMA level?

[karajan1001]

In my opion, only those config in lower levels can affect the validation process .Maybe something like

         conf = {}
         for level in self.LEVELS:
             if level in self.files:
                _merge(conf, self.load_one(level))
                if validate:
                   self.validate(conf)

I had got a simliar problem in pr3628

[efiop]

Closing as stale 🙁

[ribitskiyb]

@shcheklein, regarding

I think we should be doing this check on the schema level.

Schema and levels (system, global, etc.) address different config content-related concerns -- validation and collection, respectively. Making the schema aware of the existence of levels seems like an unnecessary complexification of it.
What if instead to introduce level-specific checks as a separate "concept" in the codebase?

[shcheklein]

Making the schema aware of the existence of levels seems like an unnecessary complexification of it.

It might be, @ribitskiyb . I was not trying to dictate any specific solution, to be honest. Rather convey the problem and brainstorm together here.

What if instead to introduce level-specific checks as a separate "concept" in the codebase?

it depends, I guess. How would it look like? How would the access interface look like?

[ribitskiyb]

@shcheklein how about simply

1. adding a dict of level name to level-specific constraints (not sure yet if it can also be done with voluptuous)

2. checking the contents of level config just before its being merged
   dvc/config.py
   

[shcheklein]

@ribitskiyb sounds good to me! Should if be part of the load_one logic?

[ribitskiyb]

@shcheklein yea would be reasonable to put the check itself inside load_one

@@ -249,6 +249,9 @@ class Config(dict):
 
     # In the order they shadow each other
     LEVELS = ("system", "global", "repo", "local")
+    PER_LEVEL_PROHIBITED_OPTIONS = make_per_level_prohibited_options_config(
+        {"core.no_scm": {"system", "global"}}
+    )
 
     CONFIG = "config"
     CONFIG_LOCAL = "config.local"
@@ -368,6 +371,17 @@ class Config(dict):
         for key in COMPILED_SCHEMA.schema:
             conf.setdefault(key, {})
 
+        # Check if violates level-specific constraints
+        forbidden_options = self.PER_LEVEL_PROHIBITED_OPTIONS.get(level, ())
+        for jsonpath in forbidden_options:
+            if not jsonpath.find(conf):
+                continue
+            raise ConfigError(
+                "Setting {option} is forbidden on the {level} level".format(
+                    option=str(jsonpath), level=level,
+                )
+            )
+
         return conf