Hi Jack,
On Wed, May 20, 2020 at 05:23:08PM +0200, PR Bot wrote:
> Author: Jack Neely <jjne...@42lines.net>
> Number of patches: 1
>
> This is an automated relay of the Github pull request:
> The agent-check fail state is represented as "fail"
>
> Patch title(s):
> The agent-check fail state is represented as "fail"
>
> Link:
> https://github.com/haproxy/haproxy/pull/642
>
> Edit locally:
> wget https://github.com/haproxy/haproxy/pull/642.patch && vi 642.patch
>
> Apply locally:
> curl https://github.com/haproxy/haproxy/pull/642.patch | git am -
>
> Description:
> Documentation has stated the string is "failed" and this doesn't match
> the source code. An agent-check returning "failed" causes HAProxy to
> not make state changes.
Very interesting catch!
However I don't agree with only changing the doc because the doc is a
spec that developers use to create their code. Someone might have
written an agent that works well against the doc and not necessarily
against haproxy and we can't change that afterwards.
But looking at this part, I'm seeing that the initial spec did mention
"fail", just like the code and comments everywhere, and it's just when
I reworded that part to extend the agent's language in 1.5-dev26 in 2013
(commit 81f5d94a0be) that I mistakenly wrote "failed" instead of "fail"
in the doc.
So now the situation is:
- the code has always exclusively checked for "fail"
- the doc prior to 1.5-dev26 used to mention "fail"
- the doc since 1.5-dev26 has always mentioned "failed"
There definitely are agents that pre-date this extension, and it's almost
certain that some people wrote their own agents since 2013 without having
an immediate access to haproxy for a test.
Thus what I'd suggest would be the following plan:
1) we modify the code to also validate "failed" as a valid keyword
(there's a single "strcasecmp" invocation to update) and we put
a comment there referencing this discussion for the background.
2) we update the doc to document "fail" as you did with no mention
of "failed" so that those who figure this potential incompatibility
are encouraged to update their code (and test it), and are not
encouraged to keep the old keyword that will not work with any
of the supported versions in field.
3) we backport this to all branches.
Please also have a look at CONTRIBUTING for your next patch. Do not
hesitate to create an issue so as not to lose it if you don't have
time to rework your patch right now.
Thanks!
Willy
