-
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
[BREAKING] Align the S3 CodePipeline Action with other Actions #492
[BREAKING] Align the S3 CodePipeline Action with other Actions #492
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 wasn't there for the previous discussion on this, and 👍on consistency, but are we sure where this is going is this still simple and convenient to use?
I'm not sure, but I hope so... Do you have any specific concerns @rix0rrr ? |
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.
General advise: try to shorten the titles of your PR descriptions 😉
import cdk = require('@aws-cdk/cdk'); | ||
import lambda_codepipeline = require('../lib'); | ||
import lambdaCodepipeline = require('../lib'); |
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.
Got to say that the camelCase syntax for these is really hurting my eyes (I know I was the one recommending it), but I am having seconds thoughts 😔
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 don't think it's that bad... if you look at how the full Action name is used: lambdaCodePipeline.InvokeAction
, it looks fine to me. Imagine the Actions were all in the codepipeline
module: in that case, the name would be something like codepipeline.LambdaInvokeAction
, which is pretty much identical to lambdaCodePipeline.InvokeAction
(certainly follows the same style, and conveys the same information).
@@ -0,0 +1,51 @@ | |||
import codepipeline = require('@aws-cdk/aws-codepipeline'); |
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.
This should be called source-action.ts
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.
That's interesting... you've never made that comment for the Actions in the different services in #459 . The reason for naming it this way is, in some cases (CodeBuild), there will be more than 1 Pipeline Action in the same module, and I wanted to put them in the same file, so the name pipeline-actions.ts
made sense to me. Do you disagree, and rather put them always in separate files? In that case, I can update #459 .
*/ | ||
bucketKey: string; | ||
|
||
// TODO: use CloudWatch events instead |
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.
We have support for CloudWatch Events. Should we open an issue to set those up across pipeline actions?
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 actually don't know - I'm not super familiar with the CloudWatch Events support, but I believe the current solution works for the entire Pipeline, and the individual Actions have their own Events needs (based on what service they integrate with).
Just the fact that people are going to have to import approximately 200 packages to use the CodePipeline features (hyperbole for effect). I understand why it's being done this way, but it still makes me unhappy. |
Well then, you'll be glad to hear we've actually changed our mind on the API, and will be going back to having the actions inside their service modules :). I'm working on a PR right now. |
52e9d8f
to
4f446fd
Compare
Rebased on top of #459 - I assume the build will fail for the same reasons it fails in that PR. |
4f446fd
to
5013211
Compare
Changed the base back to |
…with other Actions. BREAKING CHANGE: move the S3 CodePipeline Action out of the aws-codepipeline package and into the aws-s3 package, and rename it from AmazonS3Source to PipelineSource.
5013211
to
2f7eeb2
Compare
Rebased on top of the 0.8.2 release. |
The headliners of this release are __.NET support__, and a wealth of commits by external contributors who are stepping up to fix the CDK for their use cases! Thanks all for the effort put into this release! * Add strongly-named .NET targets, and a `cdk init` template for C# projects ([@mpiroc] in [#617](#617), [#643](#643)). * __@aws-cdk/aws-autoscaling__: Allow attaching additional security groups to Launch Configuration ([@moofish32] in [#636](#636)). * __@aws-cdk/aws-autoscaling__: Support update and creation policies on AutoScalingGroups ([@rix0rrr] in [#595](#595)). * __@aws-cdk/aws-codebuild__: Add support for running script from an asset ([@rix0rrr] in [#677](#677)). * __@aws-cdk/aws-codebuild__: New method `addBuildToPipeline` on Project ([@skinny85] in [783dcb3](783dcb3)). * __@aws-cdk/aws-codecommit__: New method `addToPipeline` on Repository ([@skinny85] in [#616](#616)). * __@aws-cdk/aws-codedeploy__: Add initial support for CodeDeploy ([@skinny85] in [#593](#593), [#641](#641)). * __@aws-cdk/aws-dynamodb__: Add support for DynamoDB autoscaling ([@SeekerWing] in [#637](#637)). * __@aws-cdk/aws-dynamodb__: Add support for DynamoDB streams ([@rhboyd] in [#633](#633)). * __@aws-cdk/aws-dynamodb__: Add support for server-side encryption ([@jungseoklee] in [#684](#864)). * __@aws-cdk/aws-ec2__ (_**BREAKING**_): SecurityGroup can now be used as a Connectable [#582](#582)). * __@aws-cdk/aws-ec2__: Add VPC tagging ([@moofish] in [#538](#538)). * __@aws-cdk/aws-ec2__: Add support for `InstanceSize.Nano` ([@rix0rrr] in [#581](#581)) * __@aws-cdk/aws-lambda__: Add support for dead letter queues ([@SeekerWing] in [#663](#663)). * __@aws-cdk/aws-lambda__: Add support for placing a Lambda in a VPC ([@rix0rrr] in [#598](#598)). * __@aws-cdk/aws-logs__: Add `extractMetric()` helper function ([@rix0rrr] in [#676](#676)). * __@aws-cdk/aws-rds__: Add support for Aurora PostreSQL/MySQL engines ([@cookejames] in [#586](#586)) * __@aws-cdk/aws-s3__: Additional grant methods for Buckets ([@eladb] in [#591](#591)) * __@aws-cdk/aws-s3__: New method `addToPipeline` on Bucket ([@skinny85] in [c8b7a49](c8b7a49)). * __aws-cdk__: Add support for HTTP proxies ([@rix0rrr] in [#666](#666)). * __aws-cdk__: Toolkit now shows failure reason if stack update fails ([@rix0rrr] in [#609](#609)). * __cdk-build-tools__: Add support for running experiment JSII versions ([@RomainMuller] in [#649](#649)). * _**BREAKING**_: Generate classes and types for the CloudFormation resource `.ref` attributes ([@rix0rrr] in [#627](#627)). * _**BREAKING**_: Make types accepted in Policy-related classes narrower (from `any` to `Arn`, for example) to reduce typing mistakes ([@rix0rrr] in [#629](#629)). * __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): Align the CodePipeline APIs ([@skinny85] in [#492](#492), [#568](#568)) * __@aws-cdk/aws-ec2__ (_**BREAKING**_): Move Fleet/AutoScalingGroup to its own package ([@rix0rrr] in [#608](#608)). * __aws-cdk__: Simplify plugin protocol ([@RomainMuller] in [#646](#646)). * __@aws-cdk/aws-cloudfront__: Fix CloudFront behavior for ViewerProtocolPolicy ([@mindstorms6] in [#615](#615)). * __@aws-cdk/aws-ec2__: VPC Placement now supports picking Isolated subnets ([@rix0rrr] in [#610](#610)). * __@aws-cdk/aws-logs__: Add `export()/import()` capabilities ([@rix0rrr] in [#630](#630)). * __@aws-cdk/aws-rds__: Fix a bug where a cluster with 1 instance could not be created ([@cookejames] in [#578](#578)) * __@aws-cdk/aws-s3__: Bucket notifications can now add dependencies, fixing creation order ([@eladb] in [#584](#584)). * __@aws-cdk/aws-s3__: Remove useless bucket name validation ([@rix0rrr] in [#628](#628)). * __@aws-cdk/aws-sqs__: Make `QueueRef.encryptionMasterKey` readonly ([@RomainMuller] in [#650](#650)). * __assets__: S3 read permissions are granted on a prefix to fix lost permissions during asset update ([@rix0rrr] in [#510](#510)). * __aws-cdk__: Remove bootstrapping error if multiple stacks are in the same environment ([@RomainMuller] in [#625](#625)). * __aws-cdk__: Report and continue if git throws errors during `cdk init` ([@rix0rrr] in [#587](#587)). * __@aws-cdk/cfnspec__: Updated [CloudFormation resource specification] to `v2.6.0` ([@RomainMuller] in [#594](#594)) + **New AWS Construct Library** - `@aws-cdk/aws-sagemaker` supports AWS::SageMaker resources + **New Resource Types** - AWS::AmazonMQ::Broker - AWS::AmazonMQ::Configuration - AWS::CodePipeline::Webhook - AWS::Config::AggregationAuthorization - AWS::Config::ConfigurationAggregator - AWS::EC2::VPCEndpointConnectionNotification - AWS::EC2::VPCEndpointServicePermissions - AWS::IAM::ServiceLinkedRole - AWS::SSM::ResourceDataSync - AWS::SageMaker::Endpoint - AWS::SageMaker::EndpointConfig - AWS::SageMaker::Model - AWS::SageMaker::NotebookInstance - AWS::SageMaker::NotebookInstanceLifecycleConfig + **Attribute Changes** - AWS::CodePipeline::Pipeline Version (__added__) + **Property Changes** - AWS::AppSync::DataSource HttpConfig (__added__) - AWS::DAX::Cluster SSESpecification (__added__) - AWS::DynamoDB::Table Stream (__added__) - AWS::DynamoDB::Table AutoScalingSupport (__added__) - AWS::EC2::VPCEndpoint IsPrivateDnsEnabled (__added__) - AWS::EC2::VPCEndpoint SecurityGroupIds (__added__) - AWS::EC2::VPCEndpoint SubnetIds (__added__) - AWS::EC2::VPCEndpoint VPCEndpointType (__added__) - AWS::EC2::VPCEndpoint RouteTableIds.DuplicatesAllowed (__deleted__) - AWS::EC2::VPCPeeringConnection PeerRegion (__added__) - AWS::EFS::FileSystem ProvisionedThroughputInMibps (__added__) - AWS::EFS::FileSystem ThroughputMode (__added__) - AWS::EMR::Cluster KerberosAttributes (__added__) - AWS::Glue::Classifier JsonClassifier (__added__) - AWS::Glue::Classifier XMLClassifier (__added__) - AWS::Glue::Crawler Configuration (__added__) - AWS::Lambda::Lambda DLQConfigurationSupport (__added__) - AWS::Neptune::DBInstance DBSubnetGroupName.UpdateType (__changed__) - Old: Mutable - New: Immutable - AWS::SNS::Subscription DeliveryPolicy (__added__) - AWS::SNS::Subscription FilterPolicy (__added__) - AWS::SNS::Subscription RawMessageDelivery (__added__) - AWS::SNS::Subscription Region (__added__) - AWS::SQS::Queue Tags (__added__) - AWS::ServiceDiscovery::Service HealthCheckCustomConfig (__added__) + **Property Type Changes** - AWS::AppSync::DataSource.HttpConfig (__added__) - AWS::DAX::Cluster.SSESpecification (__added__) - AWS::EMR::Cluster.KerberosAttributes (__added__) - AWS::Glue::Classifier.JsonClassifier (__added__) - AWS::Glue::Classifier.XMLClassifier (__added__) - AWS::ServiceDiscovery::Service.HealthCheckCustomConfig (__added__) - AWS::CloudFront::Distribution.CacheBehavior FieldLevelEncryptionId (__added__) - AWS::CloudFront::Distribution.DefaultCacheBehavior FieldLevelEncryptionId (__added__) - AWS::CodeBuild::Project.Artifacts EncryptionDisabled (__added__) - AWS::CodeBuild::Project.Artifacts OverrideArtifactName (__added__) - AWS::CodeBuild::Project.Environment Certificate (__added__) - AWS::CodeBuild::Project.Source ReportBuildStatus (__added__) - AWS::ServiceDiscovery::Service.DnsConfig RoutingPolicy (__added__) - AWS::WAF::WebACL.ActivatedRule Action.Required (__changed__) - Old: true - New: false * __@aws-cdk/cfnspec__: Updated Serverless Application Model (SAM) Resource Specification ([@RomainMuller] in [#594](#594)) + **Property Changes** - AWS::Serverless::Api MethodSettings (__added__) + **Property Type Changes** - AWS::Serverless::Function.SQSEvent (__added__) - AWS::Serverless::Function.EventSource Properties.Types (__changed__) - Added SQSEvent
The headliners of this release are __.NET support__, and a wealth of commits by external contributors who are stepping up to fix the CDK for their use cases! Thanks all for the effort put into this release! * Add strongly-named .NET targets, and a `cdk init` template for C# projects ([@mpiroc] in [#617](#617), [#643](#643)). * __@aws-cdk/aws-autoscaling__: Allow attaching additional security groups to Launch Configuration ([@moofish32] in [#636](#636)). * __@aws-cdk/aws-autoscaling__: Support update and creation policies on AutoScalingGroups ([@rix0rrr] in [#595](#595)). * __@aws-cdk/aws-codebuild__: Add support for running script from an asset ([@rix0rrr] in [#677](#677)). * __@aws-cdk/aws-codebuild__: New method `addBuildToPipeline` on Project ([@skinny85] in [783dcb3](783dcb3)). * __@aws-cdk/aws-codecommit__: New method `addToPipeline` on Repository ([@skinny85] in [#616](#616)). * __@aws-cdk/aws-codedeploy__: Add initial support for CodeDeploy ([@skinny85] in [#593](#593), [#641](#641)). * __@aws-cdk/aws-dynamodb__: Add support for DynamoDB autoscaling ([@SeekerWing] in [#637](#637)). * __@aws-cdk/aws-dynamodb__: Add support for DynamoDB streams ([@rhboyd] in [#633](#633)). * __@aws-cdk/aws-dynamodb__: Add support for server-side encryption ([@jungseoklee] in [#684](#864)). * __@aws-cdk/aws-ec2__ (_**BREAKING**_): SecurityGroup can now be used as a Connectable [#582](#582)). * __@aws-cdk/aws-ec2__: Add VPC tagging ([@moofish] in [#538](#538)). * __@aws-cdk/aws-ec2__: Add support for `InstanceSize.Nano` ([@rix0rrr] in [#581](#581)) * __@aws-cdk/aws-lambda__: Add support for dead letter queues ([@SeekerWing] in [#663](#663)). * __@aws-cdk/aws-lambda__: Add support for placing a Lambda in a VPC ([@rix0rrr] in [#598](#598)). * __@aws-cdk/aws-logs__: Add `extractMetric()` helper function ([@rix0rrr] in [#676](#676)). * __@aws-cdk/aws-rds__: Add support for Aurora PostreSQL/MySQL engines ([@cookejames] in [#586](#586)) * __@aws-cdk/aws-s3__: Additional grant methods for Buckets ([@eladb] in [#591](#591)) * __@aws-cdk/aws-s3__: New method `addToPipeline` on Bucket ([@skinny85] in [c8b7a49](c8b7a49)). * __aws-cdk__: Add support for HTTP proxies ([@rix0rrr] in [#666](#666)). * __aws-cdk__: Toolkit now shows failure reason if stack update fails ([@rix0rrr] in [#609](#609)). * __cdk-build-tools__: Add support for running experiment JSII versions ([@RomainMuller] in [#649](#649)). * _**BREAKING**_: Generate classes and types for the CloudFormation resource `.ref` attributes ([@rix0rrr] in [#627](#627)). * _**BREAKING**_: Make types accepted in Policy-related classes narrower (from `any` to `Arn`, for example) to reduce typing mistakes ([@rix0rrr] in [#629](#629)). * __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): Align the CodePipeline APIs ([@skinny85] in [#492](#492), [#568](#568)) * __@aws-cdk/aws-ec2__ (_**BREAKING**_): Move Fleet/AutoScalingGroup to its own package ([@rix0rrr] in [#608](#608)). * __aws-cdk__: Simplify plugin protocol ([@RomainMuller] in [#646](#646)). * __@aws-cdk/aws-cloudfront__: Fix CloudFront behavior for ViewerProtocolPolicy ([@mindstorms6] in [#615](#615)). * __@aws-cdk/aws-ec2__: VPC Placement now supports picking Isolated subnets ([@rix0rrr] in [#610](#610)). * __@aws-cdk/aws-logs__: Add `export()/import()` capabilities ([@rix0rrr] in [#630](#630)). * __@aws-cdk/aws-rds__: Fix a bug where a cluster with 1 instance could not be created ([@cookejames] in [#578](#578)) * __@aws-cdk/aws-s3__: Bucket notifications can now add dependencies, fixing creation order ([@eladb] in [#584](#584)). * __@aws-cdk/aws-s3__: Remove useless bucket name validation ([@rix0rrr] in [#628](#628)). * __@aws-cdk/aws-sqs__: Make `QueueRef.encryptionMasterKey` readonly ([@RomainMuller] in [#650](#650)). * __assets__: S3 read permissions are granted on a prefix to fix lost permissions during asset update ([@rix0rrr] in [#510](#510)). * __aws-cdk__: Remove bootstrapping error if multiple stacks are in the same environment ([@RomainMuller] in [#625](#625)). * __aws-cdk__: Report and continue if git throws errors during `cdk init` ([@rix0rrr] in [#587](#587)). * __@aws-cdk/cfnspec__: Updated [CloudFormation resource specification] to `v2.6.0` ([@RomainMuller] in [#594](#594)) + **New AWS Construct Library** - `@aws-cdk/aws-sagemaker` supports AWS::SageMaker resources + **New Resource Types** - AWS::AmazonMQ::Broker - AWS::AmazonMQ::Configuration - AWS::CodePipeline::Webhook - AWS::Config::AggregationAuthorization - AWS::Config::ConfigurationAggregator - AWS::EC2::VPCEndpointConnectionNotification - AWS::EC2::VPCEndpointServicePermissions - AWS::IAM::ServiceLinkedRole - AWS::SSM::ResourceDataSync - AWS::SageMaker::Endpoint - AWS::SageMaker::EndpointConfig - AWS::SageMaker::Model - AWS::SageMaker::NotebookInstance - AWS::SageMaker::NotebookInstanceLifecycleConfig + **Attribute Changes** - AWS::CodePipeline::Pipeline Version (__added__) + **Property Changes** - AWS::AppSync::DataSource HttpConfig (__added__) - AWS::DAX::Cluster SSESpecification (__added__) - AWS::DynamoDB::Table Stream (__added__) - AWS::DynamoDB::Table AutoScalingSupport (__added__) - AWS::EC2::VPCEndpoint IsPrivateDnsEnabled (__added__) - AWS::EC2::VPCEndpoint SecurityGroupIds (__added__) - AWS::EC2::VPCEndpoint SubnetIds (__added__) - AWS::EC2::VPCEndpoint VPCEndpointType (__added__) - AWS::EC2::VPCEndpoint RouteTableIds.DuplicatesAllowed (__deleted__) - AWS::EC2::VPCPeeringConnection PeerRegion (__added__) - AWS::EFS::FileSystem ProvisionedThroughputInMibps (__added__) - AWS::EFS::FileSystem ThroughputMode (__added__) - AWS::EMR::Cluster KerberosAttributes (__added__) - AWS::Glue::Classifier JsonClassifier (__added__) - AWS::Glue::Classifier XMLClassifier (__added__) - AWS::Glue::Crawler Configuration (__added__) - AWS::Lambda::Lambda DLQConfigurationSupport (__added__) - AWS::Neptune::DBInstance DBSubnetGroupName.UpdateType (__changed__) - Old: Mutable - New: Immutable - AWS::SNS::Subscription DeliveryPolicy (__added__) - AWS::SNS::Subscription FilterPolicy (__added__) - AWS::SNS::Subscription RawMessageDelivery (__added__) - AWS::SNS::Subscription Region (__added__) - AWS::SQS::Queue Tags (__added__) - AWS::ServiceDiscovery::Service HealthCheckCustomConfig (__added__) + **Property Type Changes** - AWS::AppSync::DataSource.HttpConfig (__added__) - AWS::DAX::Cluster.SSESpecification (__added__) - AWS::EMR::Cluster.KerberosAttributes (__added__) - AWS::Glue::Classifier.JsonClassifier (__added__) - AWS::Glue::Classifier.XMLClassifier (__added__) - AWS::ServiceDiscovery::Service.HealthCheckCustomConfig (__added__) - AWS::CloudFront::Distribution.CacheBehavior FieldLevelEncryptionId (__added__) - AWS::CloudFront::Distribution.DefaultCacheBehavior FieldLevelEncryptionId (__added__) - AWS::CodeBuild::Project.Artifacts EncryptionDisabled (__added__) - AWS::CodeBuild::Project.Artifacts OverrideArtifactName (__added__) - AWS::CodeBuild::Project.Environment Certificate (__added__) - AWS::CodeBuild::Project.Source ReportBuildStatus (__added__) - AWS::ServiceDiscovery::Service.DnsConfig RoutingPolicy (__added__) - AWS::WAF::WebACL.ActivatedRule Action.Required (__changed__) - Old: true - New: false * __@aws-cdk/cfnspec__: Updated Serverless Application Model (SAM) Resource Specification ([@RomainMuller] in [#594](#594)) + **Property Changes** - AWS::Serverless::Api MethodSettings (__added__) + **Property Type Changes** - AWS::Serverless::Function.SQSEvent (__added__) - AWS::Serverless::Function.EventSource Properties.Types (__changed__) - Added SQSEvent
BREAKING CHANGE: move the S3 CodePipeline Action out of the aws-codepipeline package
and into the aws-s3 package, and rename it from AmazonS3Source to PipelineSource.
Related to #459
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.