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.

Sign up for GitHub

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

Add Makefile #221

Merged
merged 4 commits into from
Jun 21, 2024
Merged

Add Makefile #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d62278
Add Makefile
julianbrost Jun 19, 2024
1900a36
add make test
julianbrost Jun 20, 2024
1bf100a
Allow setting libexecdir (/usr/libexec) from build flags
julianbrost Jun 20, 2024
9ad4cbc
Add default path for config file
julianbrost Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/build/
13 changes: 4 additions & 9 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,15 @@ ENV CGO_ENABLED 0
COPY . /src/icinga-notifications
WORKDIR /src/icinga-notifications

RUN mkdir bin
RUN --mount=type=cache,target=/go/pkg/mod \
--mount=type=cache,target=/root/.cache/go-build \
go build -o bin/ ./cmd/icinga-notifications
make all

RUN mkdir bin/channel
RUN --mount=type=cache,target=/go/pkg/mod \
--mount=type=cache,target=/root/.cache/go-build \
go build -o bin/channel/ ./cmd/channel/...
RUN make DESTDIR=/target install

FROM docker.io/library/alpine

COPY --from=build /src/icinga-notifications/bin/icinga-notifications /usr/bin/icinga-notifications
COPY --from=build /src/icinga-notifications/bin/channel /usr/libexec/icinga-notifications/channel
COPY --from=build /target /

RUN apk add tzdata

Expand All @@ -28,4 +23,4 @@ RUN adduser -u 1000 -H -D -G $username $username
USER $username

EXPOSE 5680
CMD ["/usr/bin/icinga-notifications", "--config", "/etc/icinga-notifications/config.yml"]
CMD ["/usr/sbin/icinga-notifications"]
43 changes: 43 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# These variables follow the naming convention from the GNU Make documentation
# but their defaults correspond to the rest of the code (note that changing
# libexecdir here wouldn't affect the default path for the channel plugin
# directory used by the dameon for example).
#
# https://www.gnu.org/software/make/manual/html_node/Directory-Variables.html
prefix ?= /usr
sbindir ?= $(prefix)/sbin
libexecdir ?= $(prefix)/libexec
datadir ?= $(prefix)/share
sysconfdir ?= /etc

all: pkg = github.com/icinga/icinga-notifications/internal
oxzi marked this conversation as resolved.
Show resolved Hide resolved
all:
mkdir -p build
go build \
-o build/ \
-ldflags "-X '$(pkg).LibExecDir=$(libexecdir)' -X '$(pkg).SysConfDir=$(sysconfdir)'" \
./cmd/icinga-notifications
go build -o build/channel/ ./cmd/channel/...

test:
go test ./...

install:
oxzi marked this conversation as resolved.
Show resolved Hide resolved
@# config
install -d $(DESTDIR)$(sysconfdir)/icinga-notifications
install -m644 config.example.yml $(DESTDIR)$(sysconfdir)/icinga-notifications/config.yml
Copy link
Member

Choose a reason for hiding this comment

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

Why changing this to 644? We use 640 literally everywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I expect the permissions to still be taken care of by the packaging as the owner has to be set there anyways (and probably isn't even known until installation time due do dynamically allocated user/group IDs). I wouldn't have given -m at all (like I did for the install -d above), but that would result in the file being executable.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this Makefile supposed to simplify the Debian rules file? If this file doesn't take care of permissions handling, as we already do in override_dh_auto_install, I don't see how this is supposed to make things simpler, other than to de-duplicate the go build .. parts, for which by the way you don't use the same build flags as we do for the package builds.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't this Makefile supposed to simplify the Debian rules file? [...] I don't see how this is supposed to make things simpler, other than to de-duplicate the go build .. parts

It takes care of which files to build an where to install them.

So it should be possible to remove the override_dh_auto_build and override_dh_auto_install targets from debian/rules (those should call make/make install automatically) and then simply set the permissions in override_dh_fixperms (I'm not sure if debian/icinga-notifications is the correct prefix for the path there):

override_dh_fixperms:
	dh_fixperms
	chmod -R o= debian/icinga-notifications/etc/icinga-notifications

for which by the way you don't use the same build flags as we do for the package builds.

These can be set using an environment variable: GOFLAGS='-buildvcs=false -trimpath'

We could consider always adding -trimpath, but I wouldn't add -buildvcs=false to the Docker image builds as these are built from a Git checkout and thus receive the proper information that way.

@# dameon
install -D build/icinga-notifications $(DESTDIR)$(sbindir)/icinga-notifications
@# channel plugins
install -d $(DESTDIR)$(libexecdir)/icinga-notifications/channel
Copy link
Member

Choose a reason for hiding this comment

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

The name channel does not make sense to me here, we will not have only one channel there, so I have already adapted this to channels in the Debian rules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That has already come up in #210 (comment) as well. Simply using a different path here would introduce an inconsistency with the default path used in the binary:

icinga-notifications/internal/daemon/config.go

Line 16 in bd9633f

ChannelPluginDir string `yaml:"channel-plugin-dir" default:"/usr/libexec/icinga-notifications/channel"`

So if that's supposed to be changed, that's something for its own PR. I don't care too much if it's singular or plural, I wouldn't say any is more or less correct than the other.

install build/channel/* $(DESTDIR)$(libexecdir)/icinga-notifications/channel/
@# database schema
install -d $(DESTDIR)$(datadir)/icinga-notifications
cp -rv --no-dereference schema $(DESTDIR)$(datadir)/icinga-notifications
@# chmod ensures consistent permissions when cp is called with umask != 022
chmod -R u=rwX,go=rX $(DESTDIR)$(datadir)/icinga-notifications/schema

clean:
rm -rf build

.PHONY: all test install clean
7 changes: 1 addition & 6 deletions cmd/icinga-notifications/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func main() {
var configPath string
var showVersion bool

flag.StringVar(&configPath, "config", "", "path to config file")
flag.StringVar(&configPath, "config", internal.SysConfDir+"/icinga-notifications/config.yml", "path to config file")
flag.BoolVar(&showVersion, "version", false, "print version")
flag.Parse()

Expand All @@ -43,11 +43,6 @@ func main() {
return
}

if configPath == "" {
_, _ = fmt.Fprintln(os.Stderr, "missing -config flag")
os.Exit(1)
}

err := daemon.LoadConfig(configPath)
if err != nil {
_, _ = fmt.Fprintln(os.Stderr, "cannot load config:", err)
Expand Down
2 changes: 1 addition & 1 deletion config.example.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#debug-password: "put-something-secret-here"

icingaweb2-url: http://localhost/icingaweb2/
channel-plugin-dir: /usr/libexec/icinga-notifications/channel
#channel-plugin-dir: /usr/libexec/icinga-notifications/channel
api-timeout: 1m

database:
Expand Down
13 changes: 12 additions & 1 deletion internal/daemon/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,31 @@ import (
"github.com/goccy/go-yaml"
"github.com/icinga/icinga-go-library/database"
"github.com/icinga/icinga-go-library/logging"
"github.com/icinga/icinga-notifications/internal"
"os"
"time"
)

type ConfigFile struct {
Listen string `yaml:"listen" default:"localhost:5680"`
DebugPassword string `yaml:"debug-password"`
ChannelPluginDir string `yaml:"channel-plugin-dir" default:"/usr/libexec/icinga-notifications/channel"`
ChannelPluginDir string `yaml:"channel-plugin-dir"`
ApiTimeout time.Duration `yaml:"api-timeout" default:"1m"`
Icingaweb2URL string `yaml:"icingaweb2-url"`
Database database.Config `yaml:"database"`
Logging logging.Config `yaml:"logging"`
}

// SetDefaults implements the defaults.Setter interface.
func (c *ConfigFile) SetDefaults() {
if defaults.CanUpdate(c.ChannelPluginDir) {
c.ChannelPluginDir = internal.LibExecDir + "/icinga-notifications/channel"
}
}

// Assert interface compliance.
var _ defaults.Setter = (*ConfigFile)(nil)

// config holds the configuration state as a singleton. It is used from LoadConfig and Config
var config *ConfigFile

Expand Down
7 changes: 7 additions & 0 deletions internal/paths.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package internal

// These variables exist to allow overwriting the paths using `go build -ldflags "-X ...", see Makefile.
var (
LibExecDir = "/usr/libexec"
SysConfDir = "/etc"
)
Loading