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.

Sign up for GitHub

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

Jump to bottom

fix(build): pass team,site, addons env variables to build command #1519

Merged
merged 3 commits into from
Nov 4, 2020
Merged

fix(build): pass team,site, addons env variables to build command #1519

merged 3 commits into from
Nov 4, 2020

Conversation

erezrokah
Copy link
Contributor

- Summary

Fixes #1331

- Test plan

Tested manually with the flow in https://github.com/nimbella/netlify-plugin-nimbella

- Description for the changelog

Inject relevant env variables when running the build command.

- A picture of a cute animal (not mandatory but encouraged)
image

@erezrokah erezrokah requested a review from a team as a code owner November 3, 2020 14:29
@github-actions github-actions bot added the type: bug code to address defects in shipped code label Nov 3, 2020
erezrokah
erezrokah commented Nov 3, 2020
View reviewed changes
src/lib/build.js Outdated
warn,
error,
})
const env = { ...teamEnv, ...addonsEnv, ...siteEnv, ...dotFilesEnv }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ehmicky since @netlify/config already evaluates some of these, should we move this logic to @netlify/config?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.
I have filed netlify/build#2036 with additional information. However, this can be done in a follow-up PR.

@erezrokah erezrokah requested a review from ehmicky November 3, 2020 14:30
@erezrokah
Copy link
Contributor Author

Checking why this is failing on node 8

@erezrokah
Copy link
Contributor Author

erezrokah commented Nov 3, 2020 •
edited
Loading

Looks like this is another issue with oclif. Some of the oclif packages we are using are referencing fs-extra@^9 which only supports node 10.

├─┬ @oclif/[email protected]
│ └── [email protected] 
├─┬ @oclif/[email protected]
│ └─┬ [email protected]
│   └── [email protected] 
├─┬ @oclif/[email protected]
│ └── [email protected]  deduped
└─┬ [email protected]
  └── [email protected]  deduped

For example https://github.com/oclif/errors/blob/2233a100fbb8822a15a74e97c5f1ebd32c876a99/package.json#L35. A breaking change was done in https://github.com/oclif/errors/pull/27/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R9, but was published as a minor version oclif/errors@v1.2.2...v1.3.0

Opened oclif/errors#97

@erezrokah erezrokah mentioned this pull request Nov 3, 2020
Merged
@erezrokah
Copy link
Contributor Author

@ehmicky this is ready for review

ehmicky
ehmicky reviewed Nov 3, 2020
View reviewed changes
src/lib/build.js Outdated
const getBuildEnv = async ({ context }) => {
const { warn, error, netlify } = context
const { site, api } = netlify
const { teamEnv, addonsEnv, siteEnv, dotFilesEnv } = await getSiteInformation({
Copy link
Contributor

Choose a reason for hiding this comment

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

We should reach a conclusion in netlify/build#1514 before adding .env support in local builds. Would it be ok to split this specific feature from this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 1556c56

ehmicky
ehmicky suggested changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

This is looking good.
Just one comment about .env files support.

@erezrokah erezrokah changed the title fix(build): pass env variables to build command fix(build): pass team,site, addons env variables to build command Nov 4, 2020
@erezrokah erezrokah merged commit ba8a7a3 into master Nov 4, 2020
@erezrokah erezrokah deleted the fix/build_with_env branch November 4, 2020 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehmicky ehmicky ehmicky approved these changes

Assignees
No one assigned
Labels
type: bug code to address defects in shipped code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: addons env variables are not available when running netlify build
2 participants
@erezrokah @ehmicky