Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve restart-commands: pass list of changed settings that's triggering the restart #1389

Closed
etungsten opened this issue Mar 12, 2021 · 2 comments
Assignees
Labels
type/enhancement New feature or request
Milestone

Comments

@etungsten
Copy link
Contributor

etungsten commented Mar 12, 2021

^ I think it could be better to improve the way restart-commands are run so that they're always given the list of settings that have changed, rather than the setting name having to be in the restart-command itself. That way we can handle dynamic settings, and not have to inspect the system to see what changed.
For example, right now, services.motd could have a restart-command like my-command settings.motd because we know services.motd is only associated with one setting, but services.host-containers is associated with any host container name under settings.host-containers, so we can't pass (hardcode) a single setting. Instead, if the command was given the changed settings, it could alter its behavior based on what changed, like in this case where we need to know which host container changed. (It could also give us the ability to re-use restart command helpers more often, if they can branch on setting.)

Originally posted by @tjkirch in #1230 (comment)

What I'd like:
For restart-command helpers like host-containers to know which settings are being changed so it can determine which host-containers needs to be restarted.
Currently host-containers are not automatically restarted if their settings changed. Users have to explicitly disable and re-enable their host-container via the enabled setting to see their new host-container settings enacted.

It would be great if thar-be-settings can optionally pass the settings being changed to the restart-command being invoked. The restart-command helper would then be able to determine exactly what needs to be done to enact the new setting changes.

Any alternatives you've considered:
host-containers can try to infer what settings has changed by checking current settings against previously generated environment files. But this approach is potentially nondeterministic and unelegant.

@etungsten etungsten added priority/p1 type/enhancement New feature or request labels Mar 12, 2021
@jhaynes jhaynes assigned sanu11 and unassigned jhaynes Mar 22, 2021
@jhaynes jhaynes added this to the next milestone Mar 24, 2021
@jhaynes jhaynes added the status/research This issue is being researched label Mar 25, 2021
@sanu11 sanu11 added status/in-progress This issue is currently being worked on and removed status/research This issue is being researched labels Apr 2, 2021
@tjkirch tjkirch modified the milestones: 1.0.8, next Apr 8, 2021
@bcressey
Copy link
Contributor

@sanu11 is this done, or if not what's left to do?

@etungsten
Copy link
Contributor Author

I believe this issue has been resolved. Restart command helpers now have access to the list of changed settings via environment variables. The additional work for handling host container restarts when settings change is being tracked in #1531.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants