Gracefully handle bad plugins?

[kdmccormick]

Reproduction Steps

1. Create a local plugin at $(tutor plugins printroot)/mybadplugin.py
2. Add some invalid Python syntax. Or, implement a hook handler in a way that causes an error, eg:

from tutor import hooks

# This will raise an exception, because `add_item` expects
# not two arguments, but one (a tuple, for this patch).
hooks.Filters.ENV_PATCHES.add_item(
    "openedx-dev-dockerfile-post-python-requirements",
    "RUN pip install requests"
)

4. Run tutor plugins enable mybadplugin, which should cause an exception (not surprisingly).
5. Run tutor plugins printroot. Notice that it exits nonzero and does not print your plugin root (this is the "bug").

Now, I'm not sure how or even if we should try to guard the CLI against all malformed plugins. Wrapping every command in a try-except would probably be overkill. On the other hand, it might be smart to insulate the tutor plugins ... CLI in particular from bad plugins, since the CLI might help the user in disabling/editing the bad plugin.

Acceptance

TBD

[regisb]

Just to clarify: the first time that we run tutor plugins enable mybadplugin the command is successful:

$ tutor plugins enable mybadplugin                                       
Plugin mybadplugin enabled                                                                                                                                                                    
Configuration saved to /home/regis/.local/share/tutor/config.yml                                                                                                                              
You should now re-generate your environment with `tutor config save`.

But any further CLI command makes a call to _convert_plugin_patches, which crashes:

$ tutor plugins printroot                                       [202/798]
Error applying filter 'env:patches': func=<function _add_my_python_requirements at 0x7f229f73da60> contexts=['plugins', 'app:mybadplugin']'                                                   
Error applying action 'plugins:loaded': func=<function _convert_plugin_patches at 0x7f229f966160> contexts=[]'                                                                                
Error applying action 'project:root:ready': func=<function _enable_plugins at 0x7f229f96c820> contexts=[]'                                                                                    
Traceback (most recent call last):                                                                                                                                                            
  File "/home/regis/venvs/tutor/bin/tutor", line 11, in <module>                                                                                                                              
    load_entry_point('tutor', 'console_scripts', 'tutor')()                                                                                                                                   
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/commands/cli.py", line 24, in main                                                                                            
    cli()  # pylint: disable=no-value-for-parameter                                                                                                                                           
  File "/home/regis/venvs/tutor/lib/python3.8/site-packages/click/core.py", line 1128, in __call__                                                                                            
    return self.main(*args, **kwargs)                                                                                                                                                         
  File "/home/regis/venvs/tutor/lib/python3.8/site-packages/click/core.py", line 1053, in main                                                                                                
    rv = self.invoke(ctx)                                                                                                                                                                     
  File "/home/regis/venvs/tutor/lib/python3.8/site-packages/click/core.py", line 1653, in invoke                                                                                              
    cmd_name, cmd, args = self.resolve_command(ctx, args)                                                                                                                                     
  File "/home/regis/venvs/tutor/lib/python3.8/site-packages/click/core.py", line 1700, in resolve_command                                                                                     
    cmd = self.get_command(ctx, cmd_name)                                                                                                                                                     
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/commands/cli.py", line 82, in get_command                                                                                     
    for command in self.iter_commands(ctx):                                                                                                                                                   
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/commands/cli.py", line 48, in iter_commands                                                                                   
    cls.ensure_plugins_enabled(ctx)                                                                                                                                                           
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/commands/cli.py", line 63, in ensure_plugins_enabled
    hooks.Actions.PROJECT_ROOT_READY.do(root=ctx.params["root"])
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/hooks/actions.py", line 78, in do
    callback.do(*args, context=context, **kwargs)
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/hooks/actions.py", line 30, in do
    self.func(*args, **kwargs)
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/config.py", line 300, in _enable_plugins
    enable_plugins(config)
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/config.py", line 273, in enable_plugins
    plugins.load_all(get_enabled_plugins(config))
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/plugins/__init__.py", line 77, in load_all
    hooks.Actions.PLUGINS_LOADED.do()
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/hooks/actions.py", line 78, in do
    callback.do(*args, context=context, **kwargs)
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/hooks/actions.py", line 30, in do
    self.func(*args, **kwargs)
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/plugins/__init__.py", line 24, in _convert_plugin_patches
    for name, content in patches:
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/hooks/filters.py", line 76, in iterate
    yield from self.apply([], *args, context=context, **kwargs)
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/hooks/filters.py", line 99, in apply
    value = callback.apply(value, *args, context=context, **kwargs)
  File "/home/regis/projets/overhang/repos/overhang/tutor/tutor/hooks/filters.py", line 33, in apply
    value = self.func(value, *args, **kwargs)
  File "/home/regis/.local/share/tutor-plugins/mybadplugin.py", line 6, in _add_my_python_requirements
    patches.add(
AttributeError: 'list' object has no attribute 'add'

In particular, the tutor plugins enable mybadplugin also fails. More annoyingly, the tutor plugins disable mybadplugin also fails, which means that it's not convenient to temporarily disable the broken plugin.

I'm not sure what's the right solution here. I don't think we should wrap the plugin loading step in try: except: ... because that would cause certain filter callbacks to be enabled, and not others.

[CodeWithEmad]

can we use some sort of validation mechanism for python plugins?
something like this:

def validate_plugin(plugin_path):
    try:
        # Check for syntax errors
        with open(plugin_path, "r") as plugin_file:
            exec(plugin_file.read())

    except SyntaxError as e:
        return False, f"Syntax error in plugin: {str(e)}"

    except ImportError as e:
        return False, f"Missing import in plugin: {str(e)}"

    except Exception as e:
        return False, f"Unknown error in plugin: {str(e)}"

    return True, ""

and on enable side maybe something like this:

def enable_plugin(plugin_name):
    plugin_path = os.path.join(tutor.plugins.printroot(), plugin_name + ".py")
    is_valid, error_message = validate_plugin(plugin_path)

    if not is_valid:
        print(f"Error: Plugin '{plugin_name}' is not valid: {error_message}")
        sys.exit(1)

    # Continue with enabling the plugin

[regisb]

I'm not a big fan of the exec(plugin_file.read()) thing... Following the Python philosophy, I'd rather ask forgiveness not permission. Thus something similar to:

try:
    load(name)
except Exception as e:
    ... handle errors here

But this is exactly the code that's in place right now. So I'm not 100% what we should do differently, and most importantly: why? Which issue are we trying to address exactly?