Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend boolean values section in playbooks_variables #1285

Open
wants to merge 10 commits into
base: devel
Choose a base branch
from

Conversation

skilyazhnev
Copy link

Hello!

in continuation of this issue

#1279

@ansible-documentation-bot ansible-documentation-bot bot added the new_contributor This PR is the first contribution by a new community member. label Apr 18, 2024
@ansible-documentation-bot
Copy link
Contributor

Thanks for your Ansible docs contribution! We talk about Ansible documentation on matrix at #docs:ansible.im and on libera IRC at #ansible-docs if you ever want to join us and chat about the docs! We meet there on Tuesdays (see the Ansible calendar) and welcome additions to our weekly agenda items - scroll down to find the upcoming agenda and add a comment to put something new on that agenda.

@oraNod oraNod added backport-2.15 Automatically create a backport for the stable-2.15 branch backport-2.16 Automatically create a backport for the stable-2.16 branch backport-2.17 Automatically create a backport for the stable-2.17 branch labels Apr 19, 2024
@skilyazhnev skilyazhnev requested a review from bcoca April 22, 2024 10:55
@oraNod oraNod added the backport-2.14 Automatically create a backport for the stable-2.14 branch label Apr 22, 2024
Copy link
Contributor

@oraNod oraNod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @skilyazhnev

@skilyazhnev skilyazhnev requested a review from ssbarnea May 7, 2024 08:03
Co-authored-by: Don Naro <[email protected]>
Co-authored-by: Don Naro <[email protected]>
Copy link
Contributor

@oraNod oraNod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor nits but the rest LGTM

docs/docsite/rst/playbook_guide/playbooks_variables.rst Outdated Show resolved Hide resolved
docs/docsite/rst/playbook_guide/playbooks_variables.rst Outdated Show resolved Hide resolved
docs/docsite/rst/playbook_guide/playbooks_variables.rst Outdated Show resolved Hide resolved
@oraNod
Copy link
Contributor

oraNod commented Jun 4, 2024

@ssbarnea and @bcoca Please take a minute to review the latest round of edits. The details seem good to me and I'd like to get them merged.

Thanks again for the docs contribution @skilyazhnev

=============================================================================================== ====================================================================
``True`` , ``'true'`` , ``'t'`` , ``'yes'`` , ``'y'`` , ``'on'`` , ``'1'`` , ``1`` , ``1.0`` Truthy values
================= =================
Valid values Description
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not really 'ansible native' but YAML that uses true and false and the rest of the list below, it would be 'more true' that True and False are the 'native' options as Ansible evaluation happens inside Python and those are the 'Python native booleans', this also depends on context as inside a {{ }} expression it is Jinja doing the evaluation, which is again using Python under the hood.


=============================================================================================== ====================================================================
.. table::
Copy link
Member

@bcoca bcoca Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bool filter, while is it Jinja, it is an Ansible customization which approximates (but does not really match) the YAML booleans, the table below is incorrect as this is the actual code:

    if a is None or isinstance(a, bool):
        return a
    if isinstance(a, string_types):
        a = a.lower()
    if a in ('yes', 'on', '1', 'true', 1):
        return True
    return False

so it realy only converts 'matching True' and everything else is False

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though this is something I'm planning to fix so they are closer to YAML by default but can be 'pythonic' otherwise.

@brlin-tw
Copy link
Contributor

brlin-tw commented Aug 26, 2024

Note that YAML 1.2 drops all of the non-upper/lowercase-variant-true-false boolean variants, we may want to discourage people from using deprecated alternative boolean value styles.

Spec

@bcoca
Copy link
Member

bcoca commented Aug 26, 2024

brlin-tw not an issue until 2 things happen:

  • pyyaml releases a 1.2 compatible version
  • that version of pyyaml is widely adopted by OS/distributions

While the first will possibly happen soon, the second will take a long time, so we can defer the change until the first actually happens.

@oraNod
Copy link
Contributor

oraNod commented Sep 11, 2024

@bcoca What about adding a note that discourages the alternative boolean value styles and clearly stating that they will be deprecated in future?

Is there anything else holding this PR up?

@bcoca
Copy link
Member

bcoca commented Sep 11, 2024

| and clearly stating that they will be deprecated in future?

will they?

@oraNod
Copy link
Contributor

oraNod commented Sep 11, 2024

| and clearly stating that they will be deprecated in future?

will they?

fair point. I reread your comment about pyyaml. what if we come up with some other phrasing to discourage the use of alternative boolean styles? I mean we do have a linter that already complains about anything other than true | false.

@bcoca
Copy link
Member

bcoca commented Sep 11, 2024

The linter is expressing an opinion, it might not be the correct one for all contexts

@oraNod
Copy link
Contributor

oraNod commented Sep 11, 2024

yeah, linters tend to be opinionated about things. I'm probably straying from the point, which is about using the | bool filter and values being interpretable as boolean. I don't want to be a pain about it.

@bcoca would it work to remove the Interpretable as boolean table and focus more on the code for the bool filter that you pointed out? my thinking was to give a brief description instead along the lines of:

"Using True and False for boolean values is generally a good choice because they are clear and unambiguous. These values are also likely to be more compatible across versions. Standardizing on True and False also makes code more consistent and aids maintainability. Additionally these values avoid the need to for Ansible to interpret various input types, which can lead to unexpected behavior in some cases such as logical expressions."

then we could show the example.

of course I could trim down the details about using True and False if that's a little too much.

thanks.

@oraNod
Copy link
Contributor

oraNod commented Sep 12, 2024

Hi @skilyazhnev We've made some changes to the underlying readthedocs project for this repository. I'm going to close and re-open this issue to kick off a new PR preview build.

You might notice there will be two checks for readthedocs. One for docs/readthedocs.org:stage-ansible-core and another for docs/readthedocs.org:ansible-core.

The stage-ansible-core check is for the old project and will result in a 404. Please ignore that and use the ansible-core check only.

Thank you for your patience and understanding. Also thanks for your contribution to the Ansible documentation. If you have any questions or concerns, please do let us know either here, in the docs channel on Matrix, or in the Ansible forum. Cheers.

@oraNod oraNod closed this Sep 12, 2024
@oraNod oraNod reopened this Sep 12, 2024
@oraNod oraNod added the backport-2.18 Automatically create a backport for the stable-2.18 branch label Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-2.14 Automatically create a backport for the stable-2.14 branch backport-2.15 Automatically create a backport for the stable-2.15 branch backport-2.16 Automatically create a backport for the stable-2.16 branch backport-2.17 Automatically create a backport for the stable-2.17 branch backport-2.18 Automatically create a backport for the stable-2.18 branch new_contributor This PR is the first contribution by a new community member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants