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

Do not start foreman.service or foreman.socket on package install #5900

Merged

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Oct 22, 2020

Warning: I do not know exactly if I am doing this right to start, or how to test this without trying to get a test build from the PR process like I would get a scratch build with RPM.

@ekohl
Copy link
Member

ekohl commented Oct 22, 2020

After the build has finished, you add your repo from https://stagingdeb.theforeman.org/ to the system. This is on top of the regular nightly builds. The README on top of it should be fairly self-explanatory, but they live in /etc/apt/sources.list. Note you need to run apt update after changing that file since (unlike yum) apt doesn't auto refresh repo metadata.

@ehelms ehelms force-pushed the deb/develop-foreman-service-socket branch from 7d1eeb8 to f509c09 Compare October 22, 2020 18:06
@@ -33,9 +33,10 @@ build:
dh $@ --with=systemd

override_dh_installinit:
dh_installinit
dh_installinit --only-scripts
Copy link
Member Author

Choose a reason for hiding this comment

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

I did this to stop foreman.service from being created during apt-get install foreman, however, I wondered more broadly if this should be dropped all together in favor of systemd?

Copy link
Member

Choose a reason for hiding this comment

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

Well, technically, Debian still supports non-systemd systems, so if that package would go into Debian proper, that'd be a bug.

Given we don't aim for that, we could say we only support systemd systems.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, dh_installinit is still responsible for also copying the foreman.service files etc, so this line can't be dropped completely yet. But at some point we will be able to. (Needs to bump some other places to make dh_systemd_* take over the copying)

@ehelms ehelms force-pushed the deb/develop-foreman-service-socket branch 2 times, most recently from 2d92674 to d34ad10 Compare October 23, 2020 03:21
debian/bionic/foreman/rules Outdated Show resolved Hide resolved
@ehelms ehelms force-pushed the deb/develop-foreman-service-socket branch from d34ad10 to ce74dc1 Compare October 23, 2020 12:03
@ehelms
Copy link
Member Author

ehelms commented Oct 23, 2020

Working now, after apt-get install foreman-service:

root@pipeline-foreman-server-nightly-ubuntu1804:/home/vagrant# systemctl status foreman.service
● foreman.service - Foreman
   Loaded: loaded (/lib/systemd/system/foreman.service; enabled; vendor preset: enabled)
   Active: inactive (dead)
     Docs: https://theforeman.org
root@pipeline-foreman-server-nightly-ubuntu1804:/home/vagrant# systemctl status foreman.socket
● foreman.socket - Foreman HTTP Server Accept Sockets
   Loaded: loaded (/lib/systemd/system/foreman.socket; enabled; vendor preset: enabled)
   Active: inactive (dead)
   Listen: 0.0.0.0:3000 (Stream)

This puts it in alignment with RPM based installation and prevents some weirdness that can happen where if you install foreman-service but not foreman-postgresql the app would attempt to start and error out.

@ekohl
Copy link
Member

ekohl commented Oct 23, 2020

Note you can also use systemctl status foreman.service foreman.socket and get the output from both at once.

@ekohl ekohl requested a review from evgeni October 23, 2020 15:26
@evgeni
Copy link
Member

evgeni commented Oct 26, 2020

Is this PR then the right time to drop https://github.com/theforeman/foreman-packaging/blob/deb/develop/debian/buster/foreman/foreman.init (and https://github.com/theforeman/foreman-packaging/blob/deb/develop/debian/buster/foreman/foreman.default)?

would require some further cleanup like we did for dynflow:

# remove sysv-init script
update-rc.d dynflowd remove >/dev/null
dpkg-maintscript-helper rm_conffile /etc/init.d/dynflowd 2.0.0~ -- "$@"
dpkg-maintscript-helper rm_conffile /etc/default/dynflowd 2.0.0~ -- "$@"

but at least it would mean we don't actually ship an init script that we don't support at all anymore

@evgeni
Copy link
Member

evgeni commented Oct 26, 2020

packaging wise this looks correct to me

@ehelms
Copy link
Member Author

ehelms commented Oct 26, 2020

Can I do the drop of initscript separately to reduce potential issues? I can do a PR for it now. I'd like to, if this change is approved, get this in so I can continue finishing up theforeman/puppet-foreman#883

@evgeni
Copy link
Member

evgeni commented Oct 26, 2020

Oh yeah, certainly!

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

from a debian packaging perspective, this looks correct so ACK

I assume you tested this that it actually does what you need in your setup.

@ehelms
Copy link
Member Author

ehelms commented Oct 26, 2020

To the best of my ability I tested this, this at least brings this into alignment with RPM which is a win

@ehelms ehelms merged commit 57776da into theforeman:deb/develop Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants