Skip to content

fix: wrong env values could be used for non-dev context in netlify deploy --build --context #5538

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

Merged
kodiakhq merged 4 commits into from
Mar 9, 2023
Merged

fix: wrong env values could be used for non-dev context in netlify deploy --build --context #5538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c8afc7
fix: fix the wrong env context for `netlify deploy --build --context`
Mar 8, 2023
b558a0d
fix: only send env vars of the functions scope to functions
Mar 8, 2023
1315b1a
Merge branch 'main' into fix/fix-wrong-env-context-for-deploy-build
jasonbarry Mar 8, 2023
6f28075
Merge branch 'main' into fix/fix-wrong-env-context-for-deploy-build
jasonbarry Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/commands/deploy/deploy.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
} from '../../utils/command-helpers.mjs'
import { DEFAULT_DEPLOY_TIMEOUT } from '../../utils/deploy/constants.mjs'
import { deploySite } from '../../utils/deploy/deploy-site.mjs'
import { getEnvelopeEnv } from '../../utils/env/index.mjs'
import { getFunctionsManifestPath, getInternalFunctionsDir } from '../../utils/functions/index.mjs'
import openBrowser from '../../utils/open-browser.mjs'
import { link } from '../link/index.mjs'
Expand Down Expand Up @@ -567,6 +568,16 @@ const deploy = async (options, command) => {
return triggerDeploy({ api, options, siteData, siteId })
}

const isUsingEnvelope = siteData && siteData.use_envelope
// if a context is passed besides dev, we need to pull env vars from that specific context
if (isUsingEnvelope && options.context && options.context !== 'dev') {
Copy link
Member

@eduardoboucas eduardoboucas Mar 9, 2023

Choose a reason for hiding this comment

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

I think this additional check is unnecessary?

Suggested change
if (isUsingEnvelope && options.context && options.context !== 'dev') {
if (isUsingEnvelope && options.context !== 'dev') {

Copy link
Contributor Author

@jasonbarry jasonbarry Mar 9, 2023

Choose a reason for hiding this comment

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

options.context could be undefined if the user doesn't pass the --context flag, but the Envelope call defaults to use the dev context in that case. So if context isn't passed, or if context is dev, then we can skip the call. We could remove this bit if we gave the option a default value besides undefined, but I'm unsure if that would be a breaking change!

command.netlify.cachedConfig.env = await getEnvelopeEnv({
api,
context: options.context,
env: command.netlify.cachedConfig.env,
siteInfo: siteData,
})
}
const { configMutations = [], newConfig } = await handleBuild({
cachedConfig: command.netlify.cachedConfig,
options,
Expand Down Expand Up @@ -595,7 +606,18 @@ const deploy = async (options, command) => {
deployFolder,
functionsFolder,
})
const siteEnv = get(siteData, 'build_settings.env')

const siteEnv = isUsingEnvelope
? await getEnvelopeEnv({
api,
context: options.context,
env: command.netlify.cachedConfig.env,
raw: true,
scope: 'functions',
siteInfo: siteData,
})
: get(siteData, 'build_settings.env')

const functionsConfig = normalizeFunctionsConfig({
functionsConfig: config.functions,
projectRoot: site.root,
Expand Down
15 changes: 14 additions & 1 deletion src/utils/env/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,12 @@ export const formatEnvelopeData = ({ context = 'dev', envelopeItems = [], scope
* @param {string} context - The deploy context or branch of the environment variable
* @param {object} env - The dictionary of environment variables
* @param {string} key - If present, fetch a single key (case-sensitive)
* @param {boolean} raw - Return a dictionary of raw key/value pairs for only the account and site sources
* @param {enum<any,builds,functions,runtime,post_processing>} scope - The scope of the environment variables
* @param {object} siteInfo - The site object
* @returns {object} An object of environment variables keys and their metadata
*/
export const getEnvelopeEnv = async ({ api, context = 'dev', env, key = '', scope = 'any', siteInfo }) => {
export const getEnvelopeEnv = async ({ api, context = 'dev', env, key = '', raw = false, scope = 'any', siteInfo }) => {
const { account_slug: accountId, id: siteId } = siteInfo

const [accountEnvelopeItems, siteEnvelopeItems] = await Promise.all([
Expand All @@ -145,6 +146,18 @@ export const getEnvelopeEnv = async ({ api, context = 'dev', env, key = '', scop

const accountEnv = formatEnvelopeData({ context, envelopeItems: accountEnvelopeItems, scope, source: 'account' })
const siteEnv = formatEnvelopeData({ context, envelopeItems: siteEnvelopeItems, scope, source: 'ui' })

if (raw) {
const entries = Object.entries({ ...accountEnv, ...siteEnv })
Copy link
Contributor Author

@jasonbarry jasonbarry Mar 8, 2023

Choose a reason for hiding this comment

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

Is it correct that only account and site env vars, and not general / addons / configFile env vars, need to be included as siteEnv for the functionsConfig? That's how it seems to be before -- only get(siteData, 'build_settings.env') was getting passed to functionsConfig.

Copy link
Member

@eduardoboucas eduardoboucas Mar 9, 2023

Choose a reason for hiding this comment

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

Yes, I think that's right.

Copy link
Contributor Author

@jasonbarry jasonbarry Mar 9, 2023

Choose a reason for hiding this comment

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

OK cool! then I think this would fix a bug where account-level env vars weren't getting applied to functions if a user deployed with this method.

return entries.reduce(
(obj, [envVarKey, metadata]) => ({
...obj,
[envVarKey]: metadata.value,
}),
{},
)
}

const generalEnv = filterEnvBySource(env, 'general')
const internalEnv = filterEnvBySource(env, 'internal')
const addonsEnv = filterEnvBySource(env, 'addons')
Expand Down