MIME-Version: 1.0
References: <20200114234705.25341-1-sir@cmpwn.com>
In-Reply-To: <20200114234705.25341-1-sir@cmpwn.com>
From: Tiago Ilieve <tiago.myhro@gmail.com>
Date: Wed, 15 Jan 2020 07:54:38 -0300
Message-ID: <CALdTKe80jbhM+mamcmqqLQwm8VD=Q4cdOrdt=CQjHzaKgu9dYg@mail.gmail.com>
Subject: Re: [PATCH] testing/prometheus-node-exporter: import from sr.ht
To: Drew DeVault <sir@cmpwn.com>
Cc: ~alpine/aports@lists.alpinelinux.org
Content-Type: text/plain; charset="UTF-8"

Hi Drew,

Your message arrived at the spam folder and I almost ended-up missing
it. I'd like to clarify a few points about the changes:

- Doesn't the patch make sense anymore? I've not been following the
state of Go race condition check in musl lately.
- Does it make sense to drop the version information from the binary?
I'm inferring that, as the software is being installed by a package
manager, you are considering this information to be irrelevant.
- If the binary name is being changed, why opt for
'prometheus_node_exporter' (with underscores) instead of
'prometheus-node-exporter' (with hyphens), like other distributions[1]
are doing?
- Changing the default binding to 127.0.0.1:9100 might break user
expectations, as it currently defaults to 0.0.0.0:9100. My use cases,
for instance, always involved fetching data from it remotely. Binding
it to localhost by default seems too restrictive in this case, akin to
doing the same to a HTTP server.

I'm all in for moving it to the community repos as soon as we agree on
the mentioned topics.

Regards,
Tiago.

[1]: https://packages.ubuntu.com/bionic/amd64/prometheus-node-exporter/filelist

On Tue, 14 Jan 2020 at 20:46, Drew DeVault <sir@cmpwn.com> wrote:
>
> This imports the package we made for sr.ht monitoring, which is more in
> line with the design of prometheus-related packages in community.
> ---
> Tiago, what do you think of this approach? If you like it, let's take
> these changes and move this package into community. I can also
> adopt/maintain the package if you prefer.
>
>  testing/prometheus-node-exporter/APKBUILD     | 49 ++++++++++---------
>  .../disable-go-race-detector.patch            | 33 -------------
>  .../node-exporter.confd                       |  7 ---
>  .../node-exporter.initd                       | 22 ---------
>  .../prometheus-node-exporter.confd            |  4 ++
>  .../prometheus-node-exporter.initd            | 13 +++++
>  .../prometheus-node-exporter.pre-install      |  8 ++-
>  .../prometheus-node-exporter.pre-upgrade      |  1 -
>  8 files changed, 45 insertions(+), 92 deletions(-)
>  delete mode 100644 testing/prometheus-node-exporter/disable-go-race-detector.patch
>  delete mode 100644 testing/prometheus-node-exporter/node-exporter.confd
>  delete mode 100755 testing/prometheus-node-exporter/node-exporter.initd
>  create mode 100644 testing/prometheus-node-exporter/prometheus-node-exporter.confd
>  create mode 100644 testing/prometheus-node-exporter/prometheus-node-exporter.initd
>  mode change 100755 => 100644 testing/prometheus-node-exporter/prometheus-node-exporter.pre-install
>  delete mode 120000 testing/prometheus-node-exporter/prometheus-node-exporter.pre-upgrade
>
> diff --git a/testing/prometheus-node-exporter/APKBUILD b/testing/prometheus-node-exporter/APKBUILD
> index c8af5fd1be..b785d4eb56 100644
> --- a/testing/prometheus-node-exporter/APKBUILD
> +++ b/testing/prometheus-node-exporter/APKBUILD
> @@ -1,44 +1,45 @@
> -# Contributor: Tiago Ilieve <tiago.myhro@gmail.com>
>  # Maintainer: Tiago Ilieve <tiago.myhro@gmail.com>
> +# Contributor: Tiago Ilieve <tiago.myhro@gmail.com>
> +# Contributor: Drew DeVault <sir@cmpwn.com>
>  pkgname=prometheus-node-exporter
> -_pkgname=node_exporter
>  pkgver=0.18.1
> -pkgrel=2
> -pkgdesc="Prometheus exporter for machine metrics"
> +pkgrel=3
> +pkgdesc="prometheus exporter for machine metrics"
>  url="https://github.com/prometheus/node_exporter"
>  arch="all"
>  license="Apache-2.0"
> -makedepends="go>=1.11 bash sed"
> -install="$pkgname.pre-install $pkgname.pre-upgrade"
> +install="prometheus-node-exporter.pre-install"
> +makedepends="go"
> +source="
> +       $pkgname-$pkgver.tar.gz::https://github.com/prometheus/node_exporter/archive/v$pkgver.tar.gz
> +       prometheus-node-exporter.initd
> +       prometheus-node-exporter.confd
> +"
>  subpackages="$pkgname-openrc"
> -source="$_pkgname-$pkgver.tar.gz::https://github.com/prometheus/node_exporter/archive/v$pkgver.tar.gz
> -       disable-go-race-detector.patch
> -       node-exporter.confd
> -       node-exporter.initd"
> -builddir="$srcdir/$_pkgname-$pkgver"
> +builddir="$srcdir/node_exporter-$pkgver"
>
>  build() {
>         cd "$builddir"
> -
> -       go build -v -ldflags "-s -w -X github.com/prometheus/common/version.Version=$pkgver" -o node_exporter
> +       go build
>  }
>
>  check() {
>         cd "$builddir"
> -
> -       make test
> -       ./node_exporter --version
> +       go test
>  }
>
>  package() {
>         cd "$builddir"
> -
> -       install -Dm644 "$srcdir/node-exporter.confd" "$pkgdir"/etc/conf.d/node-exporter
> -       install -Dm755 "$srcdir/node-exporter.initd" "$pkgdir"/etc/init.d/node-exporter
> -       install -Dm755 ./node_exporter "$pkgdir"/usr/bin/node_exporter
> +    mkdir -p "$pkgdir"/usr/bin
> +    mkdir -p "$pkgdir"/etc/init.d
> +    mkdir -p "$pkgdir"/etc/conf.d
> +       install -Dm755 node_exporter "$pkgdir"/usr/bin/prometheus_node_exporter
> +       install -Dm755 "$srcdir"/prometheus-node-exporter.initd \
> +               "$pkgdir"/etc/init.d/prometheus-node-exporter
> +       install -Dm644 "$srcdir"/prometheus-node-exporter.confd \
> +               "$pkgdir"/etc/conf.d/prometheus-node-exporter
>  }
>
> -sha512sums="cf2b854fbec1ba39b18432cdc11ab507ebc5b2828e6b34b4f49af9b949fcd365cb3e5eb1e265d0825783810c6b1572a8b3512ec27a46e5c83f89a505590159c0  node_exporter-0.18.1.tar.gz
> -36952039e5db39aa06a2ca16fa5d318f22eb967e3e9b1363508b2f3d3d0b14e5df111c77ce951bea2f65dd2be0a0a1582be245acfe4641623381a940204e53d9  disable-go-race-detector.patch
> -592d3f17a3cf487d97a14c803dded07d2dfb112b159ab1a3575310fc0176fc3255ddad6657f16a8d6d3c161bfb03c203a6271ec6e6395b75716a14b0de8baced  node-exporter.confd
> -df006b184c3b2d5e773044838db7143dc3a64e621f4da6b106f41915a07d5cef32933ab9bb44464f10c132f86997ae7753ae19627afd4ba6cfb125161786dfe0  node-exporter.initd"
> +sha512sums="cf2b854fbec1ba39b18432cdc11ab507ebc5b2828e6b34b4f49af9b949fcd365cb3e5eb1e265d0825783810c6b1572a8b3512ec27a46e5c83f89a505590159c0  prometheus-node-exporter-0.18.1.tar.gz
> +fe8320b47d93c9590ccf9a3456f74b90a960fd46a7561202470687de2fc5d2caebded682d2274bec71602e694b778f282d5876400da60d716ce3d3cee5e45b99  prometheus-node-exporter.initd
> +a19cb380e12a292af3645de18d59ebac6ce4b521281f1761d069b5d8bd574a83c92265a3a4023e372fce7ec9bab0f1849e9953cf81801fc24576acec20afb964  prometheus-node-exporter.confd"
> diff --git a/testing/prometheus-node-exporter/disable-go-race-detector.patch b/testing/prometheus-node-exporter/disable-go-race-detector.patch
> deleted file mode 100644
> index ba3c4cf571..0000000000
> --- a/testing/prometheus-node-exporter/disable-go-race-detector.patch
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -From 5136eb5266b2ff022a44a3316a07d02b4dcc9e08 Mon Sep 17 00:00:00 2001
> -From: Tiago Ilieve <tiago.myhro@gmail.com>
> -Date: Sun, 12 May 2019 11:21:01 +0200
> -Subject: [PATCH] Disable Go race detector
> -
> -Go race detector is not supported on musl[1].
> -
> -[1]: https://github.com/golang/go/issues/14481
> ----
> - Makefile.common | 7 -------
> - 1 file changed, 7 deletions(-)
> -
> -diff --git a/Makefile.common b/Makefile.common
> -index 4f18ea5..18085e4 100644
> ---- a/Makefile.common
> -+++ b/Makefile.common
> -@@ -94,13 +94,6 @@ BUILD_DOCKER_ARCHS = $(addprefix common-docker-,$(DOCKER_ARCHS))
> - PUBLISH_DOCKER_ARCHS = $(addprefix common-docker-publish-,$(DOCKER_ARCHS))
> - TAG_DOCKER_ARCHS = $(addprefix common-docker-tag-latest-,$(DOCKER_ARCHS))
> -
> --ifeq ($(GOHOSTARCH),amd64)
> --        ifeq ($(GOHOSTOS),$(filter $(GOHOSTOS),linux freebsd darwin windows))
> --                # Only supported on amd64
> --                test-flags := -race
> --        endif
> --endif
> --
> - # This rule is used to forward a target like "build" to "common-build".  This
> - # allows a new "build" target to be defined in a Makefile which includes this
> - # one and override "common-build" without override warnings.
> ---
> -2.17.1
> -
> diff --git a/testing/prometheus-node-exporter/node-exporter.confd b/testing/prometheus-node-exporter/node-exporter.confd
> deleted file mode 100644
> index 39c80a46be..0000000000
> --- a/testing/prometheus-node-exporter/node-exporter.confd
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -# /etc/conf.d/node-exporter
> -
> -# Custom arguments can be specified like:
> -#
> -# ARGS="--web.listen-address=':9100'"
> -
> -ARGS=""
> diff --git a/testing/prometheus-node-exporter/node-exporter.initd b/testing/prometheus-node-exporter/node-exporter.initd
> deleted file mode 100755
> index 5bd9839fe8..0000000000
> --- a/testing/prometheus-node-exporter/node-exporter.initd
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -#!/sbin/openrc-run
> -
> -command="/usr/bin/node_exporter"
> -command_args="$ARGS"
> -command_background="yes"
> -group="prometheus"
> -user="prometheus"
> -
> -logdir="/var/log/prometheus"
> -logfile="$logdir/${SVCNAME}.log"
> -pidfile="/var/run/${SVCNAME}.pid"
> -start_stop_daemon_args="--stderr $logfile --user $user --group $group"
> -
> -depend() {
> -       need net
> -       after firewall
> -}
> -
> -start_pre() {
> -       checkpath -d -o $user:$group -m755 $logdir
> -       checkpath -f -o $user:$group -m644 $logfile
> -}
> diff --git a/testing/prometheus-node-exporter/prometheus-node-exporter.confd b/testing/prometheus-node-exporter/prometheus-node-exporter.confd
> new file mode 100644
> index 0000000000..8843b48c8f
> --- /dev/null
> +++ b/testing/prometheus-node-exporter/prometheus-node-exporter.confd
> @@ -0,0 +1,4 @@
> +prometheus_node_exporter_web_listen_address=127.0.0.1:9100
> +
> +output_log=/var/log/prometheus-node-exporter.log
> +error_log=/var/log/prometheus-node-exporter.log
> diff --git a/testing/prometheus-node-exporter/prometheus-node-exporter.initd b/testing/prometheus-node-exporter/prometheus-node-exporter.initd
> new file mode 100644
> index 0000000000..e7ac0b74a9
> --- /dev/null
> +++ b/testing/prometheus-node-exporter/prometheus-node-exporter.initd
> @@ -0,0 +1,13 @@
> +#!/sbin/openrc-run
> +name="prometheus-node-exporter"
> +description="Prometheus node monitoring daemon"
> +supervisor=supervise-daemon
> +command=/usr/bin/prometheus_node_exporter
> +command_args="--web.listen-address=$prometheus_node_exporter_web_listen_address"
> +
> +start_pre() {
> +       [ -n "$output_log" ] && checkpath -f "$output_log" \
> +               -m 644 -o prometheus:prometheus || true
> +       [ -n "$error_log" ] && checkpath -f "$error_log" \
> +               -m 644 -o prometheus:prometheus || true
> +}
> diff --git a/testing/prometheus-node-exporter/prometheus-node-exporter.pre-install b/testing/prometheus-node-exporter/prometheus-node-exporter.pre-install
> old mode 100755
> new mode 100644
> index 120995cf35..004fd0bcb9
> --- a/testing/prometheus-node-exporter/prometheus-node-exporter.pre-install
> +++ b/testing/prometheus-node-exporter/prometheus-node-exporter.pre-install
> @@ -1,6 +1,4 @@
>  #!/bin/sh
> -
> -addgroup -S prometheus 2>/dev/null
> -adduser -S -D -h /var/lib/prometheus -s /sbin/nologin -G prometheus -g prometheus prometheus 2>/dev/null
> -
> -exit 0
> +grep '^prometheus:' /etc/group >/dev/null || addgroup -S prometheus 2>/dev/null
> +grep '^prometheus:' /etc/passwd >/dev/null || adduser -SDh/var/lib/prometheus \
> +       -s/sbin/nologin -Gprometheus -gprometheus prometheus prometheus 2>/dev/null
> diff --git a/testing/prometheus-node-exporter/prometheus-node-exporter.pre-upgrade b/testing/prometheus-node-exporter/prometheus-node-exporter.pre-upgrade
> deleted file mode 120000
> index b87af95334..0000000000
> --- a/testing/prometheus-node-exporter/prometheus-node-exporter.pre-upgrade
> +++ /dev/null
> @@ -1 +0,0 @@
> -prometheus-node-exporter.pre-install
> \ No newline at end of file
> --
> 2.24.1
>


-- 
Tiago "Myhro" Ilieve
Blog: https://blog.myhro.info/
GitHub: https://github.com/myhro
LinkedIn: https://www.linkedin.com/in/myhro/
Montes Claros, Brazil