From 1ce754ddbb290eb13f7ef84bd5d016a55cd60bb6 Mon Sep 17 00:00:00 2001 From: Henry Pan Date: Thu, 7 Oct 2021 11:09:03 +0800 Subject: [PATCH 1/2] feat: add overridable to ignore acceptAllConditions --- .../__snapshots__/conditional.spec.ts.snap | 11 ++++++++++ src/__tests__/conditional.spec.ts | 20 ++++++++++++++++++- src/conditional.ts | 14 ++++++++++++- src/sortedSteps.ts | 17 +++++++++------- src/unwrapSteps.ts | 5 ++++- 5 files changed, 57 insertions(+), 10 deletions(-) diff --git a/src/__tests__/__snapshots__/conditional.spec.ts.snap b/src/__tests__/__snapshots__/conditional.spec.ts.snap index 6bc3410..11cce3e 100644 --- a/src/__tests__/__snapshots__/conditional.spec.ts.snap +++ b/src/__tests__/__snapshots__/conditional.spec.ts.snap @@ -705,6 +705,17 @@ exports[`buildkite-graph Steps Command isEffectOf will not add steps if any effe exports[`buildkite-graph Steps Command isEffectOf will not add steps if effect dependency is rejected structure 1`] = `""`; +exports[`buildkite-graph Steps Command isEffectOf will not override when acceptAllConditions is set but isOverridable returns false in a condition json_depends_on_accept_all 1`] = ` +Object { + "steps": Array [], +} +`; + +exports[`buildkite-graph Steps Command isEffectOf will not override when acceptAllConditions is set but isOverridable returns false in a condition yaml_depends_on_accept_all 1`] = ` +"steps: [] +" +`; + exports[`buildkite-graph Steps Command step addition dot 1`] = ` "digraph \\"whatever\\" { graph [ compound =true ]; diff --git a/src/__tests__/conditional.spec.ts b/src/__tests__/conditional.spec.ts index 320de5e..5074ef5 100644 --- a/src/__tests__/conditional.spec.ts +++ b/src/__tests__/conditional.spec.ts @@ -11,8 +11,9 @@ class MyConditional extends Conditional { constructor( step: ThingOrGenerator, private readonly accepted: ReturnType['accept']>, + overridable = true, ) { - super(step as any); + super(step as any, overridable); } accept(): ReturnType['accept']> { @@ -177,6 +178,23 @@ describe('buildkite-graph', () => { ['json_depends_on_accept_all', 'yaml_depends_on_accept_all'], ); + createTest( + 'will not override when acceptAllConditions is set but isOverridable returns false in a condition', + () => { + const acceptedTests = new MyConditional( + new CommandStep('run tests'), + false, + false, + ); + const deployCoverage = new CommandStep( + 'deploy coverage', + ).isEffectOf(acceptedTests); + + return new Pipeline('x').add(acceptedTests, deployCoverage); + }, + ['json_depends_on_accept_all', 'yaml_depends_on_accept_all'], + ); + it('should not execute conditional.accept when acceptAllConditions is set', () => { const acceptedTests = new MyConditional( new CommandStep('run tests'), diff --git a/src/conditional.ts b/src/conditional.ts index 0251bf3..8febc1b 100644 --- a/src/conditional.ts +++ b/src/conditional.ts @@ -4,7 +4,10 @@ export type Generator = () => T | Promise; export type ThingOrGenerator = T | Promise | Generator; export abstract class Conditional { - constructor(private readonly guarded: ThingOrGenerator) {} + constructor( + private readonly guarded: ThingOrGenerator, + private readonly overridable = true, + ) {} get(): T | Promise { return typeof this.guarded === 'function' ? this.guarded() : this.guarded; @@ -15,4 +18,13 @@ export abstract class Conditional { * The step is rejected if the returned boolean is false or the promise resolves to false. */ abstract accept(): boolean | Promise; + + /** + * Whether this condition could be overriden to accept when `acceptAllConditions` is set. `overridable` is + * set to true by default. + * @returns boolean + */ + isOverridable(): boolean { + return this.overridable; + } } diff --git a/src/sortedSteps.ts b/src/sortedSteps.ts index b914e16..99db0fb 100644 --- a/src/sortedSteps.ts +++ b/src/sortedSteps.ts @@ -65,14 +65,17 @@ export async function sortedSteps( potentialEffectDependency, ); - // Accept all conditions/steps regardless - if (acceptAllConditions) { - addDependency(dependency); - s.dependsOn(potentialEffectDependency); - continue; - } - if (potentialEffectDependency instanceof Conditional) { + // Accept all conditions/steps regardless + if ( + acceptAllConditions && + potentialEffectDependency.isOverridable() + ) { + addDependency(dependency); + s.dependsOn(potentialEffectDependency); + continue; + } + // in case it is a conditional we are interested in whether it that one was accepted or not if ( (await potentialEffectDependency.accept()) || diff --git a/src/unwrapSteps.ts b/src/unwrapSteps.ts index 15bcfa0..398a720 100644 --- a/src/unwrapSteps.ts +++ b/src/unwrapSteps.ts @@ -14,7 +14,10 @@ export async function unwrapSteps( if (cache.has(s)) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion ret.push(cache.get(s)!); - } else if (acceptAllConditions || (await s.accept()) === true) { + } else if ( + (acceptAllConditions && s.isOverridable()) || + (await s.accept()) === true + ) { const cond = await s.get(); cache.set(s, cond); ret.push(cond); From 1fb0c2bae6d7a7d52eb7f32606fdb2b756c7e3ff Mon Sep 17 00:00:00 2001 From: Henry Pan Date: Thu, 7 Oct 2021 11:12:27 +0800 Subject: [PATCH 2/2] fix: typo --- src/conditional.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conditional.ts b/src/conditional.ts index 8febc1b..af03a4c 100644 --- a/src/conditional.ts +++ b/src/conditional.ts @@ -20,7 +20,7 @@ export abstract class Conditional { abstract accept(): boolean | Promise; /** - * Whether this condition could be overriden to accept when `acceptAllConditions` is set. `overridable` is + * Whether this condition could be overridden to accept when `acceptAllConditions` is set. `overridable` is * set to true by default. * @returns boolean */