Hi Massimiliano,
On Mon, Jul 03, 2017 at 02:55:24PM +0200, Scientiamobile wrote:
> Hi Willy,
>
> a small patch to fix a problem on Scientiamobile WURFL module's "Engine
> Target" option
Could you please provide a commit message for this fix explaining what
it's about, what is the expected visible effect for the users and if
it should be backported or not (since it's a bug fix) ?
Also I'm having some comments below :
> +For this reason, since HAProxy versions greater than 1.7.7, in order to build
> +WURFL Device Detection support, Scientiamobile InFuze C API version 1.9 or
> +greater is required.
1.7.7 is the 7th fix release of the stable 1.7 branch, so we're not changing
an API in the middle of a stable branch. That's possible for 1.8 though, but
please be nice to limit changes are we're long past the feature freeze (03-31)
in order to limit risk of cross-breakage and distraction caused by merging
code in various areas preventing development progress.
If there are issues with this version of the API affecting branch 1.7, it
needs to be worked around in the code in a way that allows them to upgrade
but doesn't force them to, otherwise you'll simply block some users at 1.7.7
because for various reasons it will be a pain for them to upgrade multiple
dependencies at once. APIs are something very important not to break.
> --- a/doc/configuration.txt
> +++ b/doc/configuration.txt
> @@ -57,7 +57,7 @@ Summary
> 4.2. Alphabetically sorted keywords reference
>
> 5. Bind and Server options
> -5.1. Bind options
> +5.1. Find options
> 5.2. Server and default-server options
> 5.3. Server DNS resolution
> 5.3.1. Global overview
This change is wrong, the correct word is "Bind", not "Find". Please be
careful not to accidently change random parts of the doc.
> +wurfl-engine-mode
> + ************** IMPORTANT: Decommissioning of Engine Mode Options
> *************
> + Prior to version 1.9 of the Scientiamobile InFuze C API, users could choose
> + between Accuracy and Performance engine optimization options. These options
> + had been introduced years ago to manage the behavior of certain web
> browsers
> + and their tendency to present "always different" User-Agent strings that
> would
(...)
Please stick to keyword description in the config documentation, and avoid
providing history. We used to do that mistake in the very first documentation
and it became a real mess where multiple versions were mentionned consecutive
to multiple changes. Since the code and the doc are versionned, only document
how the updated version works, even if that includes removing a keyword,
simply delete it, or be nice, just mention it as "obsolete".
> diff --git a/src/wurfl.c b/src/wurfl.c
> index 18723a6..20fdc84 100644
> --- a/src/wurfl.c
> +++ b/src/wurfl.c
> @@ -50,11 +50,13 @@ typedef struct {
> struct ebmb_node nd;
> } wurfl_data_t;
(...)
> - memprintf(err, "WURFL: %s valid values are %s or %s.\n", args[0],
> HA_WURFL_TARGET_PERFORMANCE, HA_WURFL_TARGET_ACCURACY);
> + if (!strcmp(args[1],HA_WURFL_TARGET_DEFAULT)) {
> + global.wurfl.engine_mode = WURFL_ENGINE_TARGET_DEFAULT;
> + return 0;
> + }
This change was not made against the updated version, there's no "wurfl"
member in the global section anymore, this moved to "global_wurfl" inside
wurfl.c in last december, I think you did it against an outdated version.
As indicated in CONTRIBUTING, changes must always be provided against the
latest development version.
Thanks!
Willy
