#300 [frontend][keygen] avoid using initscripts (/sbin/service)
Merged 5 years ago by msuchy. Opened 5 years ago by praiskup.
Unknown source avoid-condrestart  into  master

file modified
+7 -2
@@ -132,6 +132,9 @@

  Requires: js-html5shiv

  Requires: js-respond

  

+ Requires(post): initscripts

+ Requires(postun): initscripts

+ 

  Provides: bundled(bootstrap-combobox) = 1.1.6

  Provides: bundled(bootstrap-select) = 1.5.4

  Provides: bundled(bootstrap-treeview) = 1.0.1
@@ -265,8 +268,8 @@

  

  

  %post

- service httpd condrestart

- service logstash condrestart

+ service httpd    condrestart >/dev/null || :

+ service logstash condrestart >/dev/null || :

  %systemd_post copr-fedmsg-listener.service

  

  
@@ -275,6 +278,8 @@

  

  

  %postun

+ service httpd    condrestart >/dev/null || :

+ service logstash condrestart >/dev/null || :

  %systemd_postun_with_restart copr-fedmsg-listener.service

  

  

file modified
+8 -2
@@ -49,6 +49,10 @@

  Requires:   python3-pytest-cov

  Requires:   python3-mock

  

+ # scriptlets

+ Requires(post): initscripts

+ Requires(postun): initscripts

+ 

  

  %description -n copr-keygen

  COPR is lightweight build system. It allows you to create new project in WebUI,
@@ -150,10 +154,12 @@

  semanage fcontext -a -t httpd_var_lib_t '/var/lib/copr-keygen(/.*)?'

  restorecon -rv /var/lib/copr-keygen/

  

- service httpd condrestart

+ service httpd condrestart &>/dev/null || :

+ 

  

  %postun

- service httpd condrestart

+ service httpd condrestart &>/dev/null || :

+ 

  

  %files

  %license LICENSE

We don't depend on 'initscripts' package explicitly, so the
/sbin/service doesn't have to be available -- so use systemctl try-reload-or-restart <name>.service instead (it's non issue
since those packages are only supported on systemd boxes
nowadays).

Also, be more careful and ignore non-zero exit statuses (and
stdout/stderr outputs) on places where zero exit status it not
guaranteed (it's needed to avoid partial execution, because
scriptlets use set -e shell option).

Not enough; try-reload-or-restart command fails if systemd is not runnning so all occurrences have to be || true ignored.

rebased onto 329a16ff60e9df501abf8f7658fbd5739f9d75d4

5 years ago

Change from service httpd condrestart to systemctl try-reload-or-restart lookgs good to me, but I am not sure whether I understand the need of &>/dev/null || :. Can you please explain to me, in what case systemd is not running?

E.g. when you do mock -r CHROOT --install copr-frontend (no systemd in mock); or you can also install copr-* packages in Dockerfile.

Dah! Right.

Well, I am neutral to this, so don't feel blocked by me, but my opinion is following. AFAIK service command is not obsolete/deprecated, it is just that systemctl is the default one. So personally, I would rather explicitly require initscripts package and use a universal command that works in production/docker/mock than a hacky systemctl one.

I used to prefer service, but that was due RHEL{5,6}. That is not an issue now.

I'm OK to flip that back to /sbin/service, and add explicit Requires, @frostyx please confirm you want that. Motivation here in the PR is to make the scriptlets non-failing; switch to systemctl was just intuitive side-effect...

I am confirming, that personally I prefer going back to service with explicit Requires: initscripts. I don't know whether it is the best move, but I like it most.

with explicit Requires: initscripts

These are actually needed:

Requires(post): initscripts
Requires(postun): initscripts

Thinking more and trying to convince you :-) note that condrestart is still full daemon restart; while reload should be slightly more gently, but still enough for httpd.

rebased onto c208a04658798b93f17ae110f4909b6170f0fd28

5 years ago

Can you, please, reference packaging guidelines about the recommended practice?

I don't know if there are some; the question is whether guidelines would - if we went that far - allow restarting daemons provided by different packages.

rebased onto f2ff968714ae776b0dd2f3ebd12c4110593f3384

5 years ago

On Sunday, June 10, 2018 11:29:14 PM CEST Jakub Kadl=C4=8D=C3=ADk wrote:

I am confirming, that personally I prefer going back to service with
explicit Requires: initscripts.

Done, please take another look.

(for myself: old code is in my fork, under archive-avoid-condrestart branch)

It should be "service SCRIPT COMMAND"

rebased onto b44fead

5 years ago

Pull-Request has been merged by msuchy

5 years ago