ubuntu-docker-images team mailing list archive

[Athos Ribeiro <athos.ribeiro@xxxxxxxxxxxxx>]

On Fri, Nov 19, 2021 at 04:07:47PM -0500, Sergio Durigan Junior wrote:
> Hi,

Hi Sergio!

> I finally "finished" the Loki snap and have something ready for review.

Nice! Thanks :)

> TL;DR:
>
>  Git repo: https://code.launchpad.net/~sergiodj/loki-snap/+git/loki-snap
>  Test snap: https://launchpad.net/~sergiodj/+snap/loki-sergiodj-test
>
> So, Loki is a relatively simple software.  It generates a bunch of
> binaries, but the snap is only shipping 3 of them:
>
>  - loki (installed as loki), the log aggregation server
>  - promtail (installed as loki.promtail), one of its clients
>  - logcli (installed as loki.logcli), a CLI to communicate with the
>    server
>
> I'm using the same method to get/set the server configuration as the
> Cassandra snap uses: have a pair of scripts (config-{set,get})
> responsible for dumping the current configuration file and overwriting
> the existing config file with a new one.
>
> A few things worth noting:
>
> - The repository doesn't have a README.md file yet.  I will probably
>  have to create one when I apply for a new snap on the snapcraft
>  discourse.
>
> - No tests, as usual.  If you want to run a quick&dirty test to make
>  sure the service is running, install the snap and point your browser
>  to http://localhost:3100/metrics.
>
>  This snap will be used as a base for the Loki OCI image, which will
>  have tests (as usual).
>
> - I already created the loki-snap project on LP:
>
>    https://launchpad.net/loki-snap
>
>  LP won't let me file an MP against a non-existing repository
>  (rightfully so), so for now we can do our reviews via email.

I finished reviewing the snap and it LGTM.

[x] Snap builds correctly
[x] Snap installs and launches as expected
[x] Configuration wrappers work as expected
[x] Latest version was snapped
[x] Binary snap only ships the files needed for the snap runtime, in the
    correct paths

It would be nice to add a short comment when you set the build
environment to prevent the build to be triggered within a Docker
container.

I noticed that loki-canary is also built for the same target that builds
all the shipped binaries, but this one is (the only one) not being
shipped. Since this is a tool to audit loki's performance, I agree with
your initial assessment of not shipping it ATM. We can revisit this in
the future in case users start missing it (it would be fairly simple to
add it to the snap at this point).

Finally, you mentioned that a README file is missing and that you should
add one. Should we also consider including a license into the repository
as well?

> Thanks in advance,
>
> --
> Sergio
> GPG key ID: E92F D0B3 6B14 F1F4 D8E0  EB2F 106D A1C8 C3CB BF14

--
Athos Ribeiro