feat: update method for merging ISC with TOML config #5712
Conversation
|
This pull request adds or modifies JavaScript ( |
|
Still some failing tests. Will sort tomorrow. |
| export const functionConfig = z.object({ | ||
| externalNodeModules: z.array(z.string()).optional().catch([]), | ||
| generator: z.string().optional().catch(undefined), | ||
| includedFiles: z.array(z.string()).optional().catch([]), |
There was a problem hiding this comment.
Not quite sure why, but using undefined as the fallback for an optional value makes more sense to me.
| includedFiles: z.array(z.string()).optional().catch([]), | |
| includedFiles: z.array(z.string()).optional().catch(undefined), |
| includedFiles: z.array(z.string()).optional().catch([]), | ||
| includedFilesBasePath: z.string().optional().catch(undefined), | ||
| ignoredNodeModules: z.array(z.string()).optional().catch([]), | ||
| name: z.string().optional().catch(undefined), |
There was a problem hiding this comment.
there's a lot of duplication in this. does mit make sense to extract z.string().optional().... into a few constants, just so that there's fewer characters on the screen? It reads very dense right now.
There was a problem hiding this comment.
I see what you mean, but at the same time I don't know what would be the criteria for extracting something into a constant. We could have optionalString = z.string()).optional(), but is that really saving a lot of characters? We could add .catch(undefined) to that, but what do we do about the strings where we don't want to have a catch? I'm not super convinced this would help a lot, tbh.
There was a problem hiding this comment.
Are there any strings where we don't have a catch, though?
Maybe it'd already help to add a few empty lines in between the different parameters. That'd make it less of a uniform wall of text.
| */ | ||
| export const getTrafficRulesConfig = (input: RateLimit): TrafficRules | undefined => { | ||
| const { windowSize, windowLimit, algorithm, aggregateBy, action, to } = input | ||
| const rateLimitAgg = Array.isArray(aggregateBy) ? aggregateBy : [rateLimitAggregator.Enum.domain] |
There was a problem hiding this comment.
I think this incorrectly turns rateLimitAggregator: "ip" into "domain". Should it be this instead?
| const rateLimitAgg = Array.isArray(aggregateBy) ? aggregateBy : [rateLimitAggregator.Enum.domain] | |
| const rateLimitAgg = Array.isArray(aggregateBy) ? aggregateBy : [aggregateBy] |
There was a problem hiding this comment.
I'm going by the current implementation:
There was a problem hiding this comment.
Alright, then let's keep the fixing of that to a different PR. @paulo I think you wrote this (and I reviewed ...), what are your thoughts on it?
There was a problem hiding this comment.
The thought process at the time was to not fail the build and just use the default since it's not an obligatory field, but I'm now regretting it. If I could do it all over again I think I'd just fail the build if aggregateBy is not an array (if defined, not an obligatory field), but leaving the current logic is also ok
| runtimeAPIVersion?: number | ||
| } | ||
|
|
||
| interface FindISCDeclarationsOptions { | ||
| functionName: string | ||
| } | ||
|
|
||
| const httpMethods = z.preprocess( | ||
| (input) => (typeof input === 'string' ? input.toUpperCase() : input), | ||
| z.enum(['GET', 'POST', 'PUT', 'PATCH', 'OPTIONS', 'DELETE', 'HEAD']), |
There was a problem hiding this comment.
Are you sure this list is exhaustive? What about uncommon methods like TRACE or PROPFIND? Might be worth doing this change in a separate PR and explore that first.
There was a problem hiding this comment.
These are the methods that our CDN supports, everything else is blocked:
var validMethods = map[string]struct{}{
"ACL": {},
"BASELINE-CONTROL": {},
"BIND": {},
"CHECKIN": {},
"CHECKOUT": {},
"COPY": {},
"DELETE": {},
"GET": {},
"HEAD": {},
"LABEL": {},
"LINK": {},
"LOCK": {},
"MERGE": {},
"MKACTIVITY": {},
"MKCALENDAR": {},
"MKCOL": {},
"MKREDIRECTREF": {},
"MKWORKSPACE": {},
"MOVE": {},
"OPTIONS": {},
"ORDERPATCH": {},
"PATCH": {},
"POST": {},
"PRI": {},
"PROPFIND": {},
"PROPPATCH": {},
"PUT": {},
"REBIND": {},
"REPORT": {},
"SEARCH": {},
"UNBIND": {},
"UNCHECKOUT": {},
"UNLINK": {},
"UNLOCK": {},
"UPDATE": {},
"UPDATEREDIRECTREF": {},
"VERSION-CONTROL": {},
"*": {},
}
| if (configExport.rateLimit !== undefined) { | ||
| result.trafficRules = getTrafficRulesConfig(configExport.rateLimit, functionName) | ||
| try { | ||
| const iscValues = isc.parse(configExport) |
There was a problem hiding this comment.
😍
zod really is an incredibly nice piece of code.
There was a problem hiding this comment.
Yeah, I think it's going to make our lives easier!
| @@ -2,7 +2,8 @@ | |||
| "extends": "../../tsconfig.base.json", | |||
| "compilerOptions": { | |||
| "outDir": "dist" /* Specify an output folder for all emitted files. */, | |||
| "esModuleInterop": true | |||
| "esModuleInterop": true, | |||
| "strict": true | |||
Summary
In preparation for #5668, this changes the way we merge configuration properties from the TOML with in-source configuration. There was a lot of manual work involved in validating properties, and a lot of duplication in reading properties from the regular function config object and in-source configuration.
This PR makes use of
zodto create a schema for the different sources of config, and parsing/validating them accordingly.As part of this, we now allow
includedFiles,externalNodeModules,ignoredNodeModulesandnodeBundlerto be defined in-source.Part of COM-798.