feat: Add ENV_SAVED Action #1302
Conversation
tutor/env.py
Outdated
| save_all_from(src, os.path.join(root_env, dst), config) | ||
|
|
||
| upgrade_obsolete(root) | ||
| # print("asdasd") |
tutor/env.py
Outdated
|
|
||
| upgrade_obsolete(root) | ||
| # print("asdasd") | ||
| hooks.Actions.ENV_SAVED.do(root_env, config) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Awesome. Glad to see this is already proving helpful :)
Adds an ENV_SAVED Action hook that allows hooking into the save mechanism and running custom code after saving an environemnt.
xitij2000
force-pushed
the
kshitij/env-saved-action
branch
from
6225baa to
538a574
Compare
|
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. 👍
|
tutor/env.py
Outdated
| ) | ||
|
|
||
|
|
||
| def deepcompare(val1, val2): |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@Kelketek Yeah, I started implementing a more thorough compare mechanism that would show an actual diff, but realised it isn't worth it currently.
There was a problem hiding this comment.
Got it, thanks! I see it's gone now and the build is green, so you're good to go, then :)
2 participants
Adds an ENV_SAVED Action hook that allows hooking into the save mechanism and running custom code after saving an environemnt.