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

Update Debian Package Services and Config Directory #1855

Open
wants to merge 46 commits into
base: master
Choose a base branch
from

Conversation

DaAwesomeP
Copy link
Member

Now that I am using the Debian build from master in production I have found some outdated parts.

This updates the Debian package to use a modern Systemd service and moves the config folder. It also removes the reliance on Debconf to enable/disable which definitely not needed for Systemd and is redundant on Debian for init scripts (and especially redundant now that Systemd wraps init scripts by default).

Per Debian package recommendations both the init script and Systemd service will be installed, but users running systemctl start|enable|etc olad will now trigger the Systemd service instead of the init script Systemd wrapper. This fixes a big bug I was experiencing where Systemd does not properly trigger the init script and so systemctl restart olad did not work with the init script but start and stop did.

Systemd users can now override options with Systemd overrides instead of an environment file /etc/default (new preferred method).

Per Debian package recommendations the configuration files should go in /etc/ola since they are user editable and not 100% runtime-controlled. This is also where I think most users expect to find the configuration.

This creates some breaking changes, but I think this is OK for the following reasons:

  1. Until GitHub Actions Debian Builds #1812 the Debian build in this repo was broken, so surely the raw Debian build from this repo has not been used in actual production in some time.
  2. Due to the above I am 99% sure that anyone using OLA on a Debian-based system is either:
    a. Building from source, installing with make install, and creating their own config directory and service scheme
    b. Installing from the Debian package repository which already uses /etc/ola for configs and has removed the Debconf bits
  3. I am submitting this against master instead of 0.10.

These changes should make it much smoother to move between the Debian repo packages and the ones created here. As new features and bugfixes seem to have accelerated recently, the ability to use the pre-packaged master build is quite nice. Personally I experienced this because I needed KiNETv2 support (#1787).

The main remaining difference in the packages is in the way that the web assets are handled, but most users won't perceive a difference between these builds with that or would possibly try this build if that one breaks. I don't really see a need to update that part here.

Hopefully this eventually makes its way downstream to Debian and brings the Systemd service to everyone.

@DaAwesomeP
Copy link
Member Author

Requesting review from @peternewman

@DaAwesomeP
Copy link
Member Author

OK, I have this running on a production machine and it works like a charm!

@DaAwesomeP
Copy link
Member Author

@peternewman polite ping!

Copy link
Contributor

@yoe yoe left a comment

Choose a reason for hiding this comment

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

I know you didn't ask for my review, but I'm giving it anyway ;-P

Although I made some suggestions for improvement, none of them are really critical, so from my POV things can go in as-is and that would be better than the status quo; but I'll leave actual approval up to @peternewman.

debian/changelog Show resolved Hide resolved
debian/control Show resolved Hide resolved
@DaAwesomeP
Copy link
Member Author

I know you didn't ask for my review, but I'm giving it anyway ;-P

I didn't ask but you're an infinitely more experienced package maintainer than me and I am excited that you left a review! :D

@peternewman
Copy link
Member

Note from the init/systemd side there's also #1444 which I suspect is stuck on me looking at some stuff (as always)...

@DaAwesomeP
Copy link
Member Author

Polite ping! @peternewman @yoe

@DaAwesomeP
Copy link
Member Author

@peternewman @yoe resynced and ready for review!

@DaAwesomeP
Copy link
Member Author

Hey @peternewman, would appreciate a review when you have a spare moment. I want to push this out to some production machines soon if I can.

@DaAwesomeP
Copy link
Member Author

@peternewman one final ping!

@DaAwesomeP
Copy link
Member Author

@peternewman @yoe I know that this is a large pull request, but I would really appreciate your feedback!

@peternewman
Copy link
Member

Hi @DaAwesomeP ,

Sorry for the lack of reply. Could you do a new PR which just cherry picks the build version stuff and not all the Systemd bits please initially, so we can get that in before we have to consider your changes versus:

Note from the init/systemd side there's also #1444 which I suspect is stuck on me looking at some stuff (as always)...

@DaAwesomeP
Copy link
Member Author

so we can get that in before we have to consider your changes versus:

Note from the init/systemd side there's also #1444 which I suspect is stuck on me looking at some stuff (as always)...

I don't think that these are mutually exclusive/conflict. I'm only adding the Systemd service to the Debian install; I'm not adding notify support. When that pull is merged (or as a part of merging it) we can modify this service to include the notify support. Right now this is just a drop-in replacement for the initd scripts.

@DaAwesomeP
Copy link
Member Author

DaAwesomeP commented Oct 23, 2023

@peternewman Please let me know! I am a bit hesitant to work on other things while this and other pulls have been open for so long. I completely understand that this is open source work, everyone is working on their available time, and what gets merged is up to you, but I am a bit less likely to contribute when pulls stagnate amidst other active development. I mean this entirely constructively and really do not mean to be rude at all--I felt like my constant pings were becoming rude.

This doesn't conflict with the notify implementation. When notify support is implemented only a few lines in the Systemd service will be changed. This pull simply replaces the init.d implentation and continues to treat OLA as a simple start/stop service. I should also note that #1444 does not touch any of the service files in the debian/ directory.

debian/ola-rdm-tests.rdm_test_server.service Show resolved Hide resolved
debian/rules Show resolved Hide resolved
@DaAwesomeP
Copy link
Member Author

@peternewman @kripton rebased!

@kripton
Copy link
Member

kripton commented Mar 27, 2024

I saw your ping. However, since I neither use Debian nor systemd, I'm probably not really qualified to comment this one. However, when it improves the situation, I'd rather merge it (and fix later in case of problems) instead of not merging it. Ping @peternewman ;)

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

Successfully merging this pull request may close these issues.

4 participants