Added +1 to the end in random filter so that it was inclusive

[zerkms]

SUMMARY

This PR addresses #27214

The documentation states that the end fraction of the random filter is inclusive, so I believe it's safe to assume everybody who read the documentation relied on it, so this change is not a breaking change.

Fixes #27214

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

core
filters

ANSIBLE VERSION

ansible 2.3.1.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.6 (default, Oct 26 2016, 20:30:19) [GCC 4.8.4]

ADDITIONAL INFORMATION

[JamesUoM]

SUMMARY

The documentation states that the end fraction of the random filter is inclusive, so I believe it's safe to assume everybody who read the documentation relied on it, so this change is not a breaking change.

Well the Ansible docs didn't make it clear whether it was inclusive or not and I tested it on 2.3 and found that it wasn't and wrote my task accordingly. I got the docs changed to imply it was between not inclusive. #33226. Now you've changed it to be inclusive but not reverted the docs.

Finally as this was a change in behaviour rather than bug fix it should have been in the porting notes.

[maxamillion]

@zerkms we need a docs fix ^^^

[bcoca]

@maxamillion no, i think we need to return to the documented behaviour, since it was already 'documented' and it is not backwards compatible

[maxamillion]

@bcoca alright, want me to revert?

[bcoca]

yep, original issue was already fixed by the 'doc update' and this ends up creating more issues instead

if you want to add new switch to be 'inclusive=false|true' to allow for both behaviors while being backwards compatible, even better.

[zerkms]

The original documentation in its examples implied it's inclusive.

People relied on what was documented. Changing documentation is a breaking change, this PR does not BC anything, it just fixes code to align with the documentation.

#33226 - is a breaking change

For instance

"{{ 59 |random}} * * * * root /script/from/cron"

this is a valid example from the doc. For those who trusted it and implemented it like that - they expected to have an even distribution of the random numbers for the whole hour, if this PR is unmerged - all those people would lose one last minute of an hour. One cannot simply change the documentation.

Another example:

"{{ ['a','b','c']|random }}"
{{ 101 |random(step=10) }}

(now) The first line is inclusive, the second is exclusive. WAT.

[bcoca]

but it will break existing playbooks that rely on it NOT being inclusive, the docs were fixed as they were incorrect and didn't clearly state how it worked.

[zerkms]

@bcoca documentation examples were inclusive, see the cron example, and the enumeration example.

If you did read the documentation exactly - you would implement your playbooks with that in mind.

"{{ 59 |random}} * * * * root /script/from/cron"

this example makes only sense if it was inclusive.

[bcoca]

mostly, its still valid to not want minute 59 exactly, its 'implied' but not stated, that is why we updated them

[zerkms]

@bcoca honestly, does it make any sense for you to provide an example that deliberately omits 59 and not mention it?

Before it was implied. Now it's stated explicitly but with different semantics, hence it's a BC.

But whatever, you develop this project.

Wondering how many people now need to change their playbooks (at least me).

[bcoca]

That is not the point, the examples still work but not as implied, the problem with changing the code is that existing playbooks WILL break as the limit changes, while changing docs allows people to correct their usage of it willingly, not because a playbook now suddenly behaves differently.

I agree with you on what the problem was, the solution has to take into account NOT breaking existing playbooks, which the doc fix accomplishes but the behaviour change does not.

[zerkms]

existing playbooks WILL break as the limit changes

that's not true: those who read the documentation exactly assumed the intervals are inclusive.

not because a playbook now suddenly behaves differently.

it does not: behaviour was implicitly set by the documentation

NOT breaking existing playbooks, which the doc fix accomplishes but the behaviour change does not.

it totally breaks now: people who relied on random values to be inclusive now must change it everywhere. What I literally will have to do is to grep everything for random and manually change. If I must change my code to comply with the new version it's a BC.

[bcoca]

@zerkms sadly most people don't read the docs and just use the code with their own assumptions, which we have learnt in this project time and time again.

Changing the docs will clarify it for the people that actually do read them, but the people that were using the feature 'as it works' will not see their playbook's behaviour change. I don't disagree with you in principal, but given the choice i have to take the one the preserves current behaviour and make documentation match it vs the other way around.

[zerkms]

Yep, I would gladly accept any solution, my apologies for heating up this discussion :-(.

[bcoca]

@zerkms I understand and agree that your fix is 'more correct' but I have to look at it form the point of preserving existing behaviour unless it is clearly wrong. In this case it not really right or wrong to be inclusive/exclusive both work ... as long as we clearly state which one we use, so we are mostly forced to continue with behaviour and correct documentation, assumptions might need to be updated, but playbooks won't suddenly behave differently w/o users updating them.