Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the Stage insertion API in CodePipeline more flexible #460

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Aug 1, 2018

Allows inserting Stages into CodePipeline at an arbitrary index, and also before or after any given Stage.


Fixes #347


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@skinny85
Copy link
Contributor Author

skinny85 commented Aug 6, 2018

@eladb ping (you've opened the original issue, after all ;) )

@skinny85 skinny85 requested a review from eladb August 6, 2018 17:33
@eladb
Copy link
Contributor

eladb commented Aug 7, 2018

@skinny85 sorry! I provided some feedback the previous PR but forgot to "submit feedback". Copying the notes to this PR now. Apologies...

/**
* Get the number of Stages in this Pipeline.
*/
public get stagesLength(): number {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stageCount

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@@ -216,23 +230,71 @@ export class Pipeline extends cdk.Construct implements events.IEventRuleTarget {
* onChildAdded type hook.
* @override
*/
protected addChild(child: cdk.Construct, name: string) {
super.addChild(child, name);
protected addChild(child: cdk.Construct, name: string, props?: any) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think this change is needed and we are extremely sensitive to extensions of the surface area of Construct.

I see why you need to propagate props to addChild but that’s not a robust way to do this. What promises you that everyone will pass in their props to Construct?

I think the way to address this is to render the stages array only during synthesis (using a Token). Then, you wouldn’t even need to override addChild at all. When you synthesize, you will iterate over your children and devise the list of stages according to their placement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in Construct are not needed anymore after #568.

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Outdated Show resolved Hide resolved
* @see #justAfterStage
* @see #atIndex
*/
export interface StagePlacement {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I think the parent/child semantics make total sense here. Why not just “before” and “after”? What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a matter of wording. I wanted to make it clear that inserting a Stage before/after a given Stage will "displace" any stage that was before/after that Stage there before (see how clunky this explanation is?), and that's why I came up with the child/parent wording. I'm open to suggestions on how to improve those docs, of course.

*/
constructor(parent: Construct, name: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change to Construct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said above, not needed anymore.

@skinny85 skinny85 force-pushed the feature/pipeline-insert-stage branch from a496315 to 1e41ed3 Compare August 15, 2018 00:41
@skinny85 skinny85 changed the base branch from master to adamruka/stage-gets-pipeline-in-props August 15, 2018 00:41
@skinny85
Copy link
Contributor Author

Well then. After #568, this change got a whole lot easier.

@skinny85
Copy link
Contributor Author

@eladb ping :)

@skinny85 skinny85 force-pushed the feature/pipeline-insert-stage branch from 1e41ed3 to b257d46 Compare August 15, 2018 23:19
@skinny85 skinny85 changed the base branch from adamruka/stage-gets-pipeline-in-props to master August 15, 2018 23:19
@skinny85
Copy link
Contributor Author

Rebased on top of #568 which was pushed and changed the PR base branch to master.

/**
* Get a duplicate of this Pipeline's list of Stages.
*/
public get stages(): Stage[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed? (Keep surface area to minimum)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it might be nice for more complicated Stage-inserting scenarios, but I can remove it if you don't like it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this out for now unless there's a clear use case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Will remove.

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Outdated Show resolved Hide resolved
*
* @default the stage is added at the end of the Pipeline
*/
placed?: StagePlacement;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this placement. Our properties are usually nouns and not verbs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I named it so that the code reads a little better (new Stage(this, 'Stage', { placed: { atIndex: 3 }})), but I can change it to placement if you prefer that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, properties are usually nouns and not verbs. Just trying to keep our semantic style uniform

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix.

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the couple of remaining items and merge

/**
* Get a duplicate of this Pipeline's list of Stages.
*/
public get stages(): Stage[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this out for now unless there's a clear use case

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Outdated Show resolved Hide resolved
*
* @default the stage is added at the end of the Pipeline
*/
placed?: StagePlacement;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, properties are usually nouns and not verbs. Just trying to keep our semantic style uniform

@eladb
Copy link
Contributor

eladb commented Sep 16, 2018

Any updates on this?

@skinny85 skinny85 force-pushed the feature/pipeline-insert-stage branch from b257d46 to 92ecac4 Compare September 17, 2018 21:19
@skinny85
Copy link
Contributor Author

Any updates on this?

Incorporated feedback from your last review, and rebased.

```ts
const sourceStage = new Stage(this, 'Source', {
pipeline,
placement: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also allow people to optionally specify placement in addStage? It would be nicer if this used addStage instead of new Stage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we have to merge either this, or the addStage PR first (they're both based on master). Once I do that, I'll rebase the other one on top of the new master, and edit accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Both are approved.

*/
public _addStage(stage: Stage): void {
public _addStage(stage: Stage, placement?: StagePlacement): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a way to completely hide this method, which is soon going to be very confusing with addStage: you can make it private (I'd call it something else, like: _attachStage) and when invoking it from Stage, you can invoke it by casing to any:

(pipeline as any)._attachStage(this, placement)

Not ideal, but it will hide it from the public API, which is what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

…more flexible.

This commit allows clients of CodePipeline to create new Stages placed
at an arbitrary index in the Pipeline, or before/after a given Stage
(instead of only appending new Stages at the end).
@skinny85 skinny85 force-pushed the feature/pipeline-insert-stage branch from 92ecac4 to 4830254 Compare September 17, 2018 23:35
@skinny85
Copy link
Contributor Author

Rebased on top of the merged Pipeline#addStage PR, incorporating the latest feedback.

@skinny85 skinny85 merged commit d182818 into aws:master Sep 18, 2018
@skinny85 skinny85 deleted the feature/pipeline-insert-stage branch September 18, 2018 17:27
eladb pushed a commit that referenced this pull request Sep 20, 2018
__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
eladb pushed a commit that referenced this pull request Sep 20, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* v0.9.2

__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

codepipeline: There should be a way to insert a stage before an existing one (and not append)
3 participants