Hi willy and thanks for the review. Indeed I forgot about the dummy part, changes present in the last patch.
On Thu, Jan 27, 2022 at 5:18 PM Willy Tarreau <[email protected]> wrote: > Hi David, > > finally back to your patches. There are very minor things but I'm not > going to modify them without your consent since they're signed-off. > > > From eae56a44d3bf51f8e92f66baef25f2a417f837ce Mon Sep 17 00:00:00 2001 > > From: David Carlier <[email protected]> > > Date: Fri, 21 Jan 2022 20:46:40 +0000 > > Subject: [PATCH 1/3] MEDIUM: deviceatlas new optional data file > download scheduler service. > ^^^ > Please always place a colon after the subsystem. Also, previously it > was referred to as "da", it would be better to stay consistent in order > to more easily list all such related changes. > > > New specialized service to daily handle the update of download file > without > > interruption of service and to be preemptively started before HAProxy. > > While I do know from your private explanation that it's an external > service, it's not immediately obvious to the reader. Here it would > be welcome to mention that it's a standalone utility that accesses > a shared memory used by the DA module and explain a little bit how > it works (the general principles and choices). And please mention it > uses libcurl since it's a new dependency, this may save some trial > and errors to those in the process of a bisect for example. > > Also, I noticed that it doesn't build with the dummy lib. Since it's > a standalone tool I don't think it's a problem, but this really needs > to be mentioned in the commit message, ideally with a reminder about > what's missing (either a link to the download site or an invitation > to read build instructions from doc/DeviceAtlas-device-detection.txt). > > > Signed-off-by: David Carlier <[email protected]> > > --- > > addons/deviceatlas/Makefile | 49 +++++++++ > > addons/deviceatlas/dadwsch.c | 195 +++++++++++++++++++++++++++++++++++ > > 2 files changed, 244 insertions(+) > > create mode 100644 addons/deviceatlas/Makefile > > create mode 100644 addons/deviceatlas/dadwsch.c > (...) > > When I applied it I got these warnings: > > $ git am 0001-MEDIUM-deviceatlas-new-optional-data-file-download-s.patch > Applying: MEDIUM: deviceatlas new optional data file download scheduler > service. > .git/rebase-apply/patch:14: trailing whitespace. > # DEVICEATLAS_SRC : DeviceAtlas API source root path > .git/rebase-apply/patch:62: new blank line at EOF. > + > warning: 2 lines add whitespace errors. > > One of them is at the end of the line below: > > > diff --git a/addons/deviceatlas/Makefile b/addons/deviceatlas/Makefile > > new file mode 100644 > > index 000000000..33dd9434e > > --- /dev/null > > +++ b/addons/deviceatlas/Makefile > > @@ -0,0 +1,49 @@ > > +# DEVICEATLAS_SRC : DeviceAtlas API source root path > ^ here > > An easy and convenient way to catch them before committing (or see them > after to ease their fixing) is to add this into your $HOME/.gitconfig: > > [color] > ui = true > > A "git diff" or "git show" will show them on a pretty visible red > background, > that's how most of us avoid and fix them.. > > The second one is the empty line at the end of the Makefile. > > Aside these details it's OK for me. > > Willy >
0001-MEDIUM-da-new-optional-data-file-download-scheduler-.patch
Description: Binary data
