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

allow to set an env var to undefined, aka default #75680

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Sep 10, 2021

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

config

@ansibot ansibot added affects_2.12 feature This issue/PR relates to a feature request. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. core_review In order to be merged, this PR must follow the core review workflow. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 10, 2021
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Sep 14, 2021
@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 Sep 22, 2021
@lilatomic
Copy link
Contributor

lilatomic commented Oct 2, 2021

Hey, maybe a silly question, but were you intending that users would set the envvar to literally 'AnsibleUndefined'? Or that they'd be able to get an actual instance of AnsibleUndefined into the environment variable?
I initially thought that it was the second of those, and I wanted to make sure that it played nice with the (coming) feature to create undefined variables directly. But I couldn't think of a way to set the envvar during config, which led me to believe that I was on the wrong track and overthinking it.

Anyhow, here's a patch with some tests I made while fiddling with this:

From 5dee184cf8987d4e8d7d8dd65ad885c4d0d6b79c Mon Sep 17 00:00:00 2001
From: Daniel Goldman <danielgoldman4@gmail.com>
Date: Sat, 2 Oct 2021 05:36:17 +0000
Subject: [PATCH] add tests for config.manager loading envvars

---
 test/units/config/test.yml        | 22 ++++++++++++++++++
 test/units/config/test_manager.py | 38 +++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/test/units/config/test.yml b/test/units/config/test.yml
index 384a055b2d..d1850362ef 100644
--- a/test/units/config/test.yml
+++ b/test/units/config/test.yml
@@ -28,6 +28,28 @@ config_entry_bool:
     <<: *entry
     type: bool
     default: False
+config_entry_bool_default_to_true:
+    name: test config
+    default: True
+    description:
+      - This does nothing, its for testing
+    env:
+      - name: BOOL_ENVVAR
+    ini:
+      - section: defaults
+        key: bool_key
+    type: bool
+config_entry_int:
+    name: test config
+    description:
+      - This does nothing, its for testing
+    type: int
+    default: 42
+    env:
+      - name: INT_ENVVAR
+    ini:
+      - section: defaults
+        key: int_key
 config_entry_list:
     <<: *entry
     type: list
diff --git a/test/units/config/test_manager.py b/test/units/config/test_manager.py
index a957e39749..314203411f 100644
--- a/test/units/config/test_manager.py
+++ b/test/units/config/test_manager.py
@@ -22,6 +22,8 @@ cfg_file2 = os.path.join(curdir, 'test2.cfg')
 expected_ini = {'CONFIG_FILE': Setting(name='CONFIG_FILE', value=cfg_file, origin='', type='string'),
                 'config_entry': Setting(name='config_entry', value=u'fromini', origin=cfg_file, type='string'),
                 'config_entry_bool': Setting(name='config_entry_bool', value=False, origin=cfg_file, type='bool'),
+                'config_entry_bool_default_to_true': Setting(name='config_entry_bool_default_to_true', value=True, origin='default', type='bool'),
+                'config_entry_int': Setting(name='config_entry_int', value=42, origin='default', type='int'),
                 'config_entry_list': Setting(name='config_entry_list', value=['fromini'], origin=cfg_file, type='list'),
                 'config_entry_deprecated': Setting(name='config_entry_deprecated', value=u'fromini', origin=cfg_file, type='string'),
                 'config_entry_multi': Setting(name='config_entry_multi', value=u'morefromini', origin=cfg_file, type='string'),
@@ -108,6 +110,42 @@ class TestConfigManager:
     def test_value_and_origin_from_alt_ini(self):
         assert self.manager.get_config_value_and_origin('config_entry', cfile=cfg_file2) == ('fromini2', cfg_file2)
 
+    @pytest.mark.parametrize(
+        ['env_value', 'expected_value', 'expected_origin'],
+        [
+            ('fromenviron', 'fromenviron', "env: %s" % 'ENVVAR'),
+            ('', '', "env: %s" % 'ENVVAR'),
+            ('AnsibleUndefined', 'DEFAULT', 'default'),
+        ])
+    def test_value_and_origin_from_env(self, monkeypatch, env_value, expected_value, expected_origin):
+        # monkeypatching the environment has to happen through the py3compat module
+        # since py3compat._TextEnviron holds a reference to the `os.environ` dictionary
+        # and so calling setattr on `os.environ` doesn't replace the reference held by py3compat
+        # we should be able to monkeypatch `os.environ` once py3compat is deprecated
+        from ansible.utils import py3compat
+        monkeypatch.setattr(py3compat.environ, '_raw_environ', {'ENVVAR': env_value})
+
+        assert ConfigManager(None, os.path.join(curdir, 'test.yml')).get_config_value_and_origin('config_entry') == (expected_value, expected_origin)
+
+    def test_value_and_origin_from_env_empty_and_wrong_type_defaults(self, monkeypatch):
+        from ansible.utils import py3compat
+        monkeypatch.setattr(py3compat.environ, '_raw_environ', {'INT_ENVVAR': ''})
+
+        assert ConfigManager(None, os.path.join(curdir, 'test.yml')).get_config_value_and_origin('config_entry_int') == (42, 'default')
+
+    @pytest.mark.parametrize(
+        ['env_key', 'config_key', 'expected_value'],
+        [
+            ('ENVVAR', 'config_entry_bool', False),
+            ('BOOL_ENVVAR', 'config_entry_bool_default_to_true', True),
+        ])
+    def test_value_and_origin_from_env_empty_and_bool(self, monkeypatch, env_key, config_key, expected_value):
+        """Test that the empty string is not cast into a bool and the default is fallen back on"""
+        from ansible.utils import py3compat
+        monkeypatch.setattr(py3compat.environ, '_raw_environ', {env_key: ''})
+
+        assert ConfigManager(None, os.path.join(curdir, 'test.yml')).get_config_value_and_origin(config_key) == (expected_value, 'default')
+
     def test_value_from_alt_ini(self):
         assert self.manager.get_config_value('config_entry', cfile=cfg_file2) == 'fromini2'
 
-- 
2.25.1

@bcoca
Copy link
Member Author

bcoca commented Oct 4, 2021

it is meant to allow setting but not setting an env var, which should trigger a default value

@lilatomic
Copy link
Contributor

I don't think I know what it means to do that. But it sounds like that's not what those test cases test for. Can you describe how I would do that? I can then fixup the test case.

Also, should we add to the docs about this feature?

@bcoca
Copy link
Member Author

bcoca commented Oct 5, 2021

get_config_value_and_origin would return the 'next' value/origin in set (observing normal precedence), if none are set, 'default' it is.

@lilatomic
Copy link
Contributor

Ahok, gotcha. I think I was confused by the 'AnsibleUndefined' on line 497. All that means is that, if the value happens to be AnsibleUndefined, we consider that a failure for the env lookup and keep going. In the more common case, if the value is None, we keep going. Like before.

And the second piece of modified code (at the end of the function) is just to catch-and-transform the ValueError which could arise from attempting to get the default if it's an empty envvar (happening inside the first try-catch).

I'll see about getting some tests in on the second part.

@lilatomic
Copy link
Contributor

I think I found a failing test case: config values requesting Booleans will cast the emptystring to False, which will be accepted as a valid boolean. This will therefore not fall back on the default value. I've updated the git patch above with these tests (and other which pass).

@bcoca
Copy link
Member Author

bcoca commented Oct 7, 2021

the reason i'm using a specific token is that 'empty string' is a valid way of saying 'false' in shell

@lilatomic
Copy link
Contributor

Oh, gotcha, so we should expect the boolean thing to fail. So the recommended usage to force the default is to set the envvar to AnsibleUndefined. Setting the envvar to '' will sometimes work, but is also valid for booleans and strings, so it won't always reset to the default.

Should we include a doc fragment to that effect with this MR?

@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 Apr 19, 2022
@bcoca bcoca marked this pull request as draft August 24, 2022 19:37
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Aug 24, 2022
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 12, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.12 feature This issue/PR relates to a feature request. 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. support:core This issue/PR relates to code supported by the Ansible Engineering Team. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants