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

feat(codepipeline): add construct for registering custom Actions #17041

Merged
merged 11 commits into from Nov 4, 2021
Merged

feat(codepipeline): add construct for registering custom Actions #17041

merged 11 commits into from Nov 4, 2021

Conversation

rayjanwilson
Copy link
Contributor

fixes #17039

all it does is add ./common and ./custom-action-registration to the modules index.ts


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

@gitpod-io
Copy link

gitpod-io bot commented Oct 18, 2021

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @rayjanwilson, but things are not that simple (aren't they always 😜).

We will need to make some changes to custom-action-registration.ts - the build currently fails with:

@aws-cdk/aws-codepipeline-actions: �[96mlib/custom-action-registration.ts�[0m:�[93m105�[0m:�[93m3�[0m - �[91merror�[0m�[90m JSII3008: �[0mThe "actionProperties" property of struct "@aws-cdk/aws-codepipeline-actions.CustomActionRegistrationProps" must be "readonly". Rename "@aws-cdk/aws-codepipeline-actions.CustomActionRegistrationProps" to "ICustomActionRegistrationProps" if it is meant to be a behavioral interface.

We need to make all of the properties of CustomActionProperty readonly. While we're in the area, we'll probably have to make a few more adjustments before we make this a public part of the API of the module (and hence we'll have to maintain it going forwards).

Are you up for that @rayjanwilson? 🙂

Thanks,
Adam

@@ -18,3 +18,5 @@ export * from './s3/source-action';
export * from './stepfunctions/invoke-action';
export * from './servicecatalog/deploy-action-beta1';
export * from './action';
export * from './common';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the export of common? Why do you need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh just figured it helped when making custom source actions

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 it out then please 🙂.

@mergify mergify bot dismissed skinny85’s stale review October 19, 2021 21:26

Pull request has been modified.

@rayjanwilson
Copy link
Contributor Author

We need to make all of the properties of CustomActionProperty readonly. While we're in the area, we'll probably have to make a few more adjustments before we make this a public part of the API of the module (and hence we'll have to maintain it going forwards).

Are you up for that @rayjanwilson? 🙂

yep happy to give it a go and iterate towards what it should be. first time doing this =)

@skinny85
Copy link
Contributor

We need to make all of the properties of CustomActionProperty readonly. While we're in the area, we'll probably have to make a few more adjustments before we make this a public part of the API of the module (and hence we'll have to maintain it going forwards).
Are you up for that @rayjanwilson? 🙂

yep happy to give it a go and iterate towards what it should be. first time doing this =)

Sweet. Let's start by adding those readonly to the properties, and we'll take it from there 🙂.

@rayjanwilson
Copy link
Contributor Author

removed common and changed CustomActionProperty to be readonly

@skinny85
Copy link
Contributor

@rayjanwilson Now you need to do the same for CustomActionRegistrationProps 😜.

@aws-cdk/aws-codepipeline-actions: �[96mlib/custom-action-registration.ts�[0m:�[93m98�[0m:�[93m3�[0m - �[91merror�[0m�[90m JSII3008: �[0mThe "executionUrl" property of struct "@aws-cdk/aws-codepipeline-actions.CustomActionRegistrationProps" must be "readonly". Rename "@aws-cdk/aws-codepipeline-actions.CustomActionRegistrationProps" to "ICustomActionRegistrationProps" if it is meant to be a behavioral interface.

@rayjanwilson
Copy link
Contributor Author

oh, derp. fixed

@skinny85
Copy link
Contributor

Still some errors:

@aws-cdk/aws-codepipeline-actions: error: [awslint:construct-ctor:@aws-cdk/aws-codepipeline-actions.CustomActionRegistration.<initializer>.params[0]] signature of all construct constructors should be "scope, id, props".   (expected="scope",actual="parent")
@aws-cdk/aws-codepipeline-actions: error: [awslint:props-default-doc:@aws-cdk/aws-codepipeline-actions.CustomActionRegistrationProps.entityUrl] Optional property must have @default documentation 
@aws-cdk/aws-codepipeline-actions: error: [awslint:props-default-doc:@aws-cdk/aws-codepipeline-actions.CustomActionRegistrationProps.executionUrl] Optional property must have @default documentation 
@aws-cdk/aws-codepipeline-actions: Error: /codebuild/output/src259442693/src/github.com/aws/aws-cdk/tools/@aws-cdk/cdk-build-tools/bin/cdk-awslint exited with error code 1

@rayjanwilson did you go through our "Contributing" guide? You should be able to see these errors when building locally, no need to wait for the PR build 🙂.

@rayjanwilson
Copy link
Contributor Author

i'll go through it again

@rayjanwilson
Copy link
Contributor Author

k, i had to flush all the old docker images and volumes. that was causing the devcontainer to crash when doing the post-create command. it's building now and i'll be able to go through the contributing section correctly now. cheers

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @rayjanwilson! Let's add some JSDocs to the fileds, and let's get this merged in!

Would you mind also updating the ReadMe of the module, showing an example of how to use this class?

@@ -172,7 +172,9 @@
"docs-public-apis:@aws-cdk/aws-codepipeline-actions.GitHubTrigger.POLL",
"docs-public-apis:@aws-cdk/aws-codepipeline-actions.GitHubTrigger.WEBHOOK",
"docs-public-apis:@aws-cdk/aws-codepipeline-actions.CodeDeployEcsDeployAction",
"props-default-doc:@aws-cdk/aws-codepipeline-actions.CodeDeployEcsDeployActionProps.containerImageInputs"
"props-default-doc:@aws-cdk/aws-codepipeline-actions.CodeDeployEcsDeployActionProps.containerImageInputs",
"props-default-doc:@aws-cdk/aws-codepipeline-actions.CustomActionRegistrationProps.entityUrl",
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding an exemption here, let's actually fill out this documentation:

  /**
   * The URL shown for the entire Action in the Pipeline UI.
   *
   * @default - none
   */
  readonly entityUrl?: string;

"props-default-doc:@aws-cdk/aws-codepipeline-actions.CodeDeployEcsDeployActionProps.containerImageInputs"
"props-default-doc:@aws-cdk/aws-codepipeline-actions.CodeDeployEcsDeployActionProps.containerImageInputs",
"props-default-doc:@aws-cdk/aws-codepipeline-actions.CustomActionRegistrationProps.entityUrl",
"props-default-doc:@aws-cdk/aws-codepipeline-actions.CustomActionRegistrationProps.executionUrl"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here:

  /**
   * The URL shown for a particular execution of an Action in the Pipeline UI.
   *
   * @default - none
   */
  readonly executionUrl?: string;

@skinny85 skinny85 changed the title fix(aws-codepipeline-actions): include missing exports with index.ts fix(codepipeline): add a CustomActionRegistration construct Oct 22, 2021
@github-actions github-actions bot added the @aws-cdk/aws-codepipeline Related to AWS CodePipeline label Oct 22, 2021
@skinny85
Copy link
Contributor

Actually @rayjanwilson, since you're doing work in this are anyway... Can you please move these classes to the @aws-cdk/aws-codepipeline module? I think they make much more sense there (the ReadMe changes should also be to that module's file).

@skinny85 skinny85 changed the title fix(codepipeline): add a CustomActionRegistration construct feat(codepipeline): add a CustomActionRegistration construct Oct 22, 2021
@rayjanwilson
Copy link
Contributor Author

yes for sure

@skinny85
Copy link
Contributor

Thanks!

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Putting in "Request changes" to clear this one from my To-Do list. @rayjanwilson please re-request my review when this is ready!

@mergify mergify bot dismissed skinny85’s stale review October 29, 2021 15:07

Pull request has been modified.

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Thanks for the effort @rayjanwilson, but, like I mentioned before, let's leave common.ts exactly where it is now, unchanged.

@@ -1,10 +1,10 @@
import * as codepipeline from '@aws-cdk/aws-codepipeline';
import { ActionArtifactBounds } from './action';

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we resolved this already? 😃

Do not touch this file - leave it where it is, don't move it to the @aws-cdk/aws-codepipeline module, and do not make it public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we had, my bad. it's what i get for picking back up after a week or so. apologies. reverted

@mergify mergify bot dismissed skinny85’s stale review November 4, 2021 02:01

Pull request has been modified.

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @rayjanwilson!

@@ -109,6 +109,40 @@ or you can use the `IStage.addAction()` method to mutate an existing Stage:
sourceStage.addAction(someAction);
```

## Custom Action Registration

To make your own custom CodePipeline Action requires registering the action provider. Look to the `JenkinsProvider` in `@aws-cdk/aws-codepipeline-actions` for an implementation example.
Copy link
Contributor

Choose a reason for hiding this comment

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

Be wary of long lines in this file:

Suggested change
To make your own custom CodePipeline Action requires registering the action provider. Look to the `JenkinsProvider` in `@aws-cdk/aws-codepipeline-actions` for an implementation example.
To make your own custom CodePipeline Action requires registering the action provider.
Look to the `JenkinsProvider` in `@aws-cdk/aws-codepipeline-actions` for an implementation example.

@skinny85 skinny85 changed the title feat(codepipeline): add a CustomActionRegistration construct feat(codepipeline): add construct for registering custom Actions Nov 4, 2021
@skinny85 skinny85 added the pr-linter/exempt-test The PR linter will not require test changes label Nov 4, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 4, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 12d7fb6
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit c66ac89 into aws:master Nov 4, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 4, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…#17041)

fixes aws#17039

all it does is add `./common` and `./custom-action-registration` to the modules `index.ts`

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-codepipeline Related to AWS CodePipeline @aws-cdk/aws-codepipeline-actions pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-codepipeline-actions): custom-action-registration and common are not exported in index.ts
4 participants