Skip to content
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

Drop systemd_pre #926

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Drop systemd_pre #926

merged 1 commit into from
Jul 19, 2023

Conversation

kkaempf
Copy link
Contributor

@kkaempf kkaempf commented Jul 19, 2023

It does not exist 🤦

It does not exist 🤦

Signed-off-by: Klaus Kämpf <kkaempf@suse.de>
@kkaempf kkaempf requested a review from a team as a code owner July 19, 2023 14:17
@kkaempf kkaempf enabled auto-merge (rebase) July 19, 2023 14:29
Copy link
Contributor

@anmazzotti anmazzotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. However I do wonder why we use pre and post. Shouldn't service_add_post and service_del_preun be enough?

Also regarding the systemd_preun, I think it actually prevents the restart if I read this doc. I think systemd_postun_with_restart should be used instead.

Anyway not related to this PR.

@kkaempf kkaempf merged commit bc6c269 into rancher:main Jul 19, 2023
5 checks passed
@kkaempf
Copy link
Contributor Author

kkaempf commented Jul 19, 2023

Well, we just follow the openSUSE systemd packaging guidelines (resp. the Fedora one's) here 🤷🏻

@kkaempf kkaempf deleted the drop-systemd_pre branch July 19, 2023 16:54
@kkaempf
Copy link
Contributor Author

kkaempf commented Jul 19, 2023

Also regarding the systemd_preun, I think it actually prevents the restart if I read this doc. I think systemd_postun_with_restart should be used instead.

Yes, I thought about this. But since we're not doing package-based updates, this is not relevant and I decided to use systemd_preun (which, strictly speaking, wouldn't be needed either).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants