From 5400ae3f6f063f97ae0797624ffe1e7e3141205a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tadej=20Borov=C5=A1ak?=
 <70951+tadeboro@users.noreply.github.com>
Date: Tue, 18 May 2021 18:19:09 +0200
Subject: [PATCH] Ignore empty env variable values when updating them (#1552)

Right now, if the variable is not set, prerun routines will preped the
: to the variable value, which is not something we want. This commit
makes sure we properly handle the aforementioned situation and ignore
any empty values.
---
 src/ansiblelint/prerun.py |  5 +-
 test/test_prerun.py       | 96 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/src/ansiblelint/prerun.py b/src/ansiblelint/prerun.py
index 4bc66f694c..731b561db0 100644
--- a/src/ansiblelint/prerun.py
+++ b/src/ansiblelint/prerun.py
@@ -321,7 +321,10 @@ def _write_module_stub(
 def _update_env(varname: str, value: List[str], default: str = "") -> None:
     """Update colon based environment variable if needed. by appending."""
     if value:
-        value = [*os.environ.get(varname, default=default).split(':'), *value]
+        orig_value = os.environ.get(varname, default=default)
+        if orig_value:
+            # Prepend original or default variable content to custom content.
+            value = [*orig_value.split(':'), *value]
         value_str = ":".join(value)
         if value_str != os.environ.get(varname, ""):
             os.environ[varname] = value_str
diff --git a/test/test_prerun.py b/test/test_prerun.py
index b1efc34f86..edc100b2df 100644
--- a/test/test_prerun.py
+++ b/test/test_prerun.py
@@ -1,8 +1,10 @@
 """Tests related to prerun part of the linter."""
 import os
 
+import pytest
 from flaky import flaky
 
+from ansiblelint import prerun
 from ansiblelint.testing import run_ansible_lint
 
 
@@ -35,3 +37,97 @@ def test_prerun_reqs_v2() -> None:
     assert "Running ansible-galaxy role install" in result.stderr, result.stderr
     assert "Running ansible-galaxy collection install" in result.stderr, result.stderr
     assert result.returncode == 0, result
+
+
+def test__update_env_no_old_value_no_default_no_value(monkeypatch):
+    """Make sure empty value does not touch environment."""
+    monkeypatch.delenv("DUMMY_VAR", raising=False)
+
+    prerun._update_env("DUMMY_VAR", [])
+
+    assert "DUMMY_VAR" not in os.environ
+
+
+def test__update_env_no_old_value_no_value(monkeypatch):
+    """Make sure empty value does not touch environment."""
+    monkeypatch.delenv("DUMMY_VAR", raising=False)
+
+    prerun._update_env("DUMMY_VAR", [], "a:b")
+
+    assert "DUMMY_VAR" not in os.environ
+
+
+def test__update_env_no_default_no_value(monkeypatch):
+    """Make sure empty value does not touch environment."""
+    monkeypatch.setenv("DUMMY_VAR", "a:b")
+
+    prerun._update_env("DUMMY_VAR", [])
+
+    assert os.environ["DUMMY_VAR"] == "a:b"
+
+
+@pytest.mark.parametrize(
+    ("value", "result"),
+    (
+        (["a"], "a"),
+        (["a", "b"], "a:b"),
+        (["a", "b", "c"], "a:b:c"),
+    ),
+)
+def test__update_env_no_old_value_no_default(monkeypatch, value, result):
+    """Values are concatenated using : as the separator."""
+    monkeypatch.delenv("DUMMY_VAR", raising=False)
+
+    prerun._update_env("DUMMY_VAR", value)
+
+    assert os.environ["DUMMY_VAR"] == result
+
+
+@pytest.mark.parametrize(
+    ("default", "value", "result"),
+    (
+        ("a:b", ["c"], "a:b:c"),
+        ("a:b", ["c:d"], "a:b:c:d"),
+    ),
+)
+def test__update_env_no_old_value(monkeypatch, default, value, result):
+    """Values are appended to default value."""
+    monkeypatch.delenv("DUMMY_VAR", raising=False)
+
+    prerun._update_env("DUMMY_VAR", value, default)
+
+    assert os.environ["DUMMY_VAR"] == result
+
+
+@pytest.mark.parametrize(
+    ("old_value", "value", "result"),
+    (
+        ("a:b", ["c"], "a:b:c"),
+        ("a:b", ["c:d"], "a:b:c:d"),
+    ),
+)
+def test__update_env_no_default(monkeypatch, old_value, value, result):
+    """Values are appended to preexisting value."""
+    monkeypatch.setenv("DUMMY_VAR", old_value)
+
+    prerun._update_env("DUMMY_VAR", value)
+
+    assert os.environ["DUMMY_VAR"] == result
+
+
+@pytest.mark.parametrize(
+    ("old_value", "default", "value", "result"),
+    (
+        ("", "", ["e"], "e"),
+        ("a", "", ["e"], "a:e"),
+        ("", "c", ["e"], "e"),
+        ("a", "c", ["e:f"], "a:e:f"),
+    ),
+)
+def test__update_env(monkeypatch, old_value, default, value, result):
+    """Defaults are ignored when preexisting value is present."""
+    monkeypatch.setenv("DUMMY_VAR", old_value)
+
+    prerun._update_env("DUMMY_VAR", value)
+
+    assert os.environ["DUMMY_VAR"] == result