fix: better logging during plugins disable

When disable a plugin that set config entried, such as the minio plugin, tutor was logging the following:

    Disabling plugin minio...
        Removed config entry OPENEDX_AWS_ACCESS_KEY=openedx
        Removed config entry OPENEDX_AWS_SECRET_ACCESS_KEY={{ MINIO_AWS_SECRET_ACCESS_KEY }}
        Plugin disabled

The config values were not rendered during printing, which is a shame, because
the whole point of this log line is to warn users of passwords/secrets that are
being removed. Here, we make sure that the config values are properly rendered.
The new logs are now:

    Disabling plugin minio...
        Removing config entry OPENEDX_AWS_ACCESS_KEY=openedx
        Removing config entry OPENEDX_AWS_SECRET_ACCESS_KEY=64vpCVLxhDxBuNjakSrX4CQg
        Plugin disabled

CHANGELOG.md

@@ -5,6 +5,7 @@ Note: Breaking changes between versions are indicated by "💥".
 ## Unreleased
 - [Bugfix] Fix double pulling mongodb image when upgrading from Koa to Lilac.
 
+- [Improvement] Better logging during `plugins disable`.
 - [Bugfix] Fix "upstream sent too big header" error during login of existing users after a Koa to Lilac upgrade.
 - [Feature] Added the ability to skip `config.yml` file modification while running `tutor config save` command with `-e` or `--env-only` flag.
 

tests/test_plugins.py

@@ -55,31 +55,52 @@ class PluginsTests(unittest.TestCase):
         with patch.object(plugins, "is_installed", return_value=False):
             self.assertRaises(exceptions.TutorError, plugins.enable, config, "plugin1")
 
-    def test_disable(self) -> None:
+    @patch.object(
+        plugins.Plugins,
+        "iter_installed",
+        return_value=[
+            plugins.DictPlugin(
+                {
+                    "name": "plugin1",
+                    "version": "1.0.0",
+                    "config": {"set": {"KEY": "value"}},
+                }
+            ),
+            plugins.DictPlugin(
+                {
+                    "name": "plugin2",
+                    "version": "1.0.0",
+                }
+            ),
+        ],
+    )
+    def test_disable(self, _iter_installed_mock: Mock) -> None:
         config: Config = {"PLUGINS": ["plugin1", "plugin2"]}
         with patch.object(fmt, "STDOUT"):
-            plugins.disable(config, "plugin1")
+            plugin = plugins.get_enabled(config, "plugin1")
+            plugins.disable(config, plugin)
         self.assertEqual(["plugin2"], config["PLUGINS"])
 
-    def test_disable_removes_set_config(self) -> None:
-        with patch.object(
-            plugins.Plugins,
-            "iter_enabled",
-            return_value=[
-                plugins.DictPlugin(
-                    {
-                        "name": "plugin1",
-                        "version": "1.0.0",
-                        "config": {"set": {"KEY": "value"}},
-                    }
-                )
-            ],
-        ):
-            config: Config = {"PLUGINS": ["plugin1"], "KEY": "value"}
-            with patch.object(fmt, "STDOUT"):
-                plugins.disable(config, "plugin1")
-            self.assertEqual([], config["PLUGINS"])
-            self.assertNotIn("KEY", config)
+    @patch.object(
+        plugins.Plugins,
+        "iter_installed",
+        return_value=[
+            plugins.DictPlugin(
+                {
+                    "name": "plugin1",
+                    "version": "1.0.0",
+                    "config": {"set": {"KEY": "value"}},
+                }
+            ),
+        ],
+    )
+    def test_disable_removes_set_config(self, _iter_installed_mock: Mock) -> None:
+        config: Config = {"PLUGINS": ["plugin1"], "KEY": "value"}
+        plugin = plugins.get_enabled(config, "plugin1")
+        with patch.object(fmt, "STDOUT"):
+            plugins.disable(config, plugin)
+        self.assertEqual([], config["PLUGINS"])
+        self.assertNotIn("KEY", config)
 
     def test_none_plugins(self) -> None:
         config: Config = {plugins.CONFIG_KEY: None}

tutor/commands/plugins.py

@@ -60,12 +60,16 @@ def enable(context: Context, plugin_names: List[str]) -> None:
 @click.pass_obj
 def disable(context: Context, plugin_names: List[str]) -> None:
     config = tutor_config.load_user(context.root)
-    if "all" in plugin_names:
-        plugin_names = [plugin.name for plugin in plugins.iter_enabled(config)]
-    for plugin_name in plugin_names:
-        plugins.disable(config, plugin_name)
-        delete_plugin(context.root, plugin_name)
-
+    disable_all = "all" in plugin_names
+    for plugin in plugins.iter_enabled(config):
+        if disable_all or plugin.name in plugin_names:
+            fmt.echo_info("Disabling plugin {}...".format(plugin.name))
+            for key, value in plugin.config_set.items():
+                value = tutor_env.render_unknown(config, value)
+                fmt.echo_info("    Removing config entry {}={}".format(key, value))
+            plugins.disable(config, plugin)
+            delete_plugin(context.root, plugin.name)
+            fmt.echo_info("    Plugin disabled")
     tutor_config.save_config_file(context.root, config)
     fmt.echo_info(
         "You should now re-generate your environment with `tutor config save`."

tutor/plugins.py

@@ -411,19 +411,22 @@ def enable(config: Config, name: str) -> None:
     enabled.sort()
 
 
-def disable(config: Config, name: str) -> None:
-    fmt.echo_info("Disabling plugin {}...".format(name))
-    for plugin in Plugins(config).iter_enabled():
-        if name == plugin.name:
-            # Remove "set" config entries
-            for key, value in plugin.config_set.items():
-                config.pop(key, None)
-                fmt.echo_info("    Removed config entry {}={}".format(key, value))
-    # Remove plugin from list
+def disable(config: Config, plugin: BasePlugin) -> None:
+    # Remove plugin-specific set config
+    for key in plugin.config_set.keys():
+        config.pop(key, None)
+
+    # Remove plugin from list of enabled plugins
     enabled = enabled_plugins(config)
-    while name in enabled:
-        enabled.remove(name)
-    fmt.echo_info("    Plugin disabled")
+    while plugin.name in enabled:
+        enabled.remove(plugin.name)
+
+
+def get_enabled(config: Config, name: str) -> BasePlugin:
+    for plugin in iter_enabled(config):
+        if plugin.name == name:
+            return plugin
+    raise ValueError("Enabled plugin {} could not be found.".format(plugin.name))
 
 
 def iter_enabled(config: Config) -> Iterator[BasePlugin]: