feat: initial support for Deploy Configuration API #5509
Conversation
|
This pull request adds or modifies JavaScript ( |
| coreStepName: 'Applying Deploy Configuration', | ||
| coreStepDescription: () => '', | ||
| condition: () => true, | ||
| priority: 1, |
There was a problem hiding this comment.
This is a new field that will allow us to specify the order of core steps within the same event. We need it so that we always apply the configuration changes before the build command, and it will also unblock COM-328.
| coreStepDescription: () => '', | ||
| condition: () => true, | ||
| priority: 1, | ||
| quiet: true, |
There was a problem hiding this comment.
This is a new field that lets us omit log output from this specific core step, since in this case it would only add noise to people's build logs.
| return {} | ||
| } | ||
|
|
||
| const newConfig = merge(netlifyConfig, config) |
There was a problem hiding this comment.
What rules does this use for things like redirect arrays? Is the order defined?
There was a problem hiding this comment.
Right now it's merging them so that the entries from the deploy configuration come last, which is what we want in redirects, headers, and edge functions. We'll need special handling once/if we want to implement the headers! notation we discussed for defining precedence over user-defined values.
| return {} | ||
| } | ||
|
|
||
| const newConfig = merge(netlifyConfig, config) |
There was a problem hiding this comment.
This is probably something you're already aware but if I recall correctly we have an allow list of fields that we allow to mutate:
build/packages/config/src/mutations/apply.js
Lines 14 to 80 in ba81c85
Depending on the API we want to expose in the deploy config API we might need to revisit that list (or create some sort of exception for this particular step)
There was a problem hiding this comment.
Also, sort of related, if I'm understanding correctly here we're deep merging the config objects. After which ntl/config will take care of running through it's merge logic - https://github.com/netlify/build/blob/main/packages/config/src/merge.js - which takes care of removing things such as duplicate redirect entries and what not right? 🤔
There was a problem hiding this comment.
This is probably something you're already aware but if I recall correctly we have an allow list of fields that we allow to mutate
Interestingly, I'm seeing that I'm able to mutate fields in the deploy configuration API that plugins shouldn't be able to mutate, which is a bit confusing to me since I'm using the exact same mechanism for mutating the config.
I'm working on that. ⏳
There was a problem hiding this comment.
Also, sort of related, if I'm understanding correctly here we're deep merging the config objects. After which ntl/config will take care of running through it's merge logic - https://github.com/netlify/build/blob/main/packages/config/src/merge.js - which takes care of removing things such as duplicate redirect entries and what not right? 🤔
I think you're right, but I'm adding some tests to assert that.
| import type { NetlifyConfig } from '../../index.js' | ||
| import { CoreStep, CoreStepFunction } from '../types.js' | ||
|
|
||
| const coreStep: CoreStepFunction = async function ({ buildDir, netlifyConfig }) { |
There was a problem hiding this comment.
Not a blocker but thinking it could be useful to leverage our traces here and add some data on the fields that changed (and potentially the values if these don't hold sensitive info). This could help us with future headaches around debugging config changes or spot potential inconsistencies. We have other core steps already creating some spans we can use as boilerplate:
build/packages/build/src/plugins_core/secrets_scanning/index.ts
Lines 69 to 91 in ba81c85
Alternatively we could just add some events on the current span (we already create a span per execution step):
There was a problem hiding this comment.
I haven't added traces yet, but I did add a system logger. Will definitely revisit, though.
|
|
||
| config = JSON.parse(data) as Partial<NetlifyConfig> | ||
| } catch { | ||
| return {} |
There was a problem hiding this comment.
Sort of related with the above ☝️ but would be cool to add this error to the span (probably makes sense to create a span for this step in this case though)
There was a problem hiding this comment.
I've added some logic to ignore the case where we don't find a file and throw an error otherwise.
| @@ -37,6 +37,10 @@ const NORMALIZE_REGEXPS = [ | |||
| /(^|[ "'(=])((?:\.{0,2}|([A-Z]:)|file:\/\/)(\/[^ "')\n]+))/gm, | |||
|
|
|||
| (_, prefix, pathMatch, winDrive, pathTrail) => { | |||
| if (pathMatch.includes('/$netlify-snapshot-preserve/')) { | |||
There was a problem hiding this comment.
Without this, all paths would get normalised to /external/path and the snapshot would be useless in telling us which redirects take precedence. This acts as an escape hatch for the path normalisation.
Yes, this is weird. But so is the whole normalisation logic we have in snapshot tests, unfortunately.
There was a problem hiding this comment.
@eduardoboucas could you please expand on why you've chosen a build plugin for the deploy configuration API? instead of being part of @netlify/config and completely decoupled from the plugin executions?
To be honest for me this feels a bit brittle to do all those configuration merges inside netlify build as netlify config would be the better suited place IMHO.
Would love to know the background why you have chosen it to be a build plugin!
The tricky part here is that this configuration needs to be loaded at a very specific point of the build process, which is after the build command runs (since the build command is the one responsible for writing the file). Netlify Config is loaded at the beginning of the build, and I don't know of a good way to augment it at a specific point of the build without involving the build runner. Hence the core step (not a build plugin, since it lives in our space and can't be controlled by user code). If you have any ideas, I'm all ears. |
There was a problem hiding this comment.
Understanding the time constraints around this I'm 👍 on my side. A couple of things that I would like to us to revisit:
- I would like us to have more visibility into this step, ideally via tracing (which properties change, which were skipped, anything errored out, etc.)
- +1 to what @lukasholzer said and to what you suggested @eduardoboucas around moving this logic to ntl/config and maybe having a separate entrypoint we call from here (thereby scoping the whole logic to ntl/config)
| } | ||
|
|
||
| // Filtering out any properties that can't be mutated using this API. | ||
| config = filterConfig(config, [], ALLOWED_PROPERTIES, systemLog) |
There was a problem hiding this comment.
Is the reason for us to do this here too just fail fast vs letting the plugin run and ntl/config trying to run the mutations?
There was a problem hiding this comment.
Also it would be useful to maybe add some system logging or span event saying we've skipped X properties because we're filtering or something like that.
There was a problem hiding this comment.
Is the reason for us to do this here too just fail fast vs letting the plugin run and ntl/config trying to run the mutations?
I think the Deploy Configuration API should have its own set of properties that can be mutated — for example, build.command can be mutated by a plugin, but it doesn't make sense for it to be mutated using the Deploy Configuration API since that is expected to be generated during the build command.
There was a problem hiding this comment.
Also it would be useful to maybe add some system logging or span event saying we've skipped X properties because we're filtering or something like that.
I've added some system logging for every property we use and discard.
|
|
||
| systemLog(`Failed to read Deploy Configuration API: ${err.message}`) | ||
|
|
||
| throw new Error('An error occured while processing the platform configurarion defined by your framework.') |
There was a problem hiding this comment.
Non-blocker. Should we consider exposing err.message here as way for folks to potentially self serve?
There was a problem hiding this comment.
I'm not sure they can self-serve a solution if the error occurs on a file that is written by their framework, though. 😕
|
|
||
| // Merging the different configuration sources. The order here is important. | ||
| // Leftmost elements of the array take precedence. | ||
| const newConfig = merge.all([configOverrides, netlifyConfig, config]) |
There was a problem hiding this comment.
can we use the mergeConfigs function from @netlify/config for that instead of reimplementing this piece here?
| // no-op | ||
| }, | ||
| }) { | ||
| const configPath = resolve(buildDir, '.netlify', 'deploy', 'v1', 'config.json') |
There was a problem hiding this comment.
NIT: easier to search and find for paths if we preserve paths
| const configPath = resolve(buildDir, '.netlify', 'deploy', 'v1', 'config.json') | |
| const configPath = resolve(buildDir, '.netlify/deploy/v1/config.json') |
| @@ -42,7 +41,7 @@ export const runCoreSteps = async (buildSteps: string[], flags: Partial<BuildFla | |||
| } | |||
| } | |||
|
|
|||
| const getBuildSteps = function (buildSteps: CoreStep[]) { | |||
| const getBuildSteps = function (buildSteps: string[]) { | |||
There was a problem hiding this comment.
This was incorrectly typed. buildSteps is an array of strings — example: https://github.com/netlify/cli/blob/a0f7247640a3ece1fc55702c4679796ccfcd642f/src/commands/deploy/deploy.ts#L373
Summary
Adds an MVP for the Deploy Configuration API, with the view to unblock ADN-117.