Skip to content

feat: Add new command option to set env file for pdm script #3358

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Zheaoli
Copy link
Contributor

@Zheaoli Zheaoli commented Dec 25, 2024

Pull Request Checklist

  • A news fragment is added in news/ describing what is new.
  • Test cases added for changed code.

Describe what you have changed in this PR.

Fix #3355

@Zheaoli Zheaoli marked this pull request as draft December 25, 2024 06:48
Signed-off-by: Manjusaka <[email protected]>
Signed-off-by: Manjusaka <[email protected]>
Copy link

codecov bot commented Dec 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.24%. Comparing base (19f84aa) to head (cf0608f).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3358      +/-   ##
==========================================
+ Coverage   85.21%   85.24%   +0.02%     
==========================================
  Files         112      112              
  Lines       11461    11472      +11     
  Branches     2505     2507       +2     
==========================================
+ Hits         9767     9779      +12     
+ Misses       1168     1167       -1     
  Partials      526      526              
Flag Coverage Δ
unittests 85.04% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Manjusaka <[email protected]>
@Zheaoli Zheaoli marked this pull request as ready for review December 25, 2024 07:28
Copy link
Collaborator

@frostming frostming left a comment

Choose a reason for hiding this comment

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

I would like to suggest some improvements

The env files can be cascaded in a way that more specific env file can override less specific ones. For example, if PDM_ENVIRONMENT=prod, we look for the following files and load them if exists:

  1. .env (effective for all environments)
  2. .env.prod (effective for prod only)
  3. .env.local (effective for all environments)
  4. .env.prod.local (effective for prod only)

The variables are merged in the above order, with the latter taking precedence.

The improvement is to make .local a special modifier rather than a regular environment name. It contains the env vars for the current machine only, and .env*.local should be added to the .gitignore, while other .env files can be shared safely.

Also happy to hear @noirbizarre 's thoughts since he is the one involved in PDM scripts the most.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Dec 25, 2024

  1. .env (effective for all environments)
  2. .env.prod (effective for prod only)
  3. .env.local (effective for all environments)
  4. .env.prod.local (effective for prod only)

The variables are merged in the above order, with the latter taking precedence.

The improvement is to make .local a special modifier rather than a regular environment name. It contains the env vars for the current machine only, and .env*.local should be added to the .gitignore, while other .env files can be shared safely.

Nice idea! I will update it ASAP

@noirbizarre
Copy link
Member

noirbizarre commented May 16, 2025

Sorry, I thought I answered this long ago, but it wasn't the case.

So here's my position:

I see value in the environment concept, but I consider that removing the env_file option from scripts is not a good idea (in my case I have dozen of corporate projects relying on this, with different values per script, the environment approach would not fit/scale those use cases). I also consider having an automatic env file loading a source of problems (you have no idea how much time I spent debugging just because an env file was silently loaded by a tool, and we were not aware of). So I would be against in the current form.

However, I have a counterproposal:

I see 2 main use cases for dotenv files:

  • user dotenv (the classical .env): not committed, used by devs to export their personal settings, credentials... Standard and often shared by multiple apps (like docker-compose) by default. Often managed by the devs in their own environment (direnv, mise,...)
  • project wide dotenvs, committed, tests and tooling may rely on it, this is most likely the kind of dotenv we will have in env_file in the scripts.

I also like the idea of cascading env files.

So, I would:

  • support a opt-in user dotenv autoloading, with a setting, either a boolean to force the standard .env, either a configurable string (but it is important for people having their own dotenv loader like direnv or mise to avoid forcing them into a double loading, most probably with different capabilities)
  • support the environment concept with the PDM_ENVIRONMENT env var and the -e/--environment with default on local like done in this PR (think that devs may want to set PDM_ENVIRONMENT in their .env, it should be loaded first if the option is enabled)
  • make the env_file support a {environment} placeholder so it is possible to write task.env_file = ".env.{environment}" (or _.env_file to make it global) while keeping backward compatibility
  • make env_file also support a list of env_file so it is possible to have cascading (it should also be failsafe so it is possible to list optional dotenvs the user may add or not)

Then this should work:

_.env_file = [
  ".env.{environment}",
  ".env.{environment}.local",
]

But also this:

my-task.env_file = [
  "dotenvs/my-task",
  "dotenvs/my-task.{environment}",
  "dotenvs/my-task.{environment}.local",
]

Bonus: the environment could also be exposed as script placeholder

What do you think ?

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.

Allow inject enviroment variables via .env or other env file
3 participants