Skip to content

Conversation

@rolodato
Copy link
Contributor

@rolodato rolodato commented Apr 7, 2025

In #148, we changed the config behaviour so that invalid or missing configurations are rejected with an error. This effectively broke edge-proxy-render-config when called without environment variables, which also broke the Docker build: https://github.com/Flagsmith/edge-proxy/actions/runs/14308473957/job/40097587318

Because a config file is no longer required, and we don't need an empty/placeholder config file to show a useful error message, we now don't render a config file at all in the Docker build. If the config file is not found, we now log a message and don't fail with an error. This does not break customers who are mounting/copying their config file into the Edge Proxy container.

This is what running the container with no environment variables or config files looks like now:

2025-04-07 13:52:39 [info     ] Configuration file at config.json not found
Traceback (most recent call last):
  File "/opt/venv/bin/edge-proxy-serve", line 8, in <module>
    sys.exit(serve())
             ^^^^^^^
  File "/app/src/edge_proxy/main.py", line 7, in serve
    settings = get_settings()
               ^^^^^^^^^^^^^^
  File "/app/src/edge_proxy/settings.py", line 153, in get_settings
    return AppConfig()
           ^^^^^^^^^^^
  File "/opt/venv/lib/python3.12/site-packages/pydantic_settings/main.py", line 84, in __init__
    super().__init__(
  File "/opt/venv/lib/python3.12/site-packages/pydantic/main.py", line 176, in __init__
    self.__pydantic_validator__.validate_python(data, self_instance=self)
pydantic_core._pydantic_core.ValidationError: 1 validation error for AppConfig
environment_key_pairs
  Field required [type=missing, input_value={}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.7/v/missing

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

I was initially concerned about this PR as I understood that the hierarchy of settings sources as defined here was an 'all-or-nothing' situation. Based on this, my expectation was that, if someone was using environment variables for their configuration, it should never try to load the config file, and hence we should not log the info message.

In fact, the behaviour is that each setting can use the hierarchy, so we might have some settings that are defined by env vars, and some defined in the config file.

Based on this, I think it is perfectly reasonle to return an empty dictionary for the settings sourced from the config file, if one doesn't exist.

@rolodato rolodato merged commit 115c56b into main Apr 8, 2025
2 checks passed
@rolodato rolodato deleted the fix/render-config branch April 8, 2025 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants