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

give priority to '-e/--env-file' option over default files (#5628) #5629

Closed
wants to merge 2 commits into from

Conversation

jalaj711
Copy link

@jalaj711 jalaj711 commented Nov 7, 2024

This PR fixes issue #5628 . The issue arose from the fact that FlaskGroup().make_context() was loading the default env files before loading the arguments (by calling super().make_context(). Simply reversing the order of these actions fixes the issue. I have added the relevant tests to make sure it works.

  • Add tests that demonstrate the correct behavior of the change. Tests
    should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.

@jalaj711
Copy link
Author

jalaj711 commented Nov 7, 2024

The failing test seems to be failing in other actions before this PR as well: https://github.com/pallets/flask/actions/runs/11709066697/job/32612272783

@davidism
Copy link
Member

davidism commented Nov 7, 2024

I don't think this is going to work. The env vars need to be loaded before creating the context, as the processing pipeline needs to be able to see the env vars to use their values for the parameters.

…ult files

In the previous commit, pallets#5628 was fixed by creating the context
before loading the default environment variables. However, as
pointed out by @davidism, this can cause issues with the load
pipeline. Thus, this commit reverses that approach and uses the
`override` argument provided by `python-dotenv` to give priority
to custom file over default file.

Signed-off-by: Jalaj <[email protected]>
@davidism
Copy link
Member

davidism commented Nov 7, 2024

override is also not correct, as os.environ should still take precedence.

@davidism
Copy link
Member

davidism commented Nov 7, 2024

I'm also already working on this using a different approach, sorry for the confusion.

@jalaj711
Copy link
Author

jalaj711 commented Nov 7, 2024

I'm also already working on this using a different approach, sorry for the confusion.

Okay, no worries. Should I close this PR then?

@davidism
Copy link
Member

davidism commented Nov 7, 2024

Closing in favor of #5630

@davidism davidism closed this Nov 7, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants