-
Notifications
You must be signed in to change notification settings - Fork 225
Prevent service being started/enabled on install for rpms #1335
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
base: main
Are you sure you want to change the base?
Prevent service being started/enabled on install for rpms #1335
Conversation
|
I am uncertain whether overrides will deep-merge the |
|
Hmm this won't restart on an upgrade either. I will fix that tomorrow night. The original form of the script but using systemd try-restart instead should do the trick. |
57af489 to
6336052
Compare
This brings behaviour inline with usual expectations for rpm packages which usually leave this up to the system administrator. Create a separate postinstall-rpm.sh specifically for rpms and set this as an override for the postinstall script for rpm packages.
6336052 to
f47fdd0
Compare
| component: "packaging" | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: "Prevent service being started/enabled upon install for rpms" |
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.
this changelog doesn't reflect the changes. You seem to be checking for systemd presence in the system.
Please clarify this changelog.
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 believe this is an accurate description of the change in behaviour for end users - please also refer to #1334 for more expansion on the motivation here.
All of the logic in the scripts that I am adding already exists in the base postinstall.sh scripts - my change merely creates specific versions with slightly different outcomes for rpms.
For example, for the plain otelcol distribution, the diff between the existing script and my rpm specific override:
--- a/distributions/otelcol/postinstall.sh
+++ b/distributions/otelcol/postinstall-rpm.sh
@@ -7,10 +7,9 @@ if command -v systemctl >/dev/null 2>&1; then
if [ -d /run/systemd/system ]; then
systemctl daemon-reload
fi
- systemctl enable otelcol.service
if [ -f /etc/otelcol/config.yaml ]; then
if [ -d /run/systemd/system ]; then
- systemctl restart otelcol.service
+ systemctl try-restart otelcol.service
fi
fi
fiIf you think there's a better approach then I'm happy to adjust.
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.
OK I see. There is boilerplate on that script you can strip away imo ; remove the check for the config file presence. Is checking for /run/systemd/system presence even necessary?
Figured out how to build the rpms locally now - so yes the config I've used here does deep merge the scripts block: |
|
The current CI failure looks unrelated to my change, and probably transient. @mowies, any chance you could please re-trigger? |
|
I reran CI |
|
Looks better now, thanks 👍 |
| if [ -d /run/systemd/system ]; then | ||
| systemctl daemon-reload | ||
| fi | ||
| if [ -f /etc/otelcol-contrib/config.yaml ]; then |
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.
why are you checking for this file presence?
This brings behaviour inline with usual expectations for rpm packages which usually leave this up to the system administrator.
Create a separate postinstall-rpm.sh specifically for rpms and set this as an override for the postinstall script for rpm packages.
This resolves #1334.