diff --git a/bin/actor-filtering.ts b/bin/actor-filtering.ts new file mode 100644 index 0000000..d56a904 --- /dev/null +++ b/bin/actor-filtering.ts @@ -0,0 +1,21 @@ +import type { ActorConfig } from './types.js'; + +/** + * Restricts a set of actors to those selected via `--actors` and not excluded via `--ignore`. + * Both filters match on `actorFullName` (`owner/name`). `--actors` is applied first (empty means + * "all"), then `--ignore` removes from the result. A name that doesn't exist in the config throws — + * a malformed selection must never silently build/release/delete the wrong set. The caller is + * responsible for turning that into a non-zero exit. + */ +export function selectActors({ actors, ignore }: { actors: string[]; ignore: string[] }, actorConfigs: ActorConfig[]) { + const fullNames = actorConfigs.map((actor) => actor.actorFullName); + const missing = [...actors, ...ignore].filter((name) => !fullNames.includes(name)); + if (missing.length > 0) { + throw new Error(`The following actors from the filter config do not exist: ${missing.join(', ')}`); + } + + const afterOnly = actors.length + ? actorConfigs.filter((actor) => actors.includes(actor.actorFullName)) + : actorConfigs; + return afterOnly.filter((actor) => !ignore.includes(actor.actorFullName)); +} diff --git a/bin/git.ts b/bin/git.ts index 9517670..bd04be4 100644 --- a/bin/git.ts +++ b/bin/git.ts @@ -94,7 +94,11 @@ const fetchAllBranchCommits = (sourceBranch: string, targetBranch: string): Comm * Gets the commits between sourceBranch and targetBranch (exclusive). * - If baseCommit is provided, only returns commits after the baseCommit. */ -export const getCommits = ({ sourceBranch, targetBranch, baseCommit }: Config): Commit[] => { +export const getCommits = ({ + sourceBranch, + targetBranch, + baseCommit, +}: Pick): Commit[] => { const baseCommitSha = parseBaseCommit(baseCommit); const commits = fetchAllBranchCommits(sourceBranch, targetBranch); diff --git a/bin/main.ts b/bin/main.ts index ee5fced..92bdfbb 100644 --- a/bin/main.ts +++ b/bin/main.ts @@ -21,7 +21,7 @@ import { readConfigFile, setCwd, spawnCommandInGhWorkspace } from './utils.js'; */ const middlewares = [setCwd]; -const buildOptions = (y: Argv) => { +export const buildOptions = (y: Argv) => { return y .option('target-branch', { type: 'string', @@ -37,27 +37,44 @@ const buildOptions = (y: Argv) => { }) .option('base-commit', { type: 'string', + demandOption: false, }); }; -const resolveChangedActors = async ( - { targetBranch, sourceBranch, baseCommit }: Config, - { isLatest }: { isLatest: boolean }, -) => { - const actorConfigs = await readConfigFile(); +/** + * Actor-selection flags, applied to every command that reads the actor config so a caller can + * narrow the set it operates on (e.g. two-stage releases: `--ignore X`, then `--actors X`). + * Kept separate from `buildOptions` so the read-only git commands don't advertise flags they ignore. + */ +export const actorSelectionOptions = (y: Argv) => { + return y + .option('actors', { + type: 'string', + array: true, + default: [] as string[], + }) + .option('ignore', { + type: 'string', + array: true, + default: [] as string[], + }); +}; + +const resolveChangedActors = async (config: Config, { isLatest }: { isLatest: boolean }) => { + const actorConfigs = await readConfigFile(config); - // This is an optimization for the common case where a branch only has cosmetic changes but had to merge in + // This is an optimization for the common case where a branch only has cosmetic changes but had to smerge in // functional changes from master (being up-to-date is a CI requirement). Master is already validated, and // since the branch has no functional changes of its own, there is nothing new to validate. // Exception: if the branch has any functional changes alongside the merge, we must re-test — even // individually validated changes can have novel interactions when combined. - if (hasMergeFromTarget(sourceBranch, targetBranch)) { + if (hasMergeFromTarget(config.sourceBranch, config.targetBranch)) { console.error( '[MERGE-FROM-TARGET-OPTIMIZATION]: There is merge from target branch, checking if there are no functional changes in our own branch. If so, we can skip tests', ); - const branchOnlyFiles = getBranchOnlyChangedFiles(sourceBranch, targetBranch); + const branchOnlyFiles = getBranchOnlyChangedFiles(config.sourceBranch, config.targetBranch); // Omit baseCommit to get full branch history. Validated functional commits can still interact with merged ones - const allBranchCommits = getCommits({ sourceBranch, targetBranch, baseCommit: undefined }); + const allBranchCommits = getCommits({ ...config, baseCommit: undefined }); const branchOnlyActorsChanged = getChangedActors({ filepathsChanged: branchOnlyFiles, actorConfigs, @@ -73,7 +90,7 @@ const resolveChangedActors = async ( } // If the optimization doesn't apply, we check all branch commits including merges for full coverage. We don't reuse the merge optimization results because here we can apply baseCommit and check merge commits (they might be functional or just cosmetic) - const commits = getCommits({ targetBranch, sourceBranch, baseCommit }); + const commits = getCommits(config); const changedFiles = getChangedFiles(commits); return getChangedActors({ filepathsChanged: changedFiles, actorConfigs, isLatest, commits }); }; @@ -103,22 +120,19 @@ await yargs() const changedFiles = getChangedFiles(commits); console.log(JSON.stringify(changedFiles)); }) + .command('get-actor-configs', '', actorSelectionOptions, async ({ actors, ignore }) => { + const actorConfigs = await readConfigFile({ actors, ignore }); + console.log(JSON.stringify(actorConfigs)); + }) .command( - 'get-actor-configs', + 'get-affected-actors', '', - (_) => _, - async () => { - const actorConfigs = await readConfigFile(); - console.log(JSON.stringify(actorConfigs)); + (args) => actorSelectionOptions(buildOptions(args)), + async (config) => { + const actorsChanged = await resolveChangedActors(config, { isLatest: false }); + console.log(JSON.stringify(actorsChanged)); }, ) - .command('get-affected-actors', '', buildOptions, async ({ targetBranch, sourceBranch, baseCommit }) => { - const actorsChanged = await resolveChangedActors( - { targetBranch, sourceBranch, baseCommit }, - { isLatest: false }, - ); - console.log(JSON.stringify(actorsChanged)); - }) .command( 'report-tests', '', @@ -135,12 +149,9 @@ await yargs() .command( 'build', '', - (args) => buildOptions(args).option('dry-run', { type: 'boolean', default: false }), - async ({ targetBranch, sourceBranch, baseCommit, dryRun, useDockerCache }) => { - const actorsChanged = await resolveChangedActors( - { targetBranch, sourceBranch, baseCommit }, - { isLatest: false }, - ); + (args) => actorSelectionOptions(buildOptions(args)).option('dry-run', { type: 'boolean', default: false }), + async (config) => { + const actorsChanged = await resolveChangedActors(config, { isLatest: false }); // https://github.com/apify-store/google-maps#:actors/lukaskrivka_google-maps-with-contact-details // git@github.com:apify-store/google-maps#:actors/lukaskrivka_google-maps-with-contact-details const repoUrl = spawnCommandInGhWorkspace(`git remote get-url origin`).replace( @@ -151,9 +162,9 @@ await yargs() const builds = await runBuilds({ repoUrl, actorConfigs: actorsChanged, - branch: sourceBranch.replace('origin/', ''), - dryRun, - useDockerCache, + branch: config.sourceBranch.replace('origin/', ''), + dryRun: config.dryRun, + useDockerCache: config.useDockerCache, }); console.log(JSON.stringify(builds)); }, @@ -162,7 +173,7 @@ await yargs() 'release', '', (args) => - args + actorSelectionOptions(args) .option('push-event-path', { type: 'string', demandOption: true }) .option('dry-run', { type: 'boolean', default: false }) .option('report-slack-channel', { type: 'string' }) @@ -173,7 +184,7 @@ await yargs() args.pushEventPath, ); const isLatest = true; - const actorConfigs = await readConfigFile(); + const actorConfigs = await readConfigFile(args); const actorsChanged = getChangedActors({ filepathsChanged: changedFiles, actorConfigs, @@ -206,37 +217,30 @@ await yargs() .command( 'build-from-local', '', - (args) => - args - .option('actors', { - type: 'string', - description: - 'Comma-separated actor names (owner/name) to build. Defaults to all actors in the repo.', - }) - .option('dry-run', { type: 'boolean', default: false }), - async ({ actors, dryRun }) => { - const allActorConfigs = await readConfigFile(); - const actorConfigs = actors - ? actors.split(',').map((name) => { - const trimmed = name.trim(); - const config = allActorConfigs.find((c) => c.actorFullName === trimmed); - if (!config) throw new Error(`Actor "${trimmed}" not found in repo`); - return config; - }) - : allActorConfigs; + (args) => actorSelectionOptions(args).option('dry-run', { type: 'boolean', default: false }), + async ({ actors, ignore, dryRun }) => { + const actorConfigs = await readConfigFile({ actors, ignore }); const builds = await runBuildsFromLocal({ actorConfigs, dryRun }); console.log(JSON.stringify(builds)); }, ) - .command( - 'delete-old-builds', - '', - (_) => _, - async () => { - const actorConfigs = await readConfigFile(); - await deleteOldBuilds(actorConfigs); - }, - ) + .command('delete-old-builds', '', actorSelectionOptions, async ({ actors, ignore }) => { + const actorConfigs = await readConfigFile({ actors, ignore }); + await deleteOldBuilds(actorConfigs); + }) .strictCommands() .demandCommand(1, 'Command is required') + .fail((msg, err, yargsInstance) => { + // Errors thrown from a command handler (e.g. an unknown actor passed to --actors/--ignore, + // or a missing config file) arrive here as `err`. A malformed selection must fail loudly + // rather than silently operate on the wrong set of actors — print the message, no stack. + if (err) { + console.error(`[ERROR]: ${err.message}`); + } else { + // Argument-parsing/validation failure — keep yargs' usage output. + console.error(yargsInstance.help()); + console.error(`\n${msg}`); + } + process.exit(1); + }) .parse(hideBin(process.argv)); diff --git a/bin/types.ts b/bin/types.ts index 7baa85c..f4a1b96 100644 --- a/bin/types.ts +++ b/bin/types.ts @@ -3,6 +3,8 @@ export interface Config { sourceBranch: string; baseCommit?: string; workspace?: string; + actors: string[]; + ignore: string[]; } export type Commit = { diff --git a/bin/utils.ts b/bin/utils.ts index ea400ad..a1e7967 100644 --- a/bin/utils.ts +++ b/bin/utils.ts @@ -6,6 +6,7 @@ import type { ActorVersionSourceFile } from 'apify-client'; 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'; @@ -113,7 +114,7 @@ const findOverlappingContextPaths = (contextPaths: string[]): [string, string] | return undefined; }; -export const readConfigFile = async (): Promise => { +export const readConfigFile = async (selection: { actors: string[]; ignore: string[] }): Promise => { let raw: string; try { raw = await fs.readFile(CONFIG_FILE_NAME, 'utf-8'); @@ -225,7 +226,7 @@ export const readConfigFile = async (): Promise => { }); } - return actorConfigs; + return selectActors(selection, actorConfigs); }; export const setCwd = ({ workspace }: { workspace: string | undefined }) => { diff --git a/test/unit/bin/actor-filtering.test.ts b/test/unit/bin/actor-filtering.test.ts new file mode 100644 index 0000000..b21eddb --- /dev/null +++ b/test/unit/bin/actor-filtering.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest'; + +import { selectActors } from '../../../bin/actor-filtering.js'; +import type { ActorConfig } from '../../../bin/types.js'; + +const actor = (actorFullName: string): ActorConfig => ({ + actorFullName, + folder: actorFullName.split('/')[1], + tokenEnvVar: 'TOKEN', + dockerContextDir: '.', + contextPaths: [], +}); + +const configs = [actor('owner/a'), actor('owner/b'), actor('owner/c')]; +const names = (result: ActorConfig[]) => result.map((c) => c.actorFullName); + +describe('selectActors', () => { + it('returns all actors when neither filter is set', () => { + expect(names(selectActors({ actors: [], ignore: [] }, configs))).toStrictEqual([ + 'owner/a', + 'owner/b', + 'owner/c', + ]); + }); + + it('keeps only the actors listed in --actors', () => { + expect(names(selectActors({ actors: ['owner/a', 'owner/c'], ignore: [] }, configs))).toStrictEqual([ + 'owner/a', + 'owner/c', + ]); + }); + + it('drops the actors listed in --ignore', () => { + expect(names(selectActors({ actors: [], ignore: ['owner/b'] }, configs))).toStrictEqual(['owner/a', 'owner/c']); + }); + + it('applies --actors first, then removes --ignore from that subset', () => { + expect(names(selectActors({ actors: ['owner/a', 'owner/b'], ignore: ['owner/b'] }, configs))).toStrictEqual([ + 'owner/a', + ]); + }); + + it('can select down to nothing when --actors and --ignore overlap fully', () => { + expect(selectActors({ actors: ['owner/a'], ignore: ['owner/a'] }, configs)).toStrictEqual([]); + }); + + it('throws listing every unknown name across both filters', () => { + expect(() => selectActors({ actors: ['owner/x'], ignore: ['owner/y'] }, configs)).toThrow( + 'The following actors from the filter config do not exist: owner/x, owner/y', + ); + }); +}); diff --git a/test/unit/bin/utils.test.ts b/test/unit/bin/utils.test.ts index 5fa2149..893dcac 100644 --- a/test/unit/bin/utils.test.ts +++ b/test/unit/bin/utils.test.ts @@ -12,6 +12,8 @@ vi.mock('node:fs/promises', () => ({ default: fsMock })); afterEach(() => vi.restoreAllMocks()); +const emptyActorSelection = { actors: [], ignore: [] }; + const validConfig = (actors: object[]) => JSON.stringify({ actors }); const actorJson = (fields: Record = {}) => JSON.stringify(fields); @@ -39,7 +41,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({ dockerContextDir: '../../..' }), }); - const result = await readConfigFile(); + const result = await readConfigFile(emptyActorSelection); expectFileRead('actors/shopify/.actor/actor.json'); expect(result).toEqual([ { @@ -60,7 +62,7 @@ describe('readConfigFile', () => { '.actor/actor.json': actorJson({}), }); - const result = await readConfigFile(); + const result = await readConfigFile(emptyActorSelection); expect(result[0].folder).toBe(''); expectFileRead('.actor/actor.json'); }); @@ -73,7 +75,7 @@ describe('readConfigFile', () => { 'actors/web-scraper/.actor/actor.json': actorJson({}), }); - const result = await readConfigFile(); + const result = await readConfigFile(emptyActorSelection); expect(result[0].dockerContextDir).toBe('actors/web-scraper'); expect(result[0].contextPaths).toEqual(['actors/web-scraper']); }); @@ -86,7 +88,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({ dockerContextDir: '../../..' }), }); - const result = await readConfigFile(); + const result = await readConfigFile(emptyActorSelection); expect(result[0].dockerContextDir).toBe(''); }); @@ -103,7 +105,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({ dockerContextDir: '../../..' }), }); - const result = await readConfigFile(); + const result = await readConfigFile(emptyActorSelection); expect(result[0].contextPaths).toEqual(['actors/shopify', 'packages']); }); @@ -121,7 +123,7 @@ describe('readConfigFile', () => { 'actors/email-sender/.actor/actor.json': actorJson({}), }); - const result = await readConfigFile(); + const result = await readConfigFile(emptyActorSelection); expect(result).toHaveLength(2); expect(result[0].actorFullName).toBe('apify/web-scraper'); expect(result[1].actorFullName).toBe('other-team/email-sender'); @@ -129,17 +131,17 @@ describe('readConfigFile', () => { it('throws when config file is missing', async () => { fsMock.readFile.mockRejectedValue(new Error('ENOENT')); - await expect(readConfigFile()).rejects.toThrow('not found'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('not found'); }); it('throws when config file contains invalid JSON', async () => { fsMock.readFile.mockResolvedValue('{bad json'); - await expect(readConfigFile()).rejects.toThrow('invalid JSON'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('invalid JSON'); }); it('throws when actors array is missing', async () => { fsMock.readFile.mockResolvedValue(JSON.stringify({ notActors: [] })); - await expect(readConfigFile()).rejects.toThrow('"actors" array'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('"actors" array'); }); it('throws on duplicate folders', async () => { @@ -151,7 +153,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile()).rejects.toThrow('Duplicate folder'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Duplicate folder'); }); it('throws on duplicate folders after normalization ("." and "")', async () => { @@ -163,7 +165,7 @@ describe('readConfigFile', () => { '.actor/actor.json': actorJson({}), }); - await expect(readConfigFile()).rejects.toThrow('Duplicate folder'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Duplicate folder'); }); it('throws when actor.json is missing', async () => { @@ -173,7 +175,7 @@ describe('readConfigFile', () => { ]), }); - await expect(readConfigFile()).rejects.toThrow('Cannot read'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Cannot read'); }); it('throws when folder is missing', async () => { @@ -181,7 +183,7 @@ describe('readConfigFile', () => { [CONFIG_FILE_NAME]: validConfig([{ actorFullName: 'apify/shopify', tokenEnvVar: 'APIFY_TOKEN_APIFY' }]), }); - await expect(readConfigFile()).rejects.toThrow(/Invalid "folder"/); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/Invalid "folder"/); }); it('throws when folder is not a string', async () => { @@ -191,7 +193,7 @@ describe('readConfigFile', () => { ]), }); - await expect(readConfigFile()).rejects.toThrow(/Invalid "folder"/); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/Invalid "folder"/); }); it('throws when actorFullName is missing', async () => { @@ -200,7 +202,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile()).rejects.toThrow('Invalid "actorFullName"'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "actorFullName"'); }); it('throws when actorFullName has no slash', async () => { @@ -211,7 +213,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile()).rejects.toThrow('Invalid "actorFullName"'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "actorFullName"'); }); it('throws when actorFullName has empty parts', async () => { @@ -222,7 +224,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile()).rejects.toThrow('Invalid "actorFullName"'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "actorFullName"'); }); it('throws when overrideActorContext is not an array', async () => { @@ -238,7 +240,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile()).rejects.toThrow('Invalid "overrideActorContext"'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "overrideActorContext"'); }); it('throws when overrideActorContext contains non-strings', async () => { @@ -254,7 +256,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile()).rejects.toThrow('Invalid "overrideActorContext"'); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow('Invalid "overrideActorContext"'); }); it('throws when overrideActorContext entries overlap (one is a prefix of another)', async () => { @@ -270,7 +272,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile()).rejects.toThrow(/overlap/); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/overlap/); }); it('throws when overrideActorContext contains the repo root alongside another entry', async () => { @@ -286,7 +288,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - await expect(readConfigFile()).rejects.toThrow(/overlap/); + await expect(readConfigFile(emptyActorSelection)).rejects.toThrow(/overlap/); }); it('adds the actor own folder automatically when overrideActorContext does not cover it', async () => { @@ -302,7 +304,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - const result = await readConfigFile(); + const result = await readConfigFile(emptyActorSelection); expect(result[0].contextPaths).toEqual(['code', 'shared', 'actors/shopify']); }); @@ -319,7 +321,7 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - const result = await readConfigFile(); + const result = await readConfigFile(emptyActorSelection); expect(result[0].folder).toBe('actors/shopify'); expect(result[0].contextPaths).toEqual(['actors/shopify', 'packages']); }); @@ -337,7 +339,43 @@ describe('readConfigFile', () => { 'actors/shopify/.actor/actor.json': actorJson({}), }); - const result = await readConfigFile(); + const result = await readConfigFile(emptyActorSelection); expect(result[0].contextPaths).toEqual(['actors/shopify', 'code', 'shared']); }); + + describe('actor selection', () => { + const twoActors = () => + mockFiles({ + [CONFIG_FILE_NAME]: validConfig([ + { folder: 'actors/a', actorFullName: 'team/a', tokenEnvVar: 'TOKEN' }, + { folder: 'actors/b', actorFullName: 'team/b', tokenEnvVar: 'TOKEN' }, + ]), + 'actors/a/.actor/actor.json': actorJson({}), + 'actors/b/.actor/actor.json': actorJson({}), + }); + + const fullNames = (result: { actorFullName: string }[]) => result.map((c) => c.actorFullName); + + it('returns all actors when the selection is empty', async () => { + twoActors(); + expect(fullNames(await readConfigFile(emptyActorSelection))).toEqual(['team/a', 'team/b']); + }); + + it('keeps only the selected actors', async () => { + twoActors(); + expect(fullNames(await readConfigFile({ actors: ['team/a'], ignore: [] }))).toEqual(['team/a']); + }); + + it('drops ignored actors', async () => { + twoActors(); + expect(fullNames(await readConfigFile({ actors: [], ignore: ['team/a'] }))).toEqual(['team/b']); + }); + + it('throws on an unknown actor name', async () => { + twoActors(); + await expect(readConfigFile({ actors: ['team/nope'], ignore: [] })).rejects.toThrow( + 'do not exist: team/nope', + ); + }); + }); });