Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/common/gen/Configurator.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def __init__(
self.__compiled_regexes = {}

# Pre-defined exclusion sets for config processing
self.__excluded_prefixes = ("_", "PYTHON", "KUBERNETES_", "SVC_", "LB_", "SUPERVISOR_")
self.__excluded_prefixes = ("PYTHON", "KUBERNETES_", "NOMAD_", "SVC_", "LB_", "SUPERVISOR_")
self.__excluded_vars = frozenset(
{
"DOCKER_HOST",
Expand Down Expand Up @@ -111,6 +111,21 @@ def __init__(
else:
self.__variables = variables

# Allow a leading underscore prefix on env var names so that domain names
# starting with a digit can be configured (bash forbids var names starting
# with a digit, e.g. 1nteresting.io). Users write _1nteresting.io_USE_...
# and we strip the single leading underscore here before any other processing.
stripped: Dict[str, str] = {}
for k, v in self.__variables.items():
new_key = k.removeprefix("_")
if not new_key:
# Skip bare "_" or similar empty-after-strip keys
continue
if new_key in stripped:
self.__logger.warning(f"Variable collision after stripping leading underscore: both {k!r} and {new_key!r} exist, keeping last value")
stripped[new_key] = v
self.__variables = stripped
Comment on lines +114 to +127
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Collision warning message is misleading when the non-prefixed key is processed second.

When iteration order means _FOO is processed before FOO, the stripped key FOO is already in stripped. The current log reads:

"both 'FOO' and 'FOO' exist, keeping last value"

This doesn't identify the original underscore-prefixed key that caused the collision. Consider tracking which original key produced each entry in stripped so the warning can report both original names accurately.

🔧 Suggested fix
-        stripped: Dict[str, str] = {}
+        stripped: Dict[str, str] = {}
+        stripped_origins: Dict[str, str] = {}  # maps new_key -> original key
         for k, v in self.__variables.items():
             new_key = k.removeprefix("_")
             if not new_key:
                 # Skip bare "_" or similar empty-after-strip keys
                 continue
             if new_key in stripped:
-                self.__logger.warning(f"Variable collision after stripping leading underscore: both {k!r} and {new_key!r} exist, keeping last value")
+                self.__logger.warning(
+                    f"Variable collision after stripping leading underscore: "
+                    f"{stripped_origins[new_key]!r} and {k!r} both map to {new_key!r}, keeping value from {k!r}"
+                )
             stripped[new_key] = v
+            stripped_origins[new_key] = k
         self.__variables = stripped
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/common/gen/Configurator.py` around lines 114 - 127, The collision warning
in Configurator (where self.__variables is processed and assigned to stripped)
can report misleading names because it only shows the post-strip key; update the
logic to track the original source key for each entry (e.g., store new_key ->
(orig_key, value) while building stripped) so that when you detect a collision
you can call self.__logger.warning with both the previously stored orig_key and
the current k, and then store/overwrite the value with the current one before
finally collapsing the mapping back to new_key -> value for self.__variables;
adjust the warning text accordingly to show both original keys and that the last
value is kept.

Comment on lines +118 to +127
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

"Last wins" collision resolution is iteration-order dependent.

As per coding guidelines for src/common/gen/, determinism should be preserved—the same input should produce the same output. When both FOO and _FOO exist, the surviving value depends on dict iteration order, which may vary if variables originate from environment (historically unordered on older Pythons or external orchestrators) rather than file parsing.

Consider a deterministic tie-breaker, such as always preferring the explicit (non-underscore) key over the underscore-prefixed form.

♻️ Possible deterministic approach
         for k, v in self.__variables.items():
             new_key = k.removeprefix("_")
             if not new_key:
                 continue
-            if new_key in stripped:
-                self.__logger.warning(f"Variable collision after stripping leading underscore: both {k!r} and {new_key!r} exist, keeping last value")
-            stripped[new_key] = v
+            if new_key in stripped:
+                # Prefer explicit (non-prefixed) key; only overwrite if current key is non-prefixed
+                if k == new_key:
+                    self.__logger.warning(
+                        f"Variable collision: {new_key!r} overrides previously stripped key"
+                    )
+                    stripped[new_key] = v
+                else:
+                    self.__logger.warning(
+                        f"Variable collision: keeping explicit {new_key!r}, ignoring {k!r}"
+                    )
+            else:
+                stripped[new_key] = v
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
stripped: Dict[str, str] = {}
for k, v in self.__variables.items():
new_key = k.removeprefix("_")
if not new_key:
# Skip bare "_" or similar empty-after-strip keys
continue
if new_key in stripped:
self.__logger.warning(f"Variable collision after stripping leading underscore: both {k!r} and {new_key!r} exist, keeping last value")
stripped[new_key] = v
self.__variables = stripped
stripped: Dict[str, str] = {}
for k, v in self.__variables.items():
new_key = k.removeprefix("_")
if not new_key:
# Skip bare "_" or similar empty-after-strip keys
continue
if new_key in stripped:
# Prefer explicit (non-prefixed) key; only overwrite if current key is non-prefixed
if k == new_key:
self.__logger.warning(
f"Variable collision: {new_key!r} overrides previously stripped key"
)
stripped[new_key] = v
else:
self.__logger.warning(
f"Variable collision: keeping explicit {new_key!r}, ignoring {k!r}"
)
else:
stripped[new_key] = v
self.__variables = stripped
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/common/gen/Configurator.py` around lines 118 - 127, The collision
handling in the loop over self.__variables uses "last wins" which is
iteration-order dependent; change it to a deterministic rule that always prefers
the explicit non-underscore key over the underscore-prefixed form: when
computing new_key = k.removeprefix("_") and encountering new_key in stripped,
check whether the existing entry in stripped came from a non-prefixed original
(i.e., the existing source key did not start with "_") and if so keep the
existing value and do not overwrite; otherwise (existing was from a prefixed key
and current k is non-prefixed) overwrite; update the warning via
self.__logger.warning accordingly and ensure the logic references __variables,
new_key, stripped, and removeprefix so behavior is deterministic regardless of
iteration order.


self.__multisite = self.__variables.get("MULTISITE", "no") == "yes"
self.__servers = self.__map_servers()

Expand Down