Skip to content

Commit

Permalink
fix(pipelines): graphnode dependencies can have duplicates (aws#18450)
Browse files Browse the repository at this point in the history
`GraphNode.allDeps` allows duplicate dependencies to be returned. This does not have any affect on the performance of the pipelines module, but looks ugly. This was noticed in cdklabs/cdk-pipelines-github#67, where the dependencies are written out to the `deploy.yaml` file. 

I did not change the underlying `GraphNode.dependencies` structure to be a set (although I think it should) because I feel like that is a breaking change. So instead I've preserved the structure of the API and deduplicated the list of GraphNodes.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
kaizencc authored and TikiTDO committed Feb 21, 2022
1 parent f49bcf3 commit d3b6c4d
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 2 deletions.
4 changes: 2 additions & 2 deletions packages/@aws-cdk/pipelines/lib/helpers-internal/graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export class GraphNode<A> {
*/
public get allDeps(): GraphNode<A>[] {
const fromParent = this.parentGraph?.allDeps ?? [];
return [...this.dependencies, ...fromParent];
return Array.from(new Set([...this.dependencies, ...fromParent]));
}

public dependOn(...dependencies: Array<GraphNode<A> | undefined>) {
Expand Down Expand Up @@ -382,4 +382,4 @@ function projectDependencies<A>(dependencies: Map<GraphNode<A>, Set<GraphNode<A>

export function isGraph<A>(x: GraphNode<A>): x is Graph<A> {
return x instanceof Graph;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,22 @@ describe('with nested graphs', () => {
]);
});
});

test('duplicate dependencies are ignored', () => {
mkGraph('G', G => {
const A = G.graph('A', [], GA => {
GA.node('aa');
});

// parent graph depnds on A also
const B = G.graph('B', [A], GB => {
// duplicate dependency on A
GB.graph('BB', [A], GBB => {
GBB.node('bbb');
});
GB.node('bb');
});

expect(nodeNames(B.tryGetChild('BB')!.allDeps)).toStrictEqual(['A']);
});
});

0 comments on commit d3b6c4d

Please sign in to comment.