Fix: Save configs on enable/disable plugins #917
Conversation
|
Sorry @CodeWithEmad, I forgot to ask you to open this PR on top of nightly. This is a breaking change, so I'd rather introduce it only in nightly/quince. |
CodeWithEmad
force-pushed
the
fix/save-configs
branch
from
040f2cd to
466ddec
Compare
|
Oh. The devil's in the detail, indeed. |
CodeWithEmad
force-pushed
the
fix/save-configs
branch
from
466ddec to
eb01b0f
Compare
tutor/commands/plugins.py
Outdated
| fmt.echo_info( | ||
| "You should now re-generate your environment with `tutor config save`." | ||
| ) | ||
| ENV_PATCHES_DICT.clear() |
There was a problem hiding this comment.
Not quite :) One of the advantages of having an event-driven API is that we don't need to import dependencies. In particular, we don't want to spread the use of ENV_PATCHES_DICT outside of its own module (tutor.plugins). So, we should try not to use that filter here.
Instead, we should add a callback to the PLUGIN_UNLOADED action. That callback will clear and re-compute the ENV_PATCHES_DICT. It will be called automatically every time a plugin is unloaded. Thus, we don't have to worry about it outside of its own module.
This is a bit difficult to explain... Am I making sense?
There was a problem hiding this comment.
Yes. Totally. You mean something like this:
@hooks.Actions.PLUGIN_UNLOADED.add()
def _clear_plugin_patches(plugin: str, root: str, _config: Config) -> None:
"""
After a plugin is disabled, we clear the patches.
"""
ENV_PATCHES_DICT.clear()There was a problem hiding this comment.
Yes. And the callback should also call a cache-filling function.
There was a problem hiding this comment.
I apologize, but I'm feeling lost again. The issue was resolved after I cleared the ENV_PATCHES_DICT, and the disable command is now functioning properly. If by "cache-filling" you mean executing .iterate() and updating the ENV_PATCHES_DICT, I believe that _update_enabled_plugins_on_unload will take care of that. However, if I am mistaken, I would appreciate some assistance.
There was a problem hiding this comment.
Yes, the disable command seems to work. But if you don't fill the ENV_PATCHES_DICT cache again, then patches will not be correctly applied. You can verify that by enabling a plugin which has patches. The environment will be re-generated, but I believe it will not include the plugin patches. Does that make sense?
There was a problem hiding this comment.
Looks like I need to spend more time on tutor codebase :)
before this, after enabling/disabling any plugins we should re-generate all files with tutor config save.
After disabling a plugin, ENV_PATCHES_DICT should be cleared and re-calculated.
CodeWithEmad
force-pushed
the
fix/save-configs
branch
from
eb01b0f to
aac35fd
Compare
| @click.pass_obj | ||
| def disable(context: Context, plugin_names: list[str]) -> None: | ||
| config = tutor_config.load_minimal(context.root) | ||
| @click.pass_context |
There was a problem hiding this comment.
Unless I'm mistaken, we no longer have to pass the context here, right? Same for the enable command above.
There was a problem hiding this comment.
Oh my bad, we need context.invoke.
|
Closed in favour of #957 |
Before this, we had to run
tutor config saveafter enable/disable a plugin.Closes openedx-unsupported/wg-developer-experience#176