fix: read group sub-step IDs as excluded as well, when a pipeline is skipped

Author: Dominic Scheirlinck

src/merge.ts

@@ -3,10 +3,10 @@ import debug from 'debug';
 import _ from 'lodash';
 import sendBuildkiteAnnotation from './annotate';
 import { updateDecisions } from './decide';
-import Config from './models/config';
-import { isGroupStep, mergeGroups } from './models/group-step';
+import Config, { keysInConfigs } from './models/config';
+import { mergeGroups } from './models/group-step';
 import { Pipeline } from './models/pipeline';
-import { Step } from './models/step';
+import { isGroupStep, Step } from './models/step';
 import { ARTIFACT_INJECTION_STEP_KEY, artifactInjectionSteps } from './steps/artifact-injection';
 import { nothingToDoSteps } from './steps/nothing-to-do';
 import { recordSuccessSteps } from './steps/record-success';
@@ -23,9 +23,7 @@ const log = debug('monofo:merge');
  * This method also mutates the steps of the passed-in configs directly
  */
 export function replaceExcludedKeys(configs: Config[], hasArtifactStep: boolean): void {
-  const excludedKeys: string[] = configs
-    .filter((c) => !c.included)
-    .flatMap((c) => c.steps.map((s) => (typeof s.key === 'string' ? s.key : '')).filter((v) => v));
+  const excludedKeys: string[] = keysInConfigs(configs.filter((c: Config) => !c.included));
 
   // If there's no artifact dependencies for the whole build, no need to wait for this step (it won't be added either)
   const replaceWith = hasArtifactStep ? ARTIFACT_INJECTION_STEP_KEY : '';

src/models/config.ts

@@ -13,8 +13,7 @@ import { FileHasher } from '../hash';
 import Reason, { ExcludeReasonType } from '../reason';
 import { globSet } from '../util/glob';
 import { strings } from '../util/helper';
-import { isGroupStep } from './group-step';
-import { Step } from './step';
+import { isGroupStep, keysInSteps, Step } from './step';
 
 const log = debug('monofo:config');
 
@@ -363,3 +362,7 @@ export default class Config {
     configs.forEach((c) => c.useFallback());
   }
 }
+
+export function keysInConfigs(configs: Config[]): string[] {
+  return configs.flatMap((config) => keysInSteps(config.steps));
+}

src/models/group-step.ts

@@ -1,6 +1,6 @@
 import _ from 'lodash';
 import { Pipeline } from './pipeline';
-import { GroupStep, Step } from './step';
+import { GroupStep, isGroupStep, Step } from './step';
 
 /**
  * Given a group step, returns the key that will be used for grouping on that step
@@ -37,10 +37,6 @@ function checkSimilarEnoughToMerge(step1: GroupStep, step2: GroupStep): void {
   }
 }
 
-export function isGroupStep(step: Step): step is GroupStep {
-  return step?.group === null || Boolean(step?.group);
-}
-
 /**
  * Merges groups within a merged pipeline
  *

src/models/step.ts

@@ -25,3 +25,24 @@ export interface GroupStep extends AbstractStep {
 }
 
 export type Step = CommandStep | GroupStep | TriggerStep | BlockStep | InputStep;
+
+export function isGroupStep(step: Step): step is GroupStep {
+  return step?.group === null || Boolean(step?.group);
+}
+
+/**
+ * Returns all step keys within the given set of steps
+ *
+ * Note that group steps con contain sub-steps, so it's not a simple map
+ */
+export function keysInSteps(steps: Step[]): string[] {
+  return steps.flatMap((step: Step) => {
+    const self = typeof step.key === 'string' ? [step.key] : [];
+
+    if (isGroupStep(step)) {
+      return [...self, ...keysInSteps(step.steps)];
+    }
+
+    return self;
+  });
+}

src/reason.ts

@@ -20,6 +20,7 @@ export enum ExcludeReasonType {
   NEVER_INCLUDED = 'been always excluded by monorepo.matches === false',
 }
 
+// TODO: should move to models, encompass decision state as well as reason (included/excluded should be modeled)
 export default class Reason {
   previousBuild?: string;
 

test/merge.test.ts

@@ -0,0 +1,23 @@
+import { replaceExcludedKeys } from '../src/merge';
+import Config from '../src/models/config';
+import Reason, { ExcludeReasonType } from '../src/reason';
+import { ARTIFACT_INJECTION_STEP_KEY } from '../src/steps/artifact-injection';
+import { getProjectFixturePath } from './fixtures';
+
+describe('merge', () => {
+  describe('replaceExcludedKeys', () => {
+    it('replaces depends_on inside group steps', async () => {
+      const configs = await Config.getAll(getProjectFixturePath('groups'));
+
+      // decide the first two configs aren't being included
+      configs[0].decide(false, new Reason(ExcludeReasonType.FORCED));
+      configs[1].decide(false, new Reason(ExcludeReasonType.FORCED));
+
+      // the other two configs should be mutated by this operation
+      replaceExcludedKeys(configs, true);
+
+      expect(configs[2].steps[0].depends_on).toStrictEqual([ARTIFACT_INJECTION_STEP_KEY]);
+      expect(configs[3].steps[0].depends_on).toStrictEqual([ARTIFACT_INJECTION_STEP_KEY]);
+    });
+  });
+});

test/models/step.test.ts

@@ -0,0 +1,17 @@
+import Config from '../../src/models/config';
+import { keysInSteps } from '../../src/models/step';
+import { getProjectFixturePath } from '../fixtures';
+
+describe('keysInSteps', () => {
+  it('gets recursive keys', async () => {
+    const configs = await Config.getAll(getProjectFixturePath('groups'));
+    const keys = configs.map((config) => keysInSteps(config.steps));
+
+    expect(keys).toStrictEqual([
+      ['foo1-group', 'foo1'],
+      ['foo1-group', 'foo2'],
+      ['foo3-group', 'foo3'],
+      ['foo4-group', 'foo4'],
+    ]);
+  });
+});

test/projects/groups/pipeline.foo3.yml

@@ -9,6 +9,7 @@ steps:
     key: "foo3-group"
     depends_on:
       - foo1-group
+      - foo1
     steps:
       - command: echo "foo3" > foo3
         key: foo3

test/projects/groups/pipeline.foo4.yml

@@ -2,8 +2,6 @@ monorepo:
   matches: "foo/**/README.md"
   expects: foo1
 
-# This tests our ability to stitch together groups, because otherwise Buildkite only supports appending to groups from later uploads
-
 steps:
   - group: "foo4 group"
     key: "foo4-group"