Improve environment variables in local builds

[ehmicky]

Fixes #2036.

This PR improves how environment variables are handled in local builds by moving most of the logic from @netlify/build to @netlify/config

The main benefits are:

• The CLI commands would now retrieve not only site/account/addons environment variables, but to additional ones mimicking the production environment even better: URL, REPOSITORY_URL, DEPLOY_ID, CONTEXT, LANG, LANGUAGE, LC_ALL, GATSBY_TELEMETRY_DISABLED, NEXT_TELEMETRY_DISABLED, BRANCH, HEAD, PULL_REQUEST, COMMIT_REF, CACHED_COMMIT_REF.
• Move all the logic related to build environment variables from 3 different places (@netlify/build, @netlify/config, netlify-cli) to to a single place (@netlify/config)

The behavior of @netlify/build should not change, neither locally nor in production.

The main change is that, in local builds (not production), @netlify/config now returns a siteEnv plain object. siteEnv emulates the set of environment variables in production. This allows local builds and CLI commands (netlify build, netlify dev, netlify dev:exec and netlify functions:create) to mimic production behavior. This would replace the current logic in the CLI (getSiteInformation()). Note: since a single siteEnv object is returned, we don't have enough granularity to know where an environment variable came from. The logs printing the injected environment variables in the CLI would need to be simplified (or even removed? Do we need those?). Please let me know if you think this is a problem.

@netlify/config makes 3 API calls to retrieve site, account and addons environment variables. If the API call fails (or return a response with an invalid shape), it ignores those silently. This is meant to work like the --offline CLI flag, but without the user having to specify when they are offline or not to turn on/off error messages, which I believe might be a simpler experience. This is also more resilient to users experiencing temporary network problems.

The addonsUrls used by netlify dev are not handled by @netlify/config. Netlify CLI would still need to fetch and compute those itself. In a follow-up PR, we could optimize this by computing those in @netlify/config if we're concerned with fetching the addons twice when using netlify dev.

The priority order (from highest to lowest) is: site > account > addons. At the moment, the CLI uses account > addons > site, but this seemed to be wrong to me. Please let me know what you think.

The config.build.environment property returned by @netlify/config used to contain both netlify.toml environment variables and UI environment variables. Now it only contains the former one, since siteEnv can now be used to retrieve the later one. The netlify env:list and netlify env:get CLI commands is currently using config.build.environment. By switching to using siteEnv instead, it will list a set of environment variables richer and closer to the actual set used in production builds (and now local builds too). However, it will now mix user-defined and mock environment variables. This might be an issue for netlify env:list? If so, we might need to iterate on this in a follow-up PR. Apart from those CLI commands and from builds, I do not think the config.build.environment property returned by @netlify/config is used.

In order to mimic the DEPLOY_ID environment variable, @netlify/config now accepts a --deploy-id option, just like @netlify/build does. This might not be useful at the moment though since Netlify CLI does not have a deploy ID to pass (please correct me if I'm wrong).

This feature is fully tested, but we should release this as a major version to be on the safe side, and do additional testing both in the CLI and in the buildbot.
Many test snapshots are updated due to the additional siteEnv return value of @netlify/config.

[ehmicky]

Should be fixed in a separate PR.

[erezrokah]

The main benefits are:

• The CLI commands would now retrieve not only site/account/addons environment variables, but to additional ones mimicking the production environment even better: URL, REPOSITORY_URL, DEPLOY_ID, CONTEXT, LANG, LANGUAGE, LC_ALL, GATSBY_TELEMETRY_DISABLED, NEXT_TELEMETRY_DISABLED, BRANCH, HEAD, PULL_REQUEST, COMMIT_REF, CACHED_COMMIT_REF.
• Move all the logic related to build environment variables from 3 different places (@netlify/build, @netlify/config, netlify-cli) to to a single place (@netlify/config)

This is amazing, thank you for this. Great to align our logic and remove duplication.

The logs printing the injected environment variables in the CLI would need to be simplified (or even removed? Do we need those?). Please let me know if you think this is a problem.

I find these logs quite useful for debugging. Maybe @fool or someone else from support can comment on this.
Can we return the source of each env in addition to the value?
An alternative is to log just the name of the injected variable and not the source.

it ignores those silently

Missing env variables are most likely to break functionality, for example missing auth tokens, secrets, etc. This is the case for the nimbella plugin and fauna based functions.
It might be better to fail fast here with a proper message instead of having a build failing or function not working at a later stage.
Could we return an array of errors (with relevant codes) of what had failed?

This is meant to work like the --offline CLI flag

Offline is an explicit choice. I assume it is mostly used when a user doesn't have an internet connection and still wants to run netlify dev.

This is also more resilient to users experiencing temporary network problems

We can solve this by retrying requests (we've discussed this before in regards with the js-client).

The priority order (from highest to lowest) is: site > account > addons. At the moment, the CLI uses account > addons > site, but this seemed to be wrong to me. Please let me know what you think.

I think it should be site > addons > account as addons are per site. I believe this is also reflected in our API code.
The CLI has site > addons > account. The code in https://github.com/netlify/cli/blob/d646b00b0333679ca5e272f173e7720eb82d5217/src/utils/dev.js#L106 is not very readable. assignLoudly will return the original value if exists so going top to bottom will provide the current priority.
Mutating the process.env doesn't help making that code readable.

However, it will now mix user-defined and mock environment variables

We can solve this by adding a source property to each env.

(please correct me if I'm wrong).

I believe you're correct here.

[ehmicky]

Thanks a lot for pondering this PR @erezrokah, this is a tricky one.

For the source of each env, I'm going to add this by returning { siteEnv: { uiSettings: { ... }, addons: { ... }, account: { ... }, configFile: { ... }, mock: { ... }, all: { ... } } }. 👍
all will be a merge of all the other sources, in the correct priority order.

I will fix the priority order to site > addons > accounts 👍

For the offline flag, I'd still think not requiring an explicit opt-in and guessing it instead (e.g. by making a HTTP request or by using os.networkInterfaces() like the is-online library does) would be a better experience. However, I can see the benefits of the opposite too (explicit opt-in and network retries), so let's just keep the offline flag 👍
In practice, this would mean @netlify/config would need an additional offline option (defaulting to false), which would decide whether API errors should be silent or not. The --offline flag might need to be added to a few CLI commands as well, e.g. netlify functions:create and netlify build.

[erezrokah]

will be a merge of all the other sources, in the correct priority order.

This is a great solution.

would be a better experience

I agree, this should at least come with a log warning if --offline is not provided. Or a flag to --disable-offline-mode.
Regardless this would be a breaking change (some users might expect the CLI to fail if it can't retrieve env variables).

API errors should be silent or not.

--offline currently mean that the CLI doesn't even attempt the API calls. Silently failing is a bit different from that.
Also the CLI won't attempt the API calls if the directory is not linked to a site.
I believe netlify build fails in that case.

I think we should keep the current behaviour until we reach some product decisions here considering:

1. The CLI should be able to work as much as possible without a network connection (especially netlify dev)
2. The CLI should be able to work as much as possible even if not linked to a site (for example I would expect netlify build to work).
3. The above 2 considerations should be aligned as much as possible between different commands, e.g. netlify dev, netlify build and netlify functions:create.

Sadly, we're not so close to reaching 3 specifically for --offline and for other flags in general (see --silent flag and possible --json).

Removing code duplicate is a big step forward to aligning commands behaviour. So far we've being doing it one step at a time depending on the area of the code we touch, but maybe there is a place for a more dedicated effort.

[ehmicky]

Thanks, this makes total sense. This means:

• I will add the --offline option to @netlify/config, defaulting to false. When false, no API calls should be made.
• When an API call is made (which means offline is false) and fails, we should error

When it comes to netlify build failing when either the site ID or the API token is not available, this is because, without this information, the following would be missing from the local build:

• UI build settings: build command, Functions directory, publish directory, base directory. The netlify.toml is still used, but there is a very high number of users using the UI build settings
• UI environment variables. With this PR, addons and account environment variables as well
• constants.SITE_ID and the (yet undocumented) constants.NETLIFY_API_TOKEN

Missing those (especially the UI build settings) will often result in local build errors. Not only would this confuse errors, but this would also create support issues/tickets from those users.

We could possibly not fail providing --offline is passed and print a big warning though.

[erezrokah]

• When false, no API calls should be made.

I think you mean true here.

The defaults you presented makes sense for Netlify build and for the CLI we would need to provide slightly other defaults.

I see 3 scenarios here:

1. The user has a Netlify site but hasn't linked locally it yet. In that case it would make sense to fail
2. The user doesn't have a Netlify site at all. In that case it would make sense to succeed in case netlify.toml has the minimal required information. For example - for netlify build that should be a build command.
   For netlify deploy that should be a publish directory For functions invoke that should be a functions directory, etc
3. The user has a Netlify site connected to a repo and linked locally. In that case we have everything we need to resolve the configuration unless running offline (which is explicit).

I think that level of granularity should be controlled by the CLI and configurable via @netlify/config and @netlify/build, since it is hard to distinguish between 1. and 2. without some kind of user interaction.

TLDR: we're good to move forward with this 🚀

[ehmicky]

There is no great benefit in splitting this file.

[ehmicky]

I fixed all the points above.

I renamed siteEnv to just env. I also added more documentation in --help and README.md.
I also added a testOpts.env: false, which will not try to retrieve UI environment variables (getSite, listAccountsForUser, listAddons), but will still allow making other API calls. This is useful in tests that test those API calls, such as the API call to cancel sites, or the one to create plugin statuses.

Since this is a breaking change for Netlify CLI, this would be released as a major release.
This is not a breaking change for the buildbot, but we'll need to do some thorough manual testing.

According to the above discussion, those are some follow-up we would need to do in the Netlify CLI to upgrade:

• netlify build:
  • Do not pass the env option to @netlify/build anymore
  • This means getBuildEnv() can be removed.
• netlify env:list and netlify env:get:
  • Use the env return value from @netlify/config instead of config.build.environment.
    We might want to select only specific environment variables categories, and merge them.
• netlify dev, netlify dev:exec, netlify functions:create:
  • Use the env return value from @netlify/config. Note: this will also add support to build.environment.
  • The dev.environment property in netlify.toml should be merged to them: { ...env.all, ...dev.environment }.
  • Do not use getSiteInformation() anymore, except for the addonsUrl and dotenv logic
  • Adjust logs in addEnvVariables() to dissociate between netlify.toml and UI site environment variables
  • Pass --offline CLI flag to @netlify/config. netlify dev:exec and netlify functions:create might not have this flag yet, but netlify dev does.

Please let me know if I missed anything, and we can create issues for those.

[erezrokah]

Please let me know if I missed anything

This looks good. I'm going to test this PR locally with the CLI starting next week

[ehmicky]

Great!
Should we try to create a corresponding CLI PR and try to release both at the same time?

[erezrokah]

Should we try to create a corresponding CLI PR and try to release both at the same time?

Sure. I'm going to link @netlify/config with this branch locally, then use that version with the CLI and make the necessary adjustments.

[eduardoboucas]

The term account is widely used in netlify/build, but CLI uses team. Should we try to standardise this? If so, which one should we adopt?

[erezrokah]

Yes we should, great point :) We refer to those publicly as team:

So it make sense to me to use that term. @ehmicky WDYT?

[ehmicky]

Good point! I opened an issue at https://github.com/netlify/build/issues/2164

[eduardoboucas]

I have a question, looking at this from the CLI side. We're now getting from @netlify/config an object containing a set of environment variables for each source, including an object with all of them merged together according to the precedence rules.

However, in CLI there are two additional sources that we need to add to the mix (both of which have a higher precedence than the vars coming from config): .env/.env.development files and process.env. If CLI finds a variable in one of those two places that overrides a variable coming from config, and after the enhanced logging messages we've added in netlify/cli#1698, it should say something like:

Ignored [source X] env var: VAR_NAME (defined in .env file)

But CLI doesn't really know what source X is, because the precedence chain has been computed by @netlify/config and flattened in the all object. To know exactly where the variable was coming from (e.g. site or team), we'd need to look at all the sources contained in env and figure out which one contributed with the "winning value". But this feels like computing the precedence chain all over again in CLI, which is one of the things we're trying to avoid with this PR.

One option would be to change the shape of the env object returned from @netlify/config to something like this:

{
  "TEST_VAR_1": {
    "value": "foo",
    "source": "team"
  },
  "TEST_VAR_2": {
    "value": "bar",
    "source": "ui"
  },
  "GATSBY_TELEMETRY_DISABLED": {
    "value": "1",
    "source": "general"
  }
}

What do we think? Do you see any downsides of this approach?

[erezrokah]

What do we think? Do you see any downsides of this approach?

This looks good to me. does that mean we can simplify teamEnv, general etc. (everything except all) to return a list of names?

[eduardoboucas]

does that mean we can simplify teamEnv, general etc. (everything except all) to return a list of names?

Yes, we can turn those properties into arrays.

Alternatively, we can get rid of them altogether and have env be something like:

{
  "TEST_VAR_1": {
    "value": "foo",
    "sources": ["team", "addons"]
  },
  "TEST_VAR_2": {
    "value": "bar",
    "sources": ["ui"]
  },
  "GATSBY_TELEMETRY_DISABLED": {
    "value": "1",
    "sources": ["general", "team"]
  }
}

Where sources contains all the sources where this variable was found, in order of precedence. As a consumer (e.g. in CLI), you look at position 0 to know where the variable came from, whereas all subsequent positions (if any) tell you of any sources that were overridden.

[erezrokah]

Where sources contains all the sources where this variable was found, in order of precedence. As a consumer (e.g. in CLI), you look at position 0 to know where the variable came from, whereas all subsequent positions (if any) tell you of any sources that were overridden.

That's even better :)

[eduardoboucas]

I made the changes described above. For convenience, here's a direct link to the diff of my commits: https://github.com/netlify/build/pull/2040/files/c3636e0a0f59b2f37ce50b98d91cb41bb624f467..a0a499671faed7b01d12a871aae1132614f24462.

@ehmicky I tried adding you as a reviewer, but GitHub won't let me, since you created the PR. ☹️

[ehmicky]

[sand] Would using map-obj help remove the linting warnings, and maybe simplify the logic? For example:

const envs = [generalEnv, accountEnv, addonsEnv, uiEnv, configFileEnv]
const envNames = ['general', 'account', 'addons', 'ui', 'configFile']

return mapObj(Object.assign({}, ...envs), (name, value) => [
  name,
  sources: envs.filter((env) => env[name] !== undefined).map((_, index) => envNames[index]),
])

(alternative: merge envs and envNames to a single object and use Object.entries())

[eduardoboucas]

I wasn't too happy about disabling the linting rule, so I'm glad you brought this up.

I tried a different alternatives and wasn't convinced by any of them, but usingmap-obj is an interesting approach. Personally, I still find the mutation route easier to read, but not enough to justify disabling linting rules.

I'll change it. Thanks!

[eduardoboucas]

@ehmicky You can see my changes in a1f3516. I opted for a different approach, because the code above wasn't giving us exactly what we need, and as I added to it it started to feel a little bit too complex.

Let me know how you feel about the updated version. Happy to rejig it more if needed.

[ehmicky]

Looks good!

[eduardoboucas]

Note: we use a ponyfill for Object.fromEntries() in

Oh, that's ideal. I'll use it. Thanks!

[ehmicky]

@eduardoboucas The changes look great, thanks! 🎉

[eduardoboucas]

Oh, that's ideal. I'll use it. Thanks!

Done in cdd3953.

[ehmicky]

@eduardoboucas We probably should update this section of the README.md.

[ehmicky]

We can remove the 2 above TODO comments.