Support multiple files in SENTRY_DOTENV_PATH #2454
Conversation
There was a problem hiding this comment.
@Kinqdos thanks for the contribution!
While I think the comma-separation makes sense, changing SENTRY_DOTENV_PATH to accept comma-separated paths would be a breaking change, since file names on UNIX can contain commas. For example, if someone currently sets SENTRY_DOTENV_PATH="path/with,comma/.env", your proposed change would cause this to be read as two separate paths (path/with and comma/.env) instead of as the single path path/with,comma/.env.
I think we may need to introduce a separate environment variable, such as SENTRY_DOTENV_PATHS (plural) which can take multiple files separated by commas. That way, if someone has an edge-case setup with dotenv path containing a comma, their setup won't break, but it is still possible to support your use case via the new environment variable
Would you be able to implement this, or should I take over?
|
@szokeasaurusrex Thank you for the feedback, I will make a new version ready. The plural variant should be preferred over the singular variant if both is set right? |
|
@Kinqdos i think either order of precedence is fine, please pick whatever is easiest for you to implement |
|
@szokeasaurusrex I pushed the new version, looking forward for your feedback. |
|
@szokeasaurusrex Hey any updates here? |
|
@Kinqdos I will try to take a look at some point this week. Unfortunately I currently have higher priority items on my list |
|
Hey @szokeasaurusrex just a friendly reminder, it would be great if you can take a look :) |
|
Hey @Kinqdos, thanks for your patience here. I left a comment; please also check the linter errors. I can also take over the PR from here if you would prefer |
|
If it speeds up the merge you can take over now. |
Made following changes to the original PR: - Only read one of `SENTRY_DOTENV_PATH` or `SENTRY_DOTENV_PATHS`, with the former taking precedence. - Filter out any empty paths in `SENTRY_DOTENV_PATHS`. - Only read the default .env when none of these variables are provided. This allows the user to suppress reading the default dotenv by passing an empty string to `SENTRY_DOTENV_PATHS` - Error when a custom dotenv path is not found (previously, we failed silently in this case, like we continue to do for the default .env, where it makes sense to have such behavior)
|
@loewenheim can you also review this contributor PR? I made some changes to finalize it prior to the merge Also, thanks @Kinqdos for the contribution and for your patience here 🚀 |
| } else { | ||
| // Fallback to default dotenv | ||
| dotenvy::dotenv() | ||
| .map_or_else(|e| if e.not_found() { Ok(()) } else { Err(e) }, |_| Ok(())) |
There was a problem hiding this comment.
Tbh I think this map_or_else is really hard to understand, but it was already that way before. This isn't a blocker IMO.
|
@szokeasaurusrex Is this ready to merge now? |
Added support for multiple files in SENTRY_DOTENV_PATH variable. Multiple files are separated with a comma and override each other.
See #2451