ubuntu-docker-images team mailing list archive

[Sergio Durigan Junior <sergio.durigan@xxxxxxxxxxxxx>]

On Monday, November 22 2021, Athos Ribeiro wrote:

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

Hey :-).

>>I finally "finished" the Loki snap and have something ready for review.
>
> Nice! Thanks :)

Thanks for the review.

>>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.

Good point, I will do that.

> 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).

Yeah, I was a bit unsure if it made sense to ship loki-canary.  As you
said, it's not hard to modify the recipe and just install it as
loki.loki-canary.  Either way, I think we can start without it and then
include the binary later when the snap is more mature.

> 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?

Good point.  I will license the snap using Apache 2.0.

I will perform the requested changes and then push to the official
repository under the loki-snap project.  Then, I intend to proceed with
filing a request to create this snap on the snapcraft store (I've never
done that before, but hopefully this won't be too hard).

Thanks again,

-- 
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0  EB2F 106D A1C8 C3CB BF14