Create config_check.service - #7
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request creates a new systemd service file config_check.service that runs after Wpeframework.service. The service appears to perform configuration checking and creates a runtime directory. However, the service file has several critical issues that violate the repository's systemd coding guidelines and would prevent the service from functioning correctly.
Changes:
- Added
config_check.servicesystemd unit file with basic service configuration
| [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 with WantedBy=multi-user.target. Without this section, the service won't start automatically at boot. Add an [Install] section with WantedBy=multi-user.target to ensure auto-start at boot for standalone services.
| [Unit] | ||
| After=Wpeframework.service |
There was a problem hiding this comment.
Missing Description= field in the [Unit] section. A clear, descriptive Description= should explain what the service does to help others understand its purpose. Avoid generic descriptions or repeating the filename.
| ExecStart=config_check --option | ||
| ExecStartPost=mkdir -p /var/run/config_checker |
There was a problem hiding this comment.
ExecStart and ExecStartPost directives must use absolute paths. Relative paths and bare commands are not allowed. Specify the full absolute path for 'config_check' and 'mkdir' to avoid reliance on environment variables like $PATH, which systemd does not inherit.
| After=Wpeframework.service | ||
|
|
||
| [Service] | ||
| Type=simple |
There was a problem hiding this comment.
Type=simple should be avoided. 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 where dependent services start before this service is truly operational. If this is a long-running daemon, use Type=notify with sd_notify() support. If this is a one-time task, use Type=oneshot.
| 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 systemd to reject the service. If the service creates persistent state (like the directory in ExecStartPost), use Type=oneshot with RemainAfterExit=yes. If it's a long-running daemon, use Type=notify or Type=simple without RemainAfterExit.
| [Service] | ||
| Type=simple | ||
| ExecStart=config_check --option | ||
| ExecStartPost=mkdir -p /var/run/config_checker | ||
| RemainAfterExit=yes |
There was a problem hiding this comment.
Missing TimeoutStartSec and TimeoutStopSec directives. Always specify both timeout values to prevent indefinite waits and ensure clear expectations. 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.
No description provided.