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

fix: when fetching env from envelope, pass context #5426

Merged
merged 6 commits into from
Apr 22, 2024
Merged

Conversation

Skn0tt
Copy link
Contributor

@Skn0tt Skn0tt commented Dec 1, 2023

Currently, we're always doing an extra Envelope fetch in the CLI for ntl build / ntl dev. We do this because @netlify/config didn't consider the context when fetching from Envelope. Time to change that, so we can get rid that workaround in the CLI!

@Skn0tt Skn0tt self-assigned this Dec 1, 2023
Copy link
Contributor

github-actions bot commented Dec 6, 2023

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@Skn0tt Skn0tt marked this pull request as ready for review December 6, 2023 10:45
@Skn0tt Skn0tt requested review from a team as code owners December 6, 2023 10:45
JGAntunes
JGAntunes previously approved these changes Dec 6, 2023
Copy link
Contributor

@JGAntunes JGAntunes left a comment

Choose a reason for hiding this comment

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

A minor suggestion and a question just to ensure we keep the currently compatibility. LGTM overall.

if (accountId === undefined) {
return {}
}
try {
const environmentVariables = await api.getEnvVars({ accountId, siteId })
const environmentVariables = await (api as any).getEnvVars({ accountId, siteId, context_name: context })
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding correctly based on the tests, in the absence of a context this will apply the default value which will be production meaning the current behaviour is maintained?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

coooorect!

@@ -91,7 +99,7 @@ const getGeneralEnv = async function ({
context,
}) {
const gitEnv = await getGitEnv(buildDir, branch)
const deployUrls = getDeployUrls({ siteInfo, branch, deployId })
const deployUrls = getDeployUrls({ siteInfo: siteInfo as any, branch, deployId })
Copy link
Contributor

Choose a reason for hiding this comment

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

I know these any's we're adding result from the untyped API but maybe we can wrap this is these nice TSFixme thingy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, done in 972ca05

@Skn0tt Skn0tt enabled auto-merge (squash) April 22, 2024 14:19
@Skn0tt Skn0tt merged commit 8301161 into main Apr 22, 2024
34 checks passed
@Skn0tt Skn0tt deleted the pass-context-envelope branch April 22, 2024 14:51
This was referenced May 23, 2024
This was referenced Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants