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

aws-cdk-codepipeline: InvokeLambdaAction #29

Merged
merged 3 commits into from
Jun 5, 2018

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Jun 5, 2018

  1. Accepts a LambdaRef instead of function name
  2. Add lambda:ListFunctions and lambda:InvokeFunction to pipeline role.
  3. Add codepipeline:PutJobXxxResult to Lambda role.

In order to avoid the cyclic dependency:

Pipeline => Lambda
Lambda => Role
Role => Pipeline

We follow the recommendation in the CodePipeline docs
and use a "*" resource for codepipeline:PutJobXxxResult
for the Lambda execution policy.

BREAKING CHANGE:

  • The DefaultBounds function is no longer exported.
  • GitHubSourceProps.oathToken spelling fixed to oauthToken
  • BuildAction now takes an inputArtifact instead of source (allows
    using these actions for arbitrary activity within a pipeline).
  • InvokeLambdaProps takes lambda instead of functionName.
  • InvokeLambda renamed to InvokeLambdaAction

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

1. Accepts a `LambdaRef` instead of function name
2. Add `lambda:ListFunctions` and `lambda:InvokeFunction` to pipeline role.
3. Add `codepipeline:PutJobXxxResult` to Lambda role.

In order to avoid the cyclic dependency:

    Pipeline => Lambda
    Lambda => Role
    Role => Pipeline

We follow the recommendation in the [CodePipeline docs][1]
and use a "*" resource for  `codepipeline:PutJobXxxResult`
for the Lambda execution policy.

[1]: https://docs.aws.amazon.com/codepipeline/latest/userguide/actions-invoke-lambda-function.html

BREAKING CHANGE:

- The `DefaultBounds` function is no longer exported.
- `GitHubSourceProps.oathToken` spelling fixed to `oauthToken`
- `BuildAction` now takes an `inputArtifact` instead of `source` (allows
    using these actions for arbitrary activity within a pipeline).
- `InvokeLambdaProps` takes `lambda` instead of `functionName`.
- `InvokeLambda` renamed to `InvokeLambdaAction`
* the pipeline.
*
* @see
* https://docs.aws.amazon.com/codepipeline/latest/userguide/actions-invoke-lambda-function.html#actions-invoke-lambda-function-create-function
Copy link
Contributor

Choose a reason for hiding this comment

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

Would put the URL on the same line as the @see.

Copy link
Contributor Author

@eladb eladb Jun 5, 2018

Choose a reason for hiding this comment

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

I get yelled at by tslint 😢 and I don't like to constantly disable those rules.

* @see
* https://docs.aws.amazon.com/codepipeline/latest/userguide/actions-invoke-lambda-function.html#actions-invoke-lambda-function-create-function
*
* @default true
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a though -- that default is slightly awkward (because undefined is falsey). But whatever.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's perfectly fine to have defaults that default to true. At least, better than negative keywords (dontAddPutJobResultPolicy? ugh)

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely. Hence the whatever.

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, @rix0rrr convinced me of this when we formalized the design guidelines, and I am convert now.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 5, 2018

In order to avoid the cyclic dependency

Don't think that is the reason. We don't actually have a cyclic dependency, because we'd just use an IAM::Policy resource, making the dependency chain:

  +----------------------------------------------+
  |                                              v
+--------+     +----------+     +--------+     +------------+
| Policy | --> | Pipeline | --> | Lambda | --> | LambdaRole |
+--------+     +----------+     +--------+     +------------+

I think the reason is just that CodePipeline doesn't actually support ARNs in IAM policies! 🙀

@eladb
Copy link
Contributor Author

eladb commented Jun 5, 2018

@rix0rrr good point about the cyclic dependency. Let me re-check if there's something I missed.

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.

None yet

4 participants