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

refactor: cleanup createSiteAddon function #1336

Merged
merged 2 commits into from
Oct 6, 2020

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Oct 6, 2020

Related to #310 and #1331

Wanted to fix #1331, but couldn't do it since the addons code doesn't allow getting the env variables easily at the moment:

I going to do some refactoring first so we can have

async function addEnvVariables(api, site) {
return addons envs instead of mutating process.env

I removed a bunch of outdated comments from utils/addons.js and did some code cleanup.
Note that part of that function is duplicated from:

async function createSiteAddon({ addonName, settings, accessToken, siteData, error }, logger) {

I follow up in another PR.

Also changed some code in functions/create.js to use async/await instead of promise nesting and added a missing await

The code itself is used when running netlify functions:create and will add missing addons depending on the function template.
For example if you install a fauna template it adds the fauna addon

@erezrokah erezrokah added the type: chore work needed to keep the product and development running smoothly label Oct 6, 2020
const addonCreated = await createSiteAddon(api.accessToken, addonName, siteId, siteData, this.log)
if (addonCreated) {
if (addonDidInstall) {
const { addEnvVariables } = require('../../utils/dev')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a good reason to dynamically load require() here (I think not)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually not sure why this is there. Looks it's only relevant when calling addonDidInstall a bit after and addEnvVariables is also called after installAddons.

// const manifest = await getAddonManifest(addonName, accessToken);

const configValues = rawFlags
// if (manifest.config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹


/** main section - shamelessly adapted from CLI. we can extract and dedupe later. */
/** but we can DRY things up later. */
Copy link
Contributor

Choose a reason for hiding this comment

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

:)

@erezrokah erezrokah merged commit 197a742 into master Oct 6, 2020
@erezrokah erezrokah deleted the refactor/createSiteAddon branch October 6, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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