From b90066b9dcc7766bb76f4443d79c31ffb78f8695 Mon Sep 17 00:00:00 2001 From: JuanGalilea Date: Tue, 4 Aug 2026 16:30:57 +0200 Subject: [PATCH 1/4] feat(only-and-omit): pick or exclude actors --- bin/actor-filtering.ts | 21 ++++++++++++++++ bin/main.ts | 52 +++++++++++++++++++++------------------ bin/types.ts | 2 ++ test/unit/bin/git.test.ts | 13 +++++++--- 4 files changed, 60 insertions(+), 28 deletions(-) create mode 100644 bin/actor-filtering.ts diff --git a/bin/actor-filtering.ts b/bin/actor-filtering.ts new file mode 100644 index 0000000..6a8f32d --- /dev/null +++ b/bin/actor-filtering.ts @@ -0,0 +1,21 @@ +import type { ActorConfig, Config } from './types.js'; + +export function filterActorByConfig(config: Config, actorConfig: ActorConfig[]) { + const fullNames = actorConfig.map((x) => x.actorFullName); + + const missingOmitActors = config.omitActors.filter((name) => !fullNames.includes(name)); + const missingOnlyActors = config.onlyActors.filter((name) => !fullNames.includes(name)); + + if (missingOmitActors.length > 0 || missingOnlyActors.length > 0) { + const missing = [...missingOmitActors, ...missingOnlyActors]; + console.error(`[ERROR]: The following actors from the filter config do not exist: ${missing.join(', ')}`); + process.exit(1); + } + + const actorsAfterOnly = config.onlyActors.length + ? actorConfig.filter((actor) => config.onlyActors.includes(actor.actorFullName)) + : actorConfig; + const actorsAfterOmit = actorsAfterOnly.filter((actor) => !config.omitActors.includes(actor.actorFullName)); + + return actorsAfterOmit; +} diff --git a/bin/main.ts b/bin/main.ts index ee5fced..7fb54fc 100644 --- a/bin/main.ts +++ b/bin/main.ts @@ -6,6 +6,7 @@ import yargs, { type Argv } from 'yargs'; // eslint-disable-next-line import/extensions --- With .js, it cannot find types import { hideBin } from 'yargs/helpers'; +import { filterActorByConfig } from './actor-filtering.js'; import { deleteOldBuilds, runBuilds } from './build.js'; import { runBuildsFromLocal } from './build-from-local.js'; import { getChangedActors } from './diff-changes.js'; @@ -21,7 +22,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 +38,36 @@ const buildOptions = (y: Argv) => { }) .option('base-commit', { type: 'string', + demandOption: false, + }) + .option('omit-actors', { + type: 'string', + array: true, + default: [] as string[], + }) + .option('only-actors', { + type: 'string', + array: true, + default: [] as string[], }); }; -const resolveChangedActors = async ( - { targetBranch, sourceBranch, baseCommit }: Config, - { isLatest }: { isLatest: boolean }, -) => { - const actorConfigs = await readConfigFile(); +const resolveChangedActors = async (config: Config, { isLatest }: { isLatest: boolean }) => { + const originalActorConfigs = await readConfigFile(); + const actorConfigs = filterActorByConfig(config, originalActorConfigs); - // 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 +83,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 }); }; @@ -112,11 +122,8 @@ await yargs() console.log(JSON.stringify(actorConfigs)); }, ) - .command('get-affected-actors', '', buildOptions, async ({ targetBranch, sourceBranch, baseCommit }) => { - const actorsChanged = await resolveChangedActors( - { targetBranch, sourceBranch, baseCommit }, - { isLatest: false }, - ); + .command('get-affected-actors', '', buildOptions, async (config) => { + const actorsChanged = await resolveChangedActors(config, { isLatest: false }); console.log(JSON.stringify(actorsChanged)); }) .command( @@ -136,11 +143,8 @@ await yargs() '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 }, - ); + 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 +155,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)); }, diff --git a/bin/types.ts b/bin/types.ts index 7baa85c..d6c9711 100644 --- a/bin/types.ts +++ b/bin/types.ts @@ -3,6 +3,8 @@ export interface Config { sourceBranch: string; baseCommit?: string; workspace?: string; + omitActors: string[]; + onlyActors: string[]; } export type Commit = { diff --git a/test/unit/bin/git.test.ts b/test/unit/bin/git.test.ts index 84fec14..dcf56a1 100644 --- a/test/unit/bin/git.test.ts +++ b/test/unit/bin/git.test.ts @@ -10,6 +10,11 @@ import { } from '../../../bin/git.js'; import * as Utils from '../../../bin/utils.js'; +const emptyConfig = { + omitActors: [], + onlyActors: [], +}; + describe('getCommits', () => { const sourceBranch = 'feature-branch'; const targetBranch = 'main'; @@ -32,7 +37,7 @@ describe('getCommits', () => { it('should return commits between source and target branches', () => { // Act - const commits = getCommits({ sourceBranch, targetBranch }); + const commits = getCommits({ ...emptyConfig, sourceBranch, targetBranch }); // Assert expect(commits).toStrictEqual([ @@ -49,7 +54,7 @@ describe('getCommits', () => { it('should return commits after the base commit if provided', () => { // Act - const commits = getCommits({ sourceBranch, targetBranch, baseCommit: sha1 }); + const commits = getCommits({ ...emptyConfig, sourceBranch, targetBranch, baseCommit: sha1 }); // Assert expect(commits).toStrictEqual([ @@ -65,7 +70,7 @@ describe('getCommits', () => { it('should ignore the base commit and return all commits when it is the branch HEAD (rerun or force push)', () => { // Act - const commits = getCommits({ sourceBranch, targetBranch, baseCommit: sha3 }); + const commits = getCommits({ ...emptyConfig, sourceBranch, targetBranch, baseCommit: sha3 }); // Assert expect(commits).toStrictEqual([ @@ -77,7 +82,7 @@ describe('getCommits', () => { it('should return all commits if base commit is not found', () => { // Act - const commits = getCommits({ sourceBranch, targetBranch, baseCommit: 'a'.repeat(40) }); + const commits = getCommits({ ...emptyConfig, sourceBranch, targetBranch, baseCommit: 'a'.repeat(40) }); // Assert expect(commits).toStrictEqual([ From 13095c40064faa9918b51a9b2fab1b3af0fbc1e6 Mon Sep 17 00:00:00 2001 From: JuanGalilea Date: Wed, 5 Aug 2026 16:50:17 +0200 Subject: [PATCH 2/4] switch arg names and extend to other commands --- bin/actor-filtering.ts | 30 +++++++-------- bin/git.ts | 6 ++- bin/main.ts | 79 ++++++++++++++++++--------------------- bin/types.ts | 4 +- test/unit/bin/git.test.ts | 13 ++----- 5 files changed, 62 insertions(+), 70 deletions(-) diff --git a/bin/actor-filtering.ts b/bin/actor-filtering.ts index 6a8f32d..4d26ab6 100644 --- a/bin/actor-filtering.ts +++ b/bin/actor-filtering.ts @@ -1,21 +1,21 @@ -import type { ActorConfig, Config } from './types.js'; +import type { ActorConfig } from './types.js'; -export function filterActorByConfig(config: Config, actorConfig: ActorConfig[]) { - const fullNames = actorConfig.map((x) => x.actorFullName); - - const missingOmitActors = config.omitActors.filter((name) => !fullNames.includes(name)); - const missingOnlyActors = config.onlyActors.filter((name) => !fullNames.includes(name)); - - if (missingOmitActors.length > 0 || missingOnlyActors.length > 0) { - const missing = [...missingOmitActors, ...missingOnlyActors]; +/** + * 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 aborts + * the process — a malformed selection must never silently build/release/delete the wrong set. + */ +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) { console.error(`[ERROR]: The following actors from the filter config do not exist: ${missing.join(', ')}`); process.exit(1); } - const actorsAfterOnly = config.onlyActors.length - ? actorConfig.filter((actor) => config.onlyActors.includes(actor.actorFullName)) - : actorConfig; - const actorsAfterOmit = actorsAfterOnly.filter((actor) => !config.omitActors.includes(actor.actorFullName)); - - return actorsAfterOmit; + 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 7fb54fc..3118dd5 100644 --- a/bin/main.ts +++ b/bin/main.ts @@ -6,7 +6,7 @@ import yargs, { type Argv } from 'yargs'; // eslint-disable-next-line import/extensions --- With .js, it cannot find types import { hideBin } from 'yargs/helpers'; -import { filterActorByConfig } from './actor-filtering.js'; +import { selectActors } from './actor-filtering.js'; import { deleteOldBuilds, runBuilds } from './build.js'; import { runBuildsFromLocal } from './build-from-local.js'; import { getChangedActors } from './diff-changes.js'; @@ -39,13 +39,22 @@ export const buildOptions = (y: Argv) => { .option('base-commit', { type: 'string', demandOption: false, - }) - .option('omit-actors', { + }); +}; + +/** + * 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('only-actors', { + .option('ignore', { type: 'string', array: true, default: [] as string[], @@ -54,7 +63,7 @@ export const buildOptions = (y: Argv) => { const resolveChangedActors = async (config: Config, { isLatest }: { isLatest: boolean }) => { const originalActorConfigs = await readConfigFile(); - const actorConfigs = filterActorByConfig(config, originalActorConfigs); + const actorConfigs = selectActors(config, originalActorConfigs); // 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 @@ -113,19 +122,20 @@ await yargs() const changedFiles = getChangedFiles(commits); console.log(JSON.stringify(changedFiles)); }) + .command('get-actor-configs', '', actorSelectionOptions, async ({ actors, ignore }) => { + const allActorConfigs = await readConfigFile(); + const actorConfigs = selectActors({ actors, ignore }, allActorConfigs); + 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 (config) => { - const actorsChanged = await resolveChangedActors(config, { isLatest: false }); - console.log(JSON.stringify(actorsChanged)); - }) .command( 'report-tests', '', @@ -142,7 +152,7 @@ await yargs() .command( 'build', '', - (args) => buildOptions(args).option('dry-run', { type: 'boolean', default: 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 @@ -166,7 +176,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' }) @@ -177,7 +187,8 @@ await yargs() args.pushEventPath, ); const isLatest = true; - const actorConfigs = await readConfigFile(); + const allActorConfigs = await readConfigFile(); + const actorConfigs = selectActors(args, allActorConfigs); const actorsChanged = getChangedActors({ filepathsChanged: changedFiles, actorConfigs, @@ -210,37 +221,19 @@ 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 }) => { + (args) => actorSelectionOptions(args).option('dry-run', { type: 'boolean', default: false }), + async ({ actors, ignore, 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; + const actorConfigs = selectActors({ actors, ignore }, allActorConfigs); 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 allActorConfigs = await readConfigFile(); + const actorConfigs = selectActors({ actors, ignore }, allActorConfigs); + await deleteOldBuilds(actorConfigs); + }) .strictCommands() .demandCommand(1, 'Command is required') .parse(hideBin(process.argv)); diff --git a/bin/types.ts b/bin/types.ts index d6c9711..f4a1b96 100644 --- a/bin/types.ts +++ b/bin/types.ts @@ -3,8 +3,8 @@ export interface Config { sourceBranch: string; baseCommit?: string; workspace?: string; - omitActors: string[]; - onlyActors: string[]; + actors: string[]; + ignore: string[]; } export type Commit = { diff --git a/test/unit/bin/git.test.ts b/test/unit/bin/git.test.ts index dcf56a1..84fec14 100644 --- a/test/unit/bin/git.test.ts +++ b/test/unit/bin/git.test.ts @@ -10,11 +10,6 @@ import { } from '../../../bin/git.js'; import * as Utils from '../../../bin/utils.js'; -const emptyConfig = { - omitActors: [], - onlyActors: [], -}; - describe('getCommits', () => { const sourceBranch = 'feature-branch'; const targetBranch = 'main'; @@ -37,7 +32,7 @@ describe('getCommits', () => { it('should return commits between source and target branches', () => { // Act - const commits = getCommits({ ...emptyConfig, sourceBranch, targetBranch }); + const commits = getCommits({ sourceBranch, targetBranch }); // Assert expect(commits).toStrictEqual([ @@ -54,7 +49,7 @@ describe('getCommits', () => { it('should return commits after the base commit if provided', () => { // Act - const commits = getCommits({ ...emptyConfig, sourceBranch, targetBranch, baseCommit: sha1 }); + const commits = getCommits({ sourceBranch, targetBranch, baseCommit: sha1 }); // Assert expect(commits).toStrictEqual([ @@ -70,7 +65,7 @@ describe('getCommits', () => { it('should ignore the base commit and return all commits when it is the branch HEAD (rerun or force push)', () => { // Act - const commits = getCommits({ ...emptyConfig, sourceBranch, targetBranch, baseCommit: sha3 }); + const commits = getCommits({ sourceBranch, targetBranch, baseCommit: sha3 }); // Assert expect(commits).toStrictEqual([ @@ -82,7 +77,7 @@ describe('getCommits', () => { it('should return all commits if base commit is not found', () => { // Act - const commits = getCommits({ ...emptyConfig, sourceBranch, targetBranch, baseCommit: 'a'.repeat(40) }); + const commits = getCommits({ sourceBranch, targetBranch, baseCommit: 'a'.repeat(40) }); // Assert expect(commits).toStrictEqual([ From 90577d0d5a97e6ad017d10b64da580415de50b0f Mon Sep 17 00:00:00 2001 From: JuanGalilea Date: Wed, 5 Aug 2026 17:11:25 +0200 Subject: [PATCH 3/4] error management so process.exit is not used in random places --- bin/actor-filtering.ts | 8 ++--- bin/main.ts | 13 +++++++ test/unit/bin/actor-filtering.test.ts | 52 +++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 test/unit/bin/actor-filtering.test.ts diff --git a/bin/actor-filtering.ts b/bin/actor-filtering.ts index 4d26ab6..d56a904 100644 --- a/bin/actor-filtering.ts +++ b/bin/actor-filtering.ts @@ -3,15 +3,15 @@ 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 aborts - * the process — a malformed selection must never silently build/release/delete the wrong set. + * "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) { - console.error(`[ERROR]: The following actors from the filter config do not exist: ${missing.join(', ')}`); - process.exit(1); + throw new Error(`The following actors from the filter config do not exist: ${missing.join(', ')}`); } const afterOnly = actors.length diff --git a/bin/main.ts b/bin/main.ts index 3118dd5..8ed99de 100644 --- a/bin/main.ts +++ b/bin/main.ts @@ -236,4 +236,17 @@ await yargs() }) .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/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', + ); + }); +}); From 84de99693a5ced9fade17c72b997e11b59e78e2d Mon Sep 17 00:00:00 2001 From: JuanGalilea Date: Wed, 5 Aug 2026 17:34:15 +0200 Subject: [PATCH 4/4] include actor selection logic in the reading of configuration --- bin/main.ts | 16 +++---- bin/utils.ts | 5 ++- test/unit/bin/utils.test.ts | 86 ++++++++++++++++++++++++++----------- 3 files changed, 70 insertions(+), 37 deletions(-) diff --git a/bin/main.ts b/bin/main.ts index 8ed99de..92bdfbb 100644 --- a/bin/main.ts +++ b/bin/main.ts @@ -6,7 +6,6 @@ import yargs, { type Argv } from 'yargs'; // eslint-disable-next-line import/extensions --- With .js, it cannot find types import { hideBin } from 'yargs/helpers'; -import { selectActors } from './actor-filtering.js'; import { deleteOldBuilds, runBuilds } from './build.js'; import { runBuildsFromLocal } from './build-from-local.js'; import { getChangedActors } from './diff-changes.js'; @@ -62,8 +61,7 @@ export const actorSelectionOptions = (y: Argv) => { }; const resolveChangedActors = async (config: Config, { isLatest }: { isLatest: boolean }) => { - const originalActorConfigs = await readConfigFile(); - const actorConfigs = selectActors(config, originalActorConfigs); + const actorConfigs = await readConfigFile(config); // 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 @@ -123,8 +121,7 @@ await yargs() console.log(JSON.stringify(changedFiles)); }) .command('get-actor-configs', '', actorSelectionOptions, async ({ actors, ignore }) => { - const allActorConfigs = await readConfigFile(); - const actorConfigs = selectActors({ actors, ignore }, allActorConfigs); + const actorConfigs = await readConfigFile({ actors, ignore }); console.log(JSON.stringify(actorConfigs)); }) .command( @@ -187,8 +184,7 @@ await yargs() args.pushEventPath, ); const isLatest = true; - const allActorConfigs = await readConfigFile(); - const actorConfigs = selectActors(args, allActorConfigs); + const actorConfigs = await readConfigFile(args); const actorsChanged = getChangedActors({ filepathsChanged: changedFiles, actorConfigs, @@ -223,15 +219,13 @@ await yargs() '', (args) => actorSelectionOptions(args).option('dry-run', { type: 'boolean', default: false }), async ({ actors, ignore, dryRun }) => { - const allActorConfigs = await readConfigFile(); - const actorConfigs = selectActors({ actors, ignore }, allActorConfigs); + const actorConfigs = await readConfigFile({ actors, ignore }); const builds = await runBuildsFromLocal({ actorConfigs, dryRun }); console.log(JSON.stringify(builds)); }, ) .command('delete-old-builds', '', actorSelectionOptions, async ({ actors, ignore }) => { - const allActorConfigs = await readConfigFile(); - const actorConfigs = selectActors({ actors, ignore }, allActorConfigs); + const actorConfigs = await readConfigFile({ actors, ignore }); await deleteOldBuilds(actorConfigs); }) .strictCommands() 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/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', + ); + }); + }); });