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

feat(core): load environment variables from configuration name #17335

Conversation

AgentEnder
Copy link
Member

@AgentEnder AgentEnder commented May 31, 2023

Adds support for loading environment variables from .env.[configurationName] and .[configurationName].env

Fixes: #17686

@AgentEnder AgentEnder requested review from a team as code owners May 31, 2023 15:55
@vercel
Copy link

vercel bot commented May 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 21, 2023 10:39pm

@nx-cloud
Copy link

nx-cloud bot commented May 31, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 1dd261d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@SmoshySmosh
Copy link
Contributor

This solution does not provide the proper documentation and it is not clear how to use it without having deeper knowledge of the internals of NX.

I originally explored this as a quick and easy solution but I quickly realized it was untenable due to the synchronization overhead it would add to a mono repo of any size. This change requires a re-definition of every configuration in every target of every package in your mono repo, which you then need to keep in sync as you add new packages. We should think through this problem a little more and find a solution that both decreases the complexity of NX and serves the need of NX's customers.

For example, this is what every target will have to become to support .beta.env, .gamma.env, and .prod.env

"targets: {
  ...,
  "e2e": {
    ...
    "configurations": {
      "beta": {},
      "gamma": {},
      "prod": {},
    }
  }
}

@AgentEnder AgentEnder force-pushed the feat/set-environment-variables-from-configuration-name branch from 48f0748 to a53be8f Compare May 31, 2023 17:52
@AgentEnder AgentEnder force-pushed the feat/set-environment-variables-from-configuration-name branch from a53be8f to 7781d49 Compare June 20, 2023 16:04
@AgentEnder
Copy link
Member Author

This change requires a re-definition of every configuration in every target of every package in your mono repo, which you then need to keep in sync as you add new packages

Hey @SmoshySmosh. I appreciate you looking over the PR, it does also include a piece that allows Nx to infer configurations from the availability of the relevant .env files, so I think it should cover the use cases you mention.

@AgentEnder AgentEnder force-pushed the feat/set-environment-variables-from-configuration-name branch from 7781d49 to e577641 Compare June 20, 2023 16:11
// Inferred configuration from presence of .env files
(projectHasTarget(project, target) &&
[
join(workspaceRoot, `.env.${configuration}`),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think having a file there should define a configuration. Configurations should still be defined in the project configuration. If we'd like an easier way to define configurations, that is a separate discussion.

Can we remove this part from the changes here and consider them separately?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think having a file there should define a configuration. Configurations should still be defined in the project configuration. If we'd like an easier way to define configurations, that is a separate discussion.

Removing the requirement to redefine a configuration under every target is what myself and other customers are trying to get. With the current state of environment files in NX, a great deal of technical debt and configuration overhead is generated making it an unusable solution for most projects ultimately making NX very limiting.

Comment on lines 201 to 210
nx: {
targets: {
[target]: {
configurations: {
production: {},
},
},
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Were you going to do something with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was, yes. It looks like the e2e I had added somehow got lost 🤔

@AgentEnder AgentEnder force-pushed the feat/set-environment-variables-from-configuration-name branch from e0a6496 to 1dd261d Compare June 21, 2023 22:36
@FrozenPandaz FrozenPandaz merged commit 199d621 into nrwl:master Jun 28, 2023
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 4, 2023
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.

Add .[configuration-name].env to environment variables
3 participants