From c88234c6fcc5730b9e238d97dc85122af8c70c3d Mon Sep 17 00:00:00 2001 From: Alan Quach Date: Mon, 17 Jun 2019 11:30:02 -0700 Subject: [PATCH] refactor(stages): Fixed alias matching, added fallback and unit tests (#7080) --- .../pipeline/config/PipelineRegistry.spec.ts | 99 +++++++++++++++++++ .../src/pipeline/config/PipelineRegistry.ts | 39 +++++++- .../pipeline/service/ExecutionsTransformer.ts | 4 + 3 files changed, 141 insertions(+), 1 deletion(-) diff --git a/app/scripts/modules/core/src/pipeline/config/PipelineRegistry.spec.ts b/app/scripts/modules/core/src/pipeline/config/PipelineRegistry.spec.ts index 31a6f5d7ee7..ae1b8281c87 100644 --- a/app/scripts/modules/core/src/pipeline/config/PipelineRegistry.spec.ts +++ b/app/scripts/modules/core/src/pipeline/config/PipelineRegistry.spec.ts @@ -6,6 +6,7 @@ import { IStage, ITriggerTypeConfig, IStageTypeConfig } from 'core/domain'; import { IRegion } from 'core/account/AccountService'; import { Registry } from 'core/registry'; import { ITriggerTemplateComponentProps } from '../manualExecution/TriggerTemplate'; +import { PipelineRegistry } from './PipelineRegistry'; const mockProviderAccount = { accountId: 'abc', @@ -181,6 +182,104 @@ describe('PipelineRegistry: API', function() { ); }); + describe('getStageConfig all permutations', function() { + const unmatchedStage = { key: 'unmatched', description: 'Unmatched stage' }; + const simpleStage = { key: 'a', description: 'Simple stage with no provides or alias' }; + const renamedStage = { + key: 'b', + alias: 'z', + description: + '(Renamed) Stage used to be called "z" but is now standardized to "b", we still need to be able to match "z" stages to this config', + }; + const redirectedStage = { + key: 'zc', + alias: 'c', + description: + '(Redirected) Stage "za" does not actually exist, we want orca to run "a" but match "za" to this config instead of "a"', + }; + const actualStage = { + key: 'c', + description: 'Actual stage that redirected stage aliases to, this is what orca would actually run for "zc"', + }; + const titusStage = { + key: 'd', + provides: 'd', + cloudProvider: 'titus', + description: 'Titus implementation of "c" stage', + }; + const awsStage = { + key: 'd', + provides: 'd', + cloudProvider: 'aws', + description: 'Amazon implementation of "c" stage', + }; + + const slimmaker = [unmatchedStage, simpleStage, renamedStage, redirectedStage, actualStage, titusStage, awsStage]; + + it('matches stage.type with stageType.key', function() { + const pipelineRegistry = new PipelineRegistry(); + slimmaker.forEach(stage => pipelineRegistry.registerStage(stage)); + + expect(pipelineRegistry.getStageConfig({ type: 'a' } as IStage)).toEqual(simpleStage); + }); + + it('matches to "unmatched" stage when no matches are found', function() { + const pipelineRegistry = new PipelineRegistry(); + slimmaker.forEach(stage => pipelineRegistry.registerStage(stage)); + + expect(pipelineRegistry.getStageConfig({ type: 'x' } as IStage)).toEqual(unmatchedStage); + }); + + it('matches nothing (returns null) when "unmatched" stage was not registered', function() { + const pipelineRegistry = new PipelineRegistry(); + slimmaker.filter(stage => stage !== unmatchedStage).forEach(stage => pipelineRegistry.registerStage(stage)); + + expect(pipelineRegistry.getStageConfig({ type: 'x' } as IStage)).toEqual(null); + }); + + it('matches renamed stage with both stageType.key or (legacy) stageType.alias', function() { + const pipelineRegistry = new PipelineRegistry(); + slimmaker.forEach(stage => pipelineRegistry.registerStage(stage)); + + expect(pipelineRegistry.getStageConfig({ type: 'b' } as IStage)).toEqual(renamedStage); + expect(pipelineRegistry.getStageConfig({ type: 'z' } as IStage)).toEqual(renamedStage); + }); + + it('matches redirected stage.type with stageType.key even when stageType.alias collides with stageType.key of the actual stage', function() { + const pipelineRegistry = new PipelineRegistry(); + slimmaker.forEach(stage => pipelineRegistry.registerStage(stage)); + + expect(pipelineRegistry.getStageConfig({ type: 'zc' } as IStage)).toEqual(redirectedStage); + }); + + it('matches redirected stage.alias to the actual stage as a fallback when stage.type cannot be matched to a stageType.key (gracefully degrade to the underlying type)', function() { + const pipelineRegistry = new PipelineRegistry(); + slimmaker.filter(stage => stage !== redirectedStage).forEach(stage => pipelineRegistry.registerStage(stage)); + + expect(pipelineRegistry.getStageConfig({ type: 'zc', alias: 'c' } as IStage)).toEqual(actualStage); + }); + + it('matches redirect targets to ensure the actual stages do not get broken simply by having other stages alias to them', function() { + const pipelineRegistry = new PipelineRegistry(); + slimmaker.forEach(stage => pipelineRegistry.registerStage(stage)); + + expect(pipelineRegistry.getStageConfig({ type: 'c' } as IStage)).toEqual(actualStage); + }); + + it('matches provided stages to their cloudProvider specific stages', function() { + const pipelineRegistry = new PipelineRegistry(); + slimmaker.forEach(stage => pipelineRegistry.registerStage(stage)); + + expect(pipelineRegistry.getStageConfig(({ type: 'd', cloudProvider: 'titus' } as unknown) as IStage)).toEqual( + titusStage, + ); + expect(pipelineRegistry.getStageConfig(({ type: 'd', cloudProvider: 'aws' } as unknown) as IStage)).toEqual( + awsStage, + ); + expect(pipelineRegistry.getStageConfig({ type: 'd' } as IStage)).toEqual(awsStage); + }); + }); + describe('stage type retrieval', function() { describe('no provider configured', function() { it( diff --git a/app/scripts/modules/core/src/pipeline/config/PipelineRegistry.ts b/app/scripts/modules/core/src/pipeline/config/PipelineRegistry.ts index fea7a6c6387..a97b2f49b3c 100644 --- a/app/scripts/modules/core/src/pipeline/config/PipelineRegistry.ts +++ b/app/scripts/modules/core/src/pipeline/config/PipelineRegistry.ts @@ -219,16 +219,53 @@ export class PipelineRegistry { } } + /** + * Checks stage.type against stageType.alias to match stages that may have run as a legacy type. + * StageTypes set alias='legacyName' for backwards compatibility + * @param stage + */ + private checkAliasedStageTypes(stage: IStage): IStageTypeConfig { + const aliasedMatches = this.getStageTypes().filter(stageType => stageType.alias === stage.type); + if (aliasedMatches.length === 1) { + return aliasedMatches[0]; + } + return null; + } + + /** + * Checks stage.alias against stageType.key to gracefully degrade redirected stages + * For stages that don't actually exist in orca, if we couldn't find a match for them in deck either + * (i.e. deprecated/deleted) this allows us to fallback to the stage type that actually ran in orca + * @param stage + */ + private checkAliasFallback(stage: IStage): IStageTypeConfig { + if (stage.alias) { + // Allow fallback to an exact match with stage.alias + const aliasMatches = this.getStageTypes().filter(stageType => stageType.key === stage.alias); + if (aliasMatches.length === 1) { + return aliasMatches[0]; + } + } + return null; + } + public getStageConfig(stage: IStage): IStageTypeConfig { if (!stage || !stage.type) { return null; } const matches = this.getStageTypes().filter(stageType => { - return stageType.key === stage.type || stageType.provides === stage.type || stageType.alias === stage.type; + return stageType.key === stage.type || stageType.provides === stage.type; }); switch (matches.length) { case 0: + // There are really only 2 usages for 'alias': + // - to allow deck to still find a match for legacy stage types + // - to have stages that actually run as their 'alias' in orca (addAliasToConfig) because their 'key' doesn't actually exist + const aliasMatch = this.checkAliasedStageTypes(stage) || this.checkAliasFallback(stage); + if (aliasMatch) { + return aliasMatch; + } return this.getStageTypes().find(s => s.key === 'unmatched') || null; case 1: return matches[0]; diff --git a/app/scripts/modules/core/src/pipeline/service/ExecutionsTransformer.ts b/app/scripts/modules/core/src/pipeline/service/ExecutionsTransformer.ts index 6ad642251ef..35136e3cd22 100644 --- a/app/scripts/modules/core/src/pipeline/service/ExecutionsTransformer.ts +++ b/app/scripts/modules/core/src/pipeline/service/ExecutionsTransformer.ts @@ -312,6 +312,10 @@ export class ExecutionsTransformer { } const context = stage.context || {}; stage.cloudProvider = context.cloudProvider || context.cloudProviderType; + + if (context.alias) { + stage.alias = context.alias; + } }); OrchestratedItemTransformer.defineProperties(execution);