ubuntu-docker-images team mailing list archive

[Athos Ribeiro <athos.ribeiro@xxxxxxxxxxxxx>]

On Tue, Aug 24, 2021 at 05:48:02PM -0400, Sergio Durigan Junior wrote:
> On Tuesday, August 24 2021, Bryce Harrington wrote:

Hi Sergio and Bryce, Thanks for reviewing this one!

> > On Tue, Aug 24, 2021 at 04:05:04PM -0400, Sergio Durigan Junior wrote:
> > > On Tuesday, August 24 2021, Athos Ribeiro wrote:
> > >
> > > > Hello,
> > > >
> > > > I have prepared and pushed an initial version for a squid OCI [1].
> > > >
> > > > It contains docker-compose and k8s examples and an initial version of
> > > > the image documentation.
> > > >
> > > > While I am still preparing the tests for the image to be included in
> > > > https://github.com/canonical/server-test-scripts, it would be nice to
> > > > get an initial review on the new OCI.
> > >
> > > Hey again,
> > >
> > > Here's the review.
> > >
> > > - Dockerfile LGTM.
> > >
> > > - entrypoint.sh could do with a few more comments, I think.  Also, I
> > >   noticed that you implemented the idea we've been discussing, about
> > >   redirecting the logs to files that are part of volumes exported to the
> > >   user.  While we certainly should consider this idea, implementing it
> > >   for squid but not for other images will bring inconsistency to our
> > >   portfolio.  What do you think?

Squid is a special case where nothing would be written in stdout/stderr
during the service runtime if we do not try to append those logs there
somehow. It actually seems that the current approach is maintaining
consistency with (at least most of) our other images. We could forward
just the access logs or access+error logs to stdout/stderr though and
keep the cache logs in the log file only. WDYT?

> > > - Still on the entrypoint.sh script, it would be great if it could
> > >   handle extra arguments to the squid process.  You can use the
> > >   "start-redis.sh" script (from the redis image) as a base (no need to
> > >   make it fancy!  Just a standard way to deal with extra args would be
> > >   fine already).

Good catch. Most (if not all) of our images do override the ENTRYPOINT,
which I was not doing in that Dockerfile. I will file an MP soon with
these changes!

> > > - data/squid.yaml LGTM.  I was initially a bit unsure about the "(not
> > >   quite; we're getting there)" excerpt in the description, but I think
> > >   it's fine.
> >
> > Does this text come from upstream?
>
> Yeah, it comes from the About page here:
>
> http://www.squid-cache.org/Intro/
>
> > If so, I wonder if it might read
> > better to omit that as it feels odd here; the style seems informal
> > compared with the rest of the text, and might give wrong impression of
> > what Canonical's plans actually are.
>
> Indeed, I had written a comment suggesting that we blend this text with
> the one from the main page:
>
> http://www.squid-cache.org/
>
>  Squid is a caching proxy for the Web supporting HTTP, HTTPS, FTP, and
>  more. It reduces bandwidth and improves response times by caching and
>  reusing frequently-requested web pages. Squid has extensive access
>  controls and makes a great server accelerator. It runs on most available
>  operating systems, including Windows and is licensed under the GNU GPL.
>
> but then I thought that maybe this was too nitpicky...  Either way, I
> agree with Bryce that this excerpt sounds a bit unprofessional, so maybe
> just remove it indeed.

+1, I will just replace the description with Sergio's suggestion here
and include it in the same MP for the entrypoint changes.

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

Thank you!

--
Athos Ribeiro