-
Notifications
You must be signed in to change notification settings - Fork 1
feat: allow config file to have config globs #129
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,12 +3,13 @@ import fs from 'node:fs/promises'; | |
| import path from 'node:path'; | ||
|
|
||
| import type { ActorVersionSourceFile } from 'apify-client'; | ||
| import { minimatch } from 'minimatch'; | ||
|
|
||
| import { SOURCE_FILE_FORMATS } from '@apify/consts'; | ||
|
|
||
| import { selectActors } from './actor-filtering.js'; | ||
| import { isPathWithinScope } from './path-utils.js'; | ||
| import type { ActorConfig, ActorConfigFile } from './types.js'; | ||
| import type { ActorConfig, ActorConfigFile, ActorConfigFileEntry, ActorGlobConfigEntry } from './types.js'; | ||
|
|
||
| // Returns true when `childPath` is not inside `parentPath`. | ||
| // Used to detect monorepo actors whose dockerContextDir escapes the actor directory. | ||
|
|
@@ -114,6 +115,62 @@ const findOverlappingContextPaths = (contextPaths: string[]): [string, string] | | |
| return undefined; | ||
| }; | ||
|
|
||
| const validateGlobConfigEntries = (configs: unknown[]): ActorGlobConfigEntry[] => { | ||
| for (const [index, configEntry] of configs.entries()) { | ||
| const { folder, actorFullName } = configEntry as ActorGlobConfigEntry; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be zod parse I guess but I don't know if you can get such a nice errors from it, don't have experience |
||
|
|
||
| // TODO: Allow for combined filtering? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would allow this, don't think it is that confusing or dangerous. You simply filter once per folder and then per name. There might be legit use-cases for this |
||
| if (folder !== undefined && actorFullName !== undefined) { | ||
| throw new Error( | ||
| `Invalid "configs" entry at index ${index} in "${CONFIG_FILE_NAME}". ` + | ||
| `Must not have both "folder" and "actorFullName" set.`, | ||
| ); | ||
| } | ||
|
|
||
| if (folder === undefined && actorFullName === undefined) { | ||
| throw new Error( | ||
| `Invalid "configs" entry at index ${index} in "${CONFIG_FILE_NAME}". ` + | ||
| `Must have exactly one of "folder" or "actorFullName" set.`, | ||
| ); | ||
| } | ||
|
|
||
| if (typeof (folder ?? actorFullName) !== 'string') { | ||
| throw new Error( | ||
| `Invalid "configs" entry at index ${index} in "${CONFIG_FILE_NAME}". ` + | ||
| `"folder"/"actorFullName" must be a string.`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return configs as ActorGlobConfigEntry[]; | ||
| }; | ||
|
|
||
| // Precedence, lowest to highest: matching folder-glob entries, matching actorFullName-glob | ||
| // entries, the actor's own literal entry. | ||
| const mergeGlobConfigs = ( | ||
| actorEntry: ActorConfigFileEntry, | ||
| folder: string, | ||
| configs: ActorGlobConfigEntry[], | ||
| ): ActorConfigFileEntry => { | ||
| const matchingFolderConfigs = configs.filter( | ||
| (configEntry) => configEntry.folder !== undefined && minimatch(folder, configEntry.folder), | ||
| ); | ||
| const matchingActorFullNameConfigs = configs.filter( | ||
| (configEntry) => | ||
| configEntry.actorFullName !== undefined && | ||
| typeof actorEntry.actorFullName === 'string' && | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already validate this eariler and the type should be string | undefined now, no? |
||
| minimatch(actorEntry.actorFullName, configEntry.actorFullName), | ||
| ); | ||
|
|
||
| let overlay: Partial<ActorConfigFileEntry> = {}; | ||
| for (const configEntry of [...matchingFolderConfigs, ...matchingActorFullNameConfigs]) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to support having both folder and name with AND logic, this would need to change |
||
| const { folder: matchedFolder, actorFullName: matchedActorFullName, ...rest } = configEntry; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's think a bit if we shouldn't separate the matching fields |
||
| overlay = { ...overlay, ...rest }; | ||
| } | ||
|
|
||
| return { ...overlay, ...actorEntry }; | ||
| }; | ||
|
|
||
| export const readConfigFile = async (selection: { actors: string[]; ignore: string[] }): Promise<ActorConfig[]> => { | ||
| let raw: string; | ||
| try { | ||
|
|
@@ -136,18 +193,24 @@ export const readConfigFile = async (selection: { actors: string[]; ignore: stri | |
| throw new Error(`Config file "${CONFIG_FILE_NAME}" must have an "actors" array at the top level.`); | ||
| } | ||
|
|
||
| if (config.configs !== undefined && !Array.isArray(config.configs)) { | ||
| throw new Error(`Config file "${CONFIG_FILE_NAME}" "configs" must be an array.`); | ||
| } | ||
| const globConfigs = config.configs ? validateGlobConfigEntries(config.configs) : []; | ||
|
|
||
| const seenFolders = new Set<string>(); | ||
| const actorConfigs: ActorConfig[] = []; | ||
|
|
||
| for (const [index, entry] of config.actors.entries()) { | ||
| if (typeof entry.folder !== 'string') { | ||
| for (const [index, rawEntry] of config.actors.entries()) { | ||
| if (typeof rawEntry.folder !== 'string') { | ||
| throw new Error( | ||
| `Invalid "folder" for actor entry at index ${index} in "${CONFIG_FILE_NAME}". ` + | ||
| `Must be a string (use "." for a single-actor repo).`, | ||
| ); | ||
| } | ||
|
|
||
| const folder = entry.folder === '.' ? '' : stripTrailingSlash(entry.folder); | ||
| const folder = rawEntry.folder === '.' ? '' : stripTrailingSlash(rawEntry.folder); | ||
| const entry = mergeGlobConfigs(rawEntry, folder, globConfigs); | ||
|
|
||
| if (seenFolders.has(folder)) { | ||
| throw new Error( | ||
|
|
@@ -217,6 +280,13 @@ export const readConfigFile = async (selection: { actors: string[]; ignore: stri | |
| ); | ||
| } | ||
|
|
||
| if (entry.tokenEnvVar === undefined) { | ||
| throw new Error( | ||
| `Missing "tokenEnvVar" for folder "${entry.folder}" (actor "${entry.actorFullName}") in "${CONFIG_FILE_NAME}". ` + | ||
| `Set it directly on the actor entry or via a matching "configs" entry.`, | ||
| ); | ||
| } | ||
|
|
||
| actorConfigs.push({ | ||
| actorFullName: entry.actorFullName, | ||
| folder, | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These types don't match very well, one says "File", other doesn't.
btw we already have ActorConfig type which is basically the same thing, we should either unify them or derive one from the other. Are there cases where these will differ? If we are simply merging them then they should not differ. No need to solve that in this PR but sooner rather than later.