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

Build packages for releases #2

Merged
merged 4 commits into from
Apr 17, 2024
Merged

Build packages for releases #2

merged 4 commits into from
Apr 17, 2024

Conversation

gzuidhof
Copy link
Contributor

Builds deb, rpm, apk packages.

Based on haproxytech/dataplaneapi@0264138

@gzuidhof gzuidhof requested a review from imiric April 16, 2024 19:03
Base automatically changed from initial-version to main April 17, 2024 09:00
Copy link

@imiric imiric left a comment

Choose a reason for hiding this comment

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

👍

I'm not a big fan of goreleaser and prefer building packages with a script instead, but it does make things a bit more convenient.

goreleaser.yml Outdated
- apk
- deb
- rpm
bindir: /usr/sbin
Copy link

Choose a reason for hiding this comment

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

Why in sbin? I believe that is reserved for binaries that require root permissions, and hareply doesn't strictly require it (as long as the server port is >1024).

So I would make this /usr/bin instead.

Environment="PIDFILE=/run/hareply.pid"
EnvironmentFile=/etc/default/hareply
ExecStart=/usr/sbin/hareply
ExecReload=/bin/kill -s SIGUSR1 $MAINPID
Copy link

Choose a reason for hiding this comment

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

hareply doesn't currently listen to this signal, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. This comes from the copy paste from how dataplaneapi does things for their systemd service: but we indeed do not need this or use it currently.

I'll remove this command entirely, as well as the PIDFILE options below

Comment on lines 11 to 12
PIDFile=
GuessMainPID=1
Copy link

Choose a reason for hiding this comment

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

I'm not well versed in systemd unit files, but I'm curious: why set PIDFILE in the environment, but then make PIDFile blank and use GuessMainPID? I could look it up as well, but if you explain it then it saves me from reading systemd documentation 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into it in more detail, my conclusion in the end is that PID files should no longer be required for most services, and rather Type=simple should be sufficient [source]

@gzuidhof gzuidhof merged commit a9cb935 into main Apr 17, 2024
2 checks passed
@gzuidhof gzuidhof deleted the deb-file branch April 17, 2024 09:40
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.

2 participants