Create working_systemd.service - #6
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new systemd unit file to define and enable a working_systemd daemon service, including dependencies, startup settings, and install target linkage.
Changes:
- Introduces
systemd/system/working_systemd.servicewithType=notifyand an absolute-pathExecStart. - Configures dependency relationships (
After=,Requires=,Wants=) and boot enablement (WantedBy=multi-user.target). - Sets explicit startup/shutdown timeouts.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| Requires=database.service | ||
| Wants=logger.service | ||
| [Service] | ||
| Type=notify |
There was a problem hiding this comment.
This service uses Type=notify, which is inconsistent with the rest of the codebase. All existing services use Type=oneshot with RemainAfterExit=yes. Consider changing to Type=oneshot to maintain consistency with the established pattern unless there is a specific requirement for this service to use notify-based readiness signaling.
| [Install] | ||
| WantedBy=multi-user.target |
There was a problem hiding this comment.
The [Install] section is included, but most services in this codebase (except wpeframework-powermanager.service) do not have an [Install] section. Consider whether this service needs to be enabled via systemctl enable, or whether it should follow the convention of the majority of services in this codebase which omit this section.
| TimeoutStartSec=30 | ||
| TimeoutStopSec=10 |
There was a problem hiding this comment.
The timeout directives TimeoutStartSec and TimeoutStopSec are not used in any existing services in the codebase. All services rely on systemd's default timeout values. Consider removing these directives to maintain consistency with the codebase conventions, unless there is a specific requirement for custom timeout values.
| Wants=logger.service | ||
| [Service] | ||
| Type=notify | ||
| ExecStart=/usr/bin/mydaemon --config /etc/mydaemon.conf |
There was a problem hiding this comment.
According to the custom coding guidelines, for long-running services with Type=notify, an ExecReload directive should be added to support configuration reloads without service restarts. This reduces downtime and allows configuration changes to be applied with minimal interruption. Consider adding ExecReload with an appropriate signal (typically SIGHUP) for the daemon to reload its configuration.
| [Service] | ||
| Type=notify | ||
| ExecStart=/usr/bin/mydaemon --config /etc/mydaemon.conf | ||
| TimeoutStartSec=30 | ||
| TimeoutStopSec=10 |
There was a problem hiding this comment.
The Restart directive is missing. According to the custom coding guidelines, you should explicitly configure restart behavior. For critical services, keep the default Restart=no (no directive needed), but for non-critical services, consider adding Restart=on-failure with RestartSec=5 to enable automatic recovery from crashes. If this is a critical service, consider adding a comment documenting that Restart=no is intentional.
No description provided.