feat: Add ENV_SAVED Action

[xitij2000]

Adds an ENV_SAVED Action hook that allows hooking into the save mechanism and running custom code after saving an environemnt.

[Kelketek]

Left over print statement

[Kelketek]

The docs you add mention that modifying this config variable won't result in the configuration being resaved and that it should be treated as read-only.

However, the config object being passed into this function is handed to this function. I'm not sure what the lifetime is of this variable outside of the function, but presumably, the configuration could be modified by this function and affect later code execution, even if the result isn't saved.

Should we add in a bit of enforcement here, by cloning the object before passing it? If so, might we do an equality check between the config before and after the change and add a warning saying the changes will not be saved?

[xitij2000]

@Kelketek You're right that someone modifying the data here could cause issues so I've made it create a deepcopy. That said, I think it can still cause issues since the object will be cloned once and then passed to all hooks. If hook1 modifies it, hook2 might see the modified version, which isn't ideal. However, I don't see how I could change this in a backwards-compatible way.

Generally, wherever you want the value to be affected, we'd use a filter, and actions are like event handlers that can't affect the event data. If someone is modifying the config then they are asking for trouble.

I thought adding a warning would be overkill; however, it could lead to detecting subtle bugs earlier, so I think it would be nice to add that. I'll add that tomorrow.

[xitij2000]

@Kelketek I've added a check for changes to the config and also realized that the simple theme plugin is violating this since it created a copy of the config but not a deep copy so deeper modifications were being persisted. I've fixed that as well.

[Kelketek]

Awesome. Glad to see this is already proving helpful :)

[Kelketek]

@xitij2000

Update: Just noticed one thing after initial comment. Please remove the unused function I commented on. Consider the approval conditional on this change and the tests passing.

👍

• I tested this: Used a plugin that took advantage of the save hook, saw the warning when it modified config, saw the warning disappear when it didn't, saw it run.
• I read through the code
• I checked for accessibility issues
• Includes documentation
• Added to the Code Drift project board (for backports)

[Kelketek]

@xitij2000 Just saw this when I noticed the CI failing when it found this. It looks like it's unused, so I think it can be removed?

[xitij2000]

@Kelketek Yeah, I started implementing a more thorough compare mechanism that would show an actual diff, but realised it isn't worth it currently.

[Kelketek]

Got it, thanks! I see it's gone now and the build is green, so you're good to go, then :)

[xitij2000]

@regisb Could you have a look at this and see if this is something you'd want in tutor?

For our purposes, we are using this to run tasks that add additional content to the environment that can't be effectively managed using templates. Currently we're adding an enhanced save command that internally calls the tutor save command, however this way you can hook into the standard save command, and keep changes in sync when the config changes outside the save command.

[Faraz32123]

Hi @xitij2000, thanks for the PR.

I have looked at the code changes and I have some questions like what is the exact usecase for which we are modifying the tutor save command.

For our purposes, we are using this to run tasks that add additional content to the environment that can't be effectively managed using templates.

If you can provide more context, we might be able to provide a solution for your usecase.
OR If this PR is the last resort and usecase is strong(i.e. modifying the env folder after it is saved, could result in some unwanted changes that could affect the tutor setup), we need to document it well with its pros and cons. So, it would be helpful if we have the usecase and document it as well as an example if we are going this way.

[xitij2000]

Hi @xitij2000, thanks for the PR.

If you can provide more context, we might be able to provide a solution for your usecase. OR If this PR is the last resort and usecase is strong(i.e. modifying the env folder after it is saved, could result in some unwanted changes that could affect the tutor setup), we need to document it well with its pros and cons. So, it would be helpful if we have the usecase and document it as well as an example if we are going this way.

One use-case can be seen here: https://gitlab.com/opencraft/dev/tutor-contrib-grove/-/blob/main/tutorgrove/plugins/config/commands/grove_config_save.py?ref_type=heads#L59
And another here: https://gitlab.com/opencraft/dev/tutor-contrib-grove/-/blob/main/tutorgrove/plugins/simple_theme/plugin.py?ref_type=heads#L106

The use cases I see are as following:

• Allow pre-processing or post-processing the config file to add or remove extra context.
• To create or modify files in a way that can't be supported through patches and templates.
  • Particularly in this case, templates have a fixed path; we cannot have a single template be used to dynamically create multiple files in multiple locations. For example if we want to render a template "file.ini" to multiple locations based on settings in confi.yaml we can't do that with regular templates.

For our simple theme plugin people can configure multiple themes in config.yaml and those will be written to multiple locations for each theme. This can't be done using templates so we do it our own way and hooking into the save filter would mean that we could ensure that when the tutor config is saved the plugin can run its own code to perform actions that can't be done using templates.

Here are other example use cases I can envision:

• Downloading external assets into the environment on save.
• Uploading the environment directory to a remote location on save.
• Committing the environment to git on save to maintain a history.

[Faraz32123]

@xitij2000 Thanks for explaining the use cases.

That said, I have concerns about implementing this behavior by hooking into tutor save command via an ENV_SAVED action.

As tutor config save is a core command with very clear expectations i.e. persist configuration and refresh the environment. Modifying its behavior to run additional operations after the save, might introduce implicit side effects that may not be obvious to users and could be surprising or even risky, especially since tutor config save is often used in custom deployment scripts, CI, and recovery scenarios.

From a design standpoint, this feels like something that should be explicit and opt-in, rather than automatically triggered as part of save command. Personally, I would be more comfortable with this functionality being exposed as a separate command (or at least an explicitly invoked action), so users clearly understand when these extra steps are executed(as we will have no warnings for env folder change).
Given that this changes the semantics of a widely used core command and I’d prefer to keep tutor save minimal and predictable.

I think the idea itself is valuable — I just don’t think modifying the save command via an added action is the safest place to implement it.

Happy to continue discussing alternative approaches if you’d like.

[xitij2000]

@Faraz32123 Thanks for your detailed feedback! I understand your concerns. I think there are two ways I see to do what we'd like without using the hook and we are already doing one of those.

The first is to add another CLI command to do whatever operations we want. For instance, adding tutor custom-save or tutor local|dev|k8s do custom-command. The issue with this is that now simply installing the plugin is not enough you need to perform an extra step with each save.

The other option would be to evaluate what operations we need to perform on save and whether those can be accomplished with other new hooks or expanding the functionality of hooks themselves.

I'll look into this a bit more and continue this conversation.

[xitij2000]

@Faraz32123 I've looked at the places where we use this feature and for one of them (theming) it would require larger modifications to the way templates work in order to be implemented without this.
The other command handles splitting the merging the config yaml such that unique config like password etc are preserved in a separate file that is synced.

Re the concerns:

As tutor config save is a core command with very clear expectations i.e. persist configuration and refresh the environment.

I understand, however this would not change that, it would only allow doing additional operations that a plugin might need to do in order to persist it's own configuration. In fact what we are aiming to do by hooking into save is to persist config in ways that default tutor is not capable of doing, such as pushing config to s3.

For example if a plugin is designed to download certain assets from S3 and build them into the platform images, one would expect to configure the plugin's settings, and use launch or save and start to set up the instance with the plugin. However, currently you'd need to run a custom command. The mechanics of this command will be different depending on circumstances. For example if you want to do tutor ... launch then the save is implicit, and you cannot run any command after save and before launch. You would need to save the config, run your command and then launch.

Modifying its behavior to run additional operations after the save, might introduce implicit side effects that may not be obvious to users and could be surprising or even risky, especially since tutor config save is often used in custom deployment scripts, CI, and recovery scenarios.

I feel that the purpose of every plugin is to introduce side effects. These should be side effects that the user wants, such as installing xblocks or setting up a theme, or even starting a completely new service. Yes they could be surprising and risky if a plugin is poorly designed, however that is true of all plugin hooks.

From a design standpoint, this feels like something that should be explicit and opt-in, rather than automatically triggered as part of save command.

It is explicit and opt-in. A user needs to install a plugin and enable it. Plugins can already hook into the tutor ... do command and when a docker compose command is executed, or when the core is ready to load custom kinds of plugins. I don't see this as too much different from that.

---

That said I can understand that the existence of existing hooks might not justify the creation of even more hooks. If this still feels risky then I think the best path for us will be to continue using wrapper commands as we do now.

[Kelketek]

@xitij2000 @Faraz32123

Just to chime in here:

One thing I requested when reviewing this PR is a loud warning if the config was changed by a script in the hook in any way, as using this hook to modify the config is considered a bug. This allows the sanctity of the config to be preserved while still allowing the hooks to read the final result for outside actions. @xitij2000 implemented this in the PR and immediately found a relevant bug that would have violated this principle in our plugin. So we know it works for mitigating unintended config changes.

This could be upgraded to an error, if need be, or we could change something about hooks to force deep cloning of the config for the save action, so that any changes would automatically be discarded. My preference is to keep it as a warning, or make it an error as a fallback.

[xitij2000]

@Kelketek I agree, however I can also imagine it being useful to modify the config before save. For instance, if a plugin's schema changes they might want to automatically migrate the config to the new scheme. That said if we enforce no changes to the config, plugin authors can just print a message asking users to upgrade the config and print out the updated config to copy and paste.