pdm icon indicating copy to clipboard operation
pdm copied to clipboard

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

Open Zheaoli opened this issue 1 year ago • 3 comments

Pull Request Checklist

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

Describe what you have changed in this PR.

Fix #3355

Zheaoli avatar Dec 25 '24 06:12 Zheaoli

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 25 '24 07:12 codecov[bot]

  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

Zheaoli avatar Dec 25 '24 18:12 Zheaoli

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 ?

noirbizarre avatar May 16 '25 00:05 noirbizarre