Add first version of integration with systemd #43
Conversation
Save changes to branch. Working version (compiles and produces file - not a running service) - still needs environment variables. Added restart policy. Working version of unit file creation. Store work Added rough structure of systemd module / might be refactored into a crate at a later time.
|
Not sure what issue the clippy action has looks a bit like the known bug when creating a PR from a remote repo. |
| ); | ||
| // Get a mutable reference to the first element of the command array as we might need to | ||
| // add the package directory to this to make it an absolute path | ||
| let binary = match command.get_mut(0) { |
There was a problem hiding this comment.
This match could be changed to a .... ok_or_else? I believe...
There was a problem hiding this comment.
Unless I am mistaken that would not work with the early return?
|
Trying to build this right now I have one more comment: It'd be good if you could add a sentence to the README about the requirement for dbus headers etc. |
…omments related to that.
…ctively not doing anything.
|
|
||
| // If no external file was created check if the file currently exists | ||
| if !linked_unit_file { | ||
| debug!("Target file [{:?}] already exists", &target_file); |
There was a problem hiding this comment.
Isn't this in the wrong place? (Should be in the inner if?)
You never check wheter some file exists, here, do you? All you check if the option is "Some", no?
And can't you combine the two ifs?
if !linked_unit_file && target_file.exists {
There was a problem hiding this comment.
I think the code is correct as it is, I've added a comment to address your question above as well, which hopefully explains what I am doing..
There was a problem hiding this comment.
Actually, doing some more thinking and testing, I think the entire block should be reworked a little, but it works for now.
There was a problem hiding this comment.
I think I disagree.
When I see the debug statement "Target file already exists" it has not actually been checked yet if it exists.
There's nothing in that first if block other than the debug! statement.
This is what I'm proposing:
if !linked_unit_file && target_file.exists() {
// Target file already exists, remove if force is supplied, otherwise abort
debug!("Target file [{:?}] already exists", &target_file);
if force {
self.delete_unit_file(&unit_name)?;
} else {
return Err(anyhow!(
"Target unit file [{:?}] exists and force parameter was not specified.",
target_file
));
}
}There was a problem hiding this comment.
I have pushed a commit that should hopefully fix the entire write / delete file topic.
src/provider/states/starting.rs
Outdated
| } | ||
| } | ||
| } else { | ||
| warn!("No unit definitions found, not starting anything!"); |
There was a problem hiding this comment.
Missing context which pod we're talking about. May be worth starting a tracing span somewhere here in the state machine as well
There was a problem hiding this comment.
Yes, I have not paid special attention to log message context in this pull request in expectation of the larger log overhaul to add tracing spans and structured logging.
|
|
||
| // If no external file was created check if the file currently exists | ||
| if !linked_unit_file { | ||
| debug!("Target file [{:?}] already exists", &target_file); |
There was a problem hiding this comment.
I think I disagree.
When I see the debug statement "Target file already exists" it has not actually been checked yet if it exists.
There's nothing in that first if block other than the debug! statement.
This is what I'm proposing:
if !linked_unit_file && target_file.exists() {
// Target file already exists, remove if force is supplied, otherwise abort
debug!("Target file [{:?}] already exists", &target_file);
if force {
self.delete_unit_file(&unit_name)?;
} else {
return Err(anyhow!(
"Target unit file [{:?}] exists and force parameter was not specified.",
target_file
));
}
}# Conflicts: # src/bin/agent.rs # src/config/mod.rs # src/provider/mod.rs
This is a first version of systemd integration.
Working so far are:
I do not expect this version to be ready to merge, but it can serve as first basis for review.
I have more or less accidentally bundled a lot of clippy fixes in here as well, apologies!
fixes #11
fixes #9
fixes #6
fixes #5