Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
modules/supervisord: init, modules/openssh: init #203
base: master
Are you sure you want to change the base?
modules/supervisord: init, modules/openssh: init #203
Changes from 24 commits
dc61ae1
3a02d09
88b5731
a00dc02
7834088
16107f5
86248da
4d93571
3278a9c
269f6d1
6e9389c
0faa47c
bd44a10
c39cb39
42b95bd
06d245c
5ea968c
f861f45
b416b58
2f92f24
5278b3d
edb3247
82c42b3
56f4e44
ca13b29
01a7447
9222c03
ba1526a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably add the
-e
flag so log output goes to stderr, which supervisord can actually pick up (unlike syslog).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking out loud: Does it make sense to expose
script
andcommand
options like this? Thecommand
option looks to me like an unnecessary option if there is alreadyscript
. Also not sure iftypes.lines
is a good fit forscript
because that means that its values will be merged instead of overwritten when this option is defined multiple times.Another idea to be more user-friendly and more error-prone (because I am sure there will be a lot of people forgetting to use
exec
in front of their commands): How about apreStart
/initScript
/.. and acommand
option and the module builds a script likeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
command
is the direct way of specifying what to run (systemd.services.<name>.serviceConfig.ExecStart
) and works fine for simple services that don't require much setup. I think there's no need to involve a script when settingcommand
directly is sufficient (also avoid theexec
pitfalls).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do think about splitting set up logic from the actual command? I like the concept of ExecPreStart and similar lifecycle hooks of systemd that we could just implement for the user.
If you don't like it, please put a note about the necessity of
exec
in the description ofscript
and clarify when each of the options are recommended to use and that not both options can be set simultaneously, ideally throwing an assertion error there to avoid unexpected behaviour.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it simple and not create illusions of lifecycle hooks that don't actually exist in supervisord. Scripts don't have to include an
exec
(like some kind ofwait
setup with custom cleanup logic) so I'm hesitant to add assertions to enforce this incorrect assumption.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you set e.g.
signals like SIGTERM will not be forwarded by default to this process or is supervisord doing some magic there? These signals need to be forwarded to the actual main process.
Regarding lifecycle hooks: I agree that that this looks like an illusion but I am just thinking about improving UX through providing useful abstractions