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

Update examples in documentation for env lookup plugin #62662

Merged
merged 2 commits into from Jan 23, 2020

Conversation

jtyr
Copy link
Contributor

@jtyr jtyr commented Sep 20, 2019

SUMMARY

The current env lookup plugin is setting the default value to empty string if the variable is not defined. This is nor particularly great as it doesn't allow to use the default() filter if the variable is not defined. It would be great if we could change the default value from empty string to None but that would break the backward compatibility. This PR is keeping the current behavior (non-existing variable returns empty string) and it's adding the possibility to define custom default value. It also adds some basic unit tests.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

env lookup plugin

ADDITIONAL INFORMATION
# This returns empty string
lookup('env', 'MYVAR')

# This returns the default value defined in the lookup plugin
lookup('env', 'MYVAR', default='myval')

# It also allows to use the default() filter if setting the default to None
lookup('env', 'MYVAR', default=None) | default('myval')

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. new_plugin This PR includes a new plugin. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 20, 2019
@flowerysong
Copy link
Contributor

You could instead use | default('myval', True), which returns the default for false values in addition to undefined.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 1, 2019
@samdoran
Copy link
Contributor

samdoran commented Oct 1, 2019

Rather than a change in behavior, we should add an example to the documentation of the solution @flowerysong suggested.

@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Oct 1, 2019
@jtyr
Copy link
Contributor Author

jtyr commented Oct 1, 2019

Adding this new feature doesn't change the behavior. It just allows to set default value using one filter. But I agree, that documenting the | default('myval', True) would be a good thing as well.

@sivel
Copy link
Member

sivel commented Oct 1, 2019

The consensus amongst all of the core devs today, was that this feature would not be accepted. Replacing this feature with a docs pull request would be.

@jtyr
Copy link
Contributor Author

jtyr commented Oct 1, 2019

OK. Let me amend the PR to document the use of the default() filter and adding the tests for the existing functionality of the lookup plugin.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. core_review In order to be merged, this PR must follow the core review workflow. labels Oct 21, 2019
@jtyr jtyr force-pushed the jtyr-lookup_env_default branch 6 times, most recently from c48669d to 7feb00b Compare October 23, 2019 13:00
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 23, 2019
@jtyr
Copy link
Contributor Author

jtyr commented Oct 23, 2019

@samdoran @sivel Please see the latest commit where I have removed my original implementation of the default value and just documented how to set default value via the default filter.

To be honest, I'm not happy about such implementation as it doesn't allow to differentiate between env variable which is not defined and env variable which is defined but has empty string as its value. This is what the original PR was facilitating.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 31, 2019
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Dec 27, 2019
@bcoca bcoca added docs This issue/PR relates to or includes documentation. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 9, 2020
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 9, 2020
@jtyr jtyr force-pushed the jtyr-lookup_env_default branch 4 times, most recently from ff6cc0b to 46430e0 Compare January 23, 2020 15:22
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 23, 2020
@samdoran samdoran changed the title Allow do define default value for the env lookup plugin Update examples in documentation for env lookup plugin Jan 23, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 23, 2020
@samdoran samdoran removed the new_plugin This PR includes a new plugin. label Jan 23, 2020
@samdoran samdoran merged commit 9130377 into ansible:devel Jan 23, 2020
@ansible ansible locked and limited conversation to collaborators Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. has_issue support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants