Skip to content

Conversation

@matthewelwell
Copy link
Contributor

@matthewelwell matthewelwell commented Jan 22, 2025

Related to this issue.

The purpose is to prevent the application from hard crashing when unrecognised environment variables are provided.

@matthewelwell matthewelwell changed the title fix: try ignoring extra in AppConfig fix: ignore unknown env vars Jan 22, 2025
Copy link
Contributor

@rolodato rolodato left a comment

Choose a reason for hiding this comment

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

The error is not about unknown environment variables, it only happens if you try to use defined aliases

validation_alias=AliasChoices(
"api_poll_frequency_seconds",
"api_poll_frequency",


class AppConfig(AppSettings, BaseSettings):
class Meta:
extra = "ignore"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... yes, but it's overridden specifically in the BaseSettings class as per this.

That being said, I expected (based on this) that something like docker run --rm -e FOO_BAR=foo flagsmith/edge-proxy:2.17.0 would also generate the error, but it didn't.

Probably more investigation is needed, although the solution I added here does prevent the error reported in the issue when using API_POLL_FREQUENCY, but perhaps it also has some other knock on effects that need investigating before (if) we merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested the behaviour and verified that the code here does resolve the issue - it prevents the exception as well as behaving as expected if you to set API_POLL_FREQUENCY environment variable (or any other aliased name of a given setting).

I think we should go ahead and merge this.

@matthewelwell matthewelwell force-pushed the fix/ignore-unkown-env-vars branch from 376f357 to 92a2142 Compare January 22, 2025 16:43
@matthewelwell matthewelwell requested review from rolodato and removed request for rolodato February 10, 2025 15:35
@matthewelwell matthewelwell marked this pull request as ready for review February 11, 2025 11:52
@khvn26 khvn26 self-requested a review February 11, 2025 12:47
@matthewelwell matthewelwell merged commit 9638ca2 into main Feb 14, 2025
3 checks passed
@matthewelwell matthewelwell deleted the fix/ignore-unkown-env-vars branch February 14, 2025 11:39
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.

4 participants