-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat: re-factor the CodePipeline Action bind
method
#2085
Conversation
There was a problem hiding this 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?
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. |
Sure. What are your thoughts? |
7c48e82
to
7416e9f
Compare
Rebased on top of newest |
There was a problem hiding this 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
.
7416e9f
to
4e9d39c
Compare
…o take a Role separately from the Pipeline.
4e9d39c
to
5f138fd
Compare
Rebased, and re-named |
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
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.