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: fix local builds with plugins and build settings from the ui #4929

Merged
merged 3 commits into from
Aug 17, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
15 changes: 15 additions & 0 deletions src/lib/build.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// @ts-check
const process = require('process')

const netlifyBuildPromise = import('@netlify/build')

/**
Expand All @@ -20,6 +22,7 @@ const netlifyBuildPromise = import('@netlify/build')
*/
const getBuildOptions = ({ cachedConfig, options: { context, cwd, debug, dry, json, offline, silent }, token }) => ({
cachedConfig,
siteId: cachedConfig.siteInfo.id,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This, and this only is the fix, the rest is for the tests.

token,
dry,
debug,
Expand All @@ -42,6 +45,18 @@ const getBuildOptions = ({ cachedConfig, options: { context, cwd, debug, dry, js
*/
const runBuild = async (options) => {
const { default: build } = await netlifyBuildPromise

// If netlify NETLIFY_API_URL is set we need to pass this information to @netlify/build
// TODO don't use testOpts, but add real properties to do this.
if (process.env.NETLIFY_API_URL) {
const apiUrl = new URL(process.env.NETLIFY_API_URL)
const testOpts = {
scheme: apiUrl.protocol.slice(0, -1),
host: apiUrl.host,
}
options = { ...options, testOpts }
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not nice,.
The host could be supplied by setting apiHost but the scheme cannot be changed besides the testOpts.

I'm also not sure if NETLIFY_API_URL is only for tests or if that is meant to be used by users?

Should I add apiScheme to @netlify/build? Or is it okay this way?


const { configMutations, netlifyConfig: newConfig, severityCode: exitCode } = await build(options)
return { exitCode, newConfig, configMutations }
}
Expand Down
79 changes: 72 additions & 7 deletions tests/integration/110.command.build.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ const { withSiteBuilder } = require('./utils/site-builder')

const defaultEnvs = {
NETLIFY_AUTH_TOKEN: 'fake-token',
NETLIFY_SITE_ID: 'site_id',
Copy link
Contributor Author

@danez danez Aug 12, 2022

Choose a reason for hiding this comment

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

I opted to not set it for every test here, because it does not really reflect real world scenarios.
If NETLIFY_SITE_ID is set this bug I'm fixing here never surfaces, because @netlify/build and @netlify/config both also read it.
But most users never set it probably.

I instead opted to add a new method to the site-builder withStateFile, which mimics netlify link

FORCE_COLOR: '1',
}

Expand All @@ -19,7 +18,7 @@ const defaultEnvs = {
const runBuildCommand = async function (
t,
cwd,
{ apiUrl, exitCode: expectedExitCode = 0, output, flags = [], env = defaultEnvs } = {},
{ apiUrl, exitCode: expectedExitCode = 0, output: outputs, flags = [], env = defaultEnvs } = {},
) {
const { all, exitCode } = await execa(cliPath, ['build', ...flags], {
reject: false,
Expand All @@ -35,7 +34,12 @@ const runBuildCommand = async function (
console.error(all)
}

t.true(all.includes(output))
if (!Array.isArray(outputs)) {
outputs = [outputs]
}
outputs.forEach((output) => {
t.true(all.includes(output), `Output of build command does not include '${output}'`)
})
t.is(exitCode, expectedExitCode)
}

Expand All @@ -44,6 +48,12 @@ const siteInfo = {
id: 'site_id',
name: 'site-name',
}
const siteInfoWithCommand = {
...siteInfo,
build_settings: {
cmd: 'echo uiCommand',
},
}
const routes = [
{ path: 'sites/site_id', response: siteInfo },
{ path: 'sites/site_id/service-instances', response: [] },
Expand All @@ -52,10 +62,60 @@ const routes = [
response: [{ slug: siteInfo.account_slug }],
},
]
const routesWithCommand = [...routes]
routesWithCommand.splice(0, 1, { path: 'sites/site_id', response: siteInfoWithCommand })

test('should use build command from UI', async (t) => {
await withSiteBuilder('success-site', async (builder) => {
builder.withNetlifyToml({ config: {} }).withStateFile({ siteId: siteInfo.id })

await builder.buildAsync()
await withMockApi(routesWithCommand, async ({ apiUrl }) => {
await runBuildCommand(t, builder.directory, { apiUrl, output: 'uiCommand' })
})
})
})

test('should use build command from UI with build plugin', async (t) => {
await withSiteBuilder('success-site', async (builder) => {
builder
.withNetlifyToml({
config: {
plugins: [{ package: '/plugins/' }],
},
})
.withStateFile({ siteId: siteInfo.id })
.withBuildPlugin({
name: 'index',
plugin: {
onPreBuild: ({ netlifyConfig }) => {
console.log('test-pre-build')

netlifyConfig.build.environment ||= {}
netlifyConfig.build.environment.TEST_123 = '12345'
},
},
})

await builder.buildAsync()
await withMockApi(routesWithCommand, async ({ apiUrl }) => {
await runBuildCommand(t, builder.directory, {
apiUrl,
output: ['uiCommand', 'test-pre-build'],
env: {
NETLIFY_AUTH_TOKEN: 'fake-token',
FORCE_COLOR: '1',
},
})
})
})
})

test('should print output for a successful command', async (t) => {
await withSiteBuilder('success-site', async (builder) => {
builder.withNetlifyToml({ config: { build: { command: 'echo testCommand' } } })
builder
.withNetlifyToml({ config: { build: { command: 'echo testCommand' } } })
.withStateFile({ siteId: siteInfo.id })

await builder.buildAsync()

Expand All @@ -67,7 +127,7 @@ test('should print output for a successful command', async (t) => {

test('should print output for a failed command', async (t) => {
await withSiteBuilder('failure-site', async (builder) => {
builder.withNetlifyToml({ config: { build: { command: 'doesNotExist' } } })
builder.withNetlifyToml({ config: { build: { command: 'doesNotExist' } } }).withStateFile({ siteId: siteInfo.id })

await builder.buildAsync()

Expand All @@ -79,7 +139,9 @@ test('should print output for a failed command', async (t) => {

test('should run in dry mode when the --dry flag is set', async (t) => {
await withSiteBuilder('success-site', async (builder) => {
builder.withNetlifyToml({ config: { build: { command: 'echo testCommand' } } })
builder
.withNetlifyToml({ config: { build: { command: 'echo testCommand' } } })
.withStateFile({ siteId: siteInfo.id })

await builder.buildAsync()

Expand Down Expand Up @@ -140,7 +202,9 @@ test('should run the staging context command when the context env variable is se

test('should print debug information when the --debug flag is set', async (t) => {
await withSiteBuilder('success-site', async (builder) => {
builder.withNetlifyToml({ config: { build: { command: 'echo testCommand' } } })
builder
.withNetlifyToml({ config: { build: { command: 'echo testCommand' } } })
.withStateFile({ siteId: siteInfo.id })

await builder.buildAsync()

Expand All @@ -154,6 +218,7 @@ test('should use root directory netlify.toml when runs in subdirectory', async (
await withSiteBuilder('subdir-site', async (builder) => {
builder
.withNetlifyToml({ config: { build: { command: 'echo testCommand' } } })
.withStateFile({ siteId: siteInfo.id })
.withContentFile({ path: path.join('subdir', '.gitkeep'), content: '' })

await builder.buildAsync()
Expand Down
9 changes: 9 additions & 0 deletions tests/integration/utils/site-builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ const createSiteBuilder = ({ siteName }) => {
})
return builder
},
withStateFile: ({ siteId = '' }) => {
const dest = path.join(directory, '.netlify', 'state.json')
tasks.push(async () => {
const content = `{ "siteId" : "${siteId}" }`
await ensureDir(path.dirname(dest))
await writeFile(dest, content)
})
return builder
},
withPackageJson: ({ packageJson, pathPrefix = '' }) => {
const dest = path.join(directory, pathPrefix, 'package.json')
tasks.push(async () => {
Expand Down