Create Config_check.service - #9
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new systemd service file Config_check.service to perform configuration validation after the WPEFramework service starts. However, the service file contains multiple critical violations of the repository's systemd service coding guidelines that must be addressed before merging.
Changes:
- Adds new systemd service file
Config_check.servicewith dependency on Wpeframework.service
| [Service] | ||
| Type=simple | ||
| ExecStart=config_check --option | ||
| ExecStartPost=mkdir -p /var/run/config_checker |
There was a problem hiding this comment.
ExecStartPost must use an absolute path. The mkdir command needs to be specified with its full path.
Change from:
ExecStartPost=mkdir -p /var/run/config_checker
To:
ExecStartPost=/bin/mkdir -p /var/run/config_checker
| After=Wpeframework.service | ||
|
|
||
| [Service] | ||
| Type=simple |
There was a problem hiding this comment.
Using Type=simple is not recommended. Prefer Type=notify for long-running services or Type=oneshot for one-time tasks. Type=simple doesn't guarantee service readiness and can cause race conditions.
Based on the service behavior (creates a directory which suggests persistent state), this should likely be Type=oneshot. If config_check is a long-running daemon that can send readiness signals, use Type=notify instead.
| Type=simple | ||
| ExecStart=config_check --option | ||
| ExecStartPost=mkdir -p /var/run/config_checker | ||
| RemainAfterExit=yes |
There was a problem hiding this comment.
RemainAfterExit=yes is only valid with Type=oneshot, not with Type=simple. This configuration is invalid and will cause errors.
The service must be changed to Type=oneshot if RemainAfterExit=yes is needed. RemainAfterExit tells systemd that the service's "effect" persists after the process exits, which only makes sense for one-time operations (oneshot), not for running processes (simple or notify).
| [Service] | ||
| Type=simple | ||
| ExecStart=config_check --option | ||
| ExecStartPost=mkdir -p /var/run/config_checker | ||
| RemainAfterExit=yes |
There was a problem hiding this comment.
Missing timeout configuration. Both TimeoutStartSec and TimeoutStopSec should be specified to prevent indefinite waits and ensure clear expectations.
Add to the [Service] section:
TimeoutStartSec=30
TimeoutStopSec=10
TimeoutStartSec should be 30 seconds or less, and TimeoutStopSec should be 10 seconds or less. If values greater than 90 seconds are needed, proper justification is required.
| [Unit] | ||
| After=Wpeframework.service | ||
|
|
||
| [Service] | ||
| Type=simple | ||
| ExecStart=config_check --option | ||
| ExecStartPost=mkdir -p /var/run/config_checker | ||
| RemainAfterExit=yes |
There was a problem hiding this comment.
Missing [Install] section. Services must include an [Install] section with WantedBy=multi-user.target to ensure the service starts automatically during boot.
Add at the end of the file:
[Install]
WantedBy=multi-user.target
Without this section, the service won't start automatically at boot and won't be properly integrated into the boot sequence.
| [Unit] | ||
| After=Wpeframework.service |
There was a problem hiding this comment.
Missing Description field in the [Unit] section. Every service must have a clear, descriptive Description= that explains what the service does. This helps others understand the service's purpose.
For example:
Description=Configuration Validation Service for System Settings
|
|
||
| [Service] | ||
| Type=simple | ||
| ExecStart=config_check --option |
There was a problem hiding this comment.
ExecStart must use an absolute path instead of a relative command. Systemd does not inherit environment variables like $PATH, so bare commands will fail.
Change from:
ExecStart=config_check --option
To (example):
ExecStart=/usr/bin/config_check --option
No description provided.