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: re-factor the CodePipeline Action bind method #2085

Merged

Conversation

skinny85
Copy link
Contributor

The role it gets passed is no longer directly tied to the Pipeline
(although it still might be the Pipeline Role, behind the scenes).
That's needed for some more advanced use-cases, like cross-account Pipelines.

Changed to also have the method accept an interface,
to facilitate future evolution.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@skinny85 skinny85 requested a review from eladb March 23, 2019 00:09
@skinny85 skinny85 requested review from RomainMuller and a team as code owners March 23, 2019 00:09
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.

I am not sure I understand if this makes it easier or harder to implement custom actions (see #2073), which I believe we should address. Maybe we should think about those two changes together?

@skinny85
Copy link
Contributor Author

I am not sure I understand if this makes it easier or harder to implement custom actions (see #2073), which I believe we should address. Maybe we should think about those two changes together?

It doesn't affect the custom actions at all - it's completely unrelated. I'd rather not combine these two changes.

@eladb
Copy link
Contributor

eladb commented Mar 25, 2019

It doesn't affect the custom actions at all - it's completely unrelated. I'd rather not combine these two changes.

I am okay not combining, but I'd like to discuss the full API before we proceed here.

@skinny85
Copy link
Contributor Author

I am okay not combining, but I'd like to discuss the full API before we proceed here.

Sure. What are your thoughts?

@skinny85 skinny85 force-pushed the feature/pipeline-actions-bind-api-refactor branch from 7c48e82 to 7416e9f Compare March 26, 2019 20:26
@skinny85
Copy link
Contributor Author

Rebased on top of newest master.

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.

Approved, conditional of converting (basically just renaming) IBindInfo to BindInfo. Perhaps rename to just ActionBind.

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts Outdated Show resolved Hide resolved
@skinny85 skinny85 force-pushed the feature/pipeline-actions-bind-api-refactor branch from 7416e9f to 4e9d39c Compare March 28, 2019 22:57
…o take a Role separately from the Pipeline.
@skinny85 skinny85 force-pushed the feature/pipeline-actions-bind-api-refactor branch from 4e9d39c to 5f138fd Compare March 28, 2019 23:08
@skinny85
Copy link
Contributor Author

skinny85 commented Mar 28, 2019

Rebased, and re-named IBindInfo to ActionBind.

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.

4 participants