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

Stop change CWD to .env/.flaskenv location #3560

Merged
merged 2 commits into from
Jul 18, 2020
Merged

Conversation

greyli
Copy link
Member

@greyli greyli commented Apr 7, 2020

By the current implementation, if .env or .flaskenv was found in the top-level directory, Flask will change the current work directory to the directory that contains the .env or .flaskenv file.

When the user accidentally put a .env or .flaskenv in the top-level directory, then executing flask run in the current directory (contains app.py) will throw out NoAppException.

See more in #3561.

@davidism
Copy link
Member

@jab any thoughts on this, especially in relation to #3108? I think your projects had setups that were affected by the cd.

@jab
Copy link
Member

jab commented Apr 13, 2020

Thanks for the ping on this, @davidism. I can confirm these changes don't break anything in my customized Flask CLI (since mine does some more sophisticated detection of .env files that is independent of Flask's built-in detection, which I can explain more in #3108 or elsewhere).

And these changes seem good to me otherwise. Silently changing the CWD based on the presence/location of (hidden) .env files seems like it can break things in a hard-to-debug way for users. I may be missing something, since I'm not clear on what the benefit was in the first place. I'd expect users needing to compute paths relative to their app's modules should be doing so in a CWD-independent way (e.g. using __file__). If they're not, I guess this change could break them. But even if it does, it seems the benefits here outweigh that cost.

The only thing missing before I think this can be merged is an update to the changelog.

Thanks @greyli for contributing this!

@davidism davidism added this to the 2.0.0 milestone Apr 13, 2020
@jab
Copy link
Member

jab commented Jul 18, 2020

This looks good to me, and no one's raised any concerns, so I'm going to merge this before it stalls any longer.

@jab jab merged commit a40c381 into pallets:master Jul 18, 2020
@greyli greyli deleted the fix-env-chdir branch July 19, 2020 01:10
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants