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(s3-deployment): add additional sources with addSource #23321

Merged
merged 2 commits into from
Dec 14, 2022
Merged

feat(s3-deployment): add additional sources with addSource #23321

merged 2 commits into from
Dec 14, 2022

Conversation

corymhall
Copy link
Contributor

PR #22857 is introducing a use case where we need to be able to add additional sources after the BucketDeployment resource is created.

This PR adds an addSource method and changes all the sources evaluation within the construct to be lazy.


All Submissions:

Adding new Construct Runtime Dependencies:

  • This PR adds new construct runtime dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

PR #22857 is introducing a use case where we need to be able to add
additional sources after the `BucketDeployment` resource is created.

This PR adds an `addSource` method and changes all the sources
evaluation within the construct to be lazy.
@gitpod-io
Copy link

gitpod-io bot commented Dec 12, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team December 12, 2022 20:55
@github-actions github-actions bot added the p2 label Dec 12, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 12, 2022
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

@corymhall corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Dec 12, 2022
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 12, 2022 21:00

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2022

Thank you for contributing! Your pull request will be updated from main 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: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 3be742a
  • 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 b34d0b7 into aws:main Dec 14, 2022
@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2022

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

@moltar
Copy link
Contributor

moltar commented Dec 15, 2022

After the upgrade, getting:

Message returned: 'source_markers' and 's3_source_zips' must be the same length (RequestId: c232d0f4-b0bb-49c1-b79c-2aa0771dff42)

@moltar
Copy link
Contributor

moltar commented Dec 15, 2022

Our use case might be a bit unusual, which may cause this?

    const dummyFile = `
      -- Comment
      {{ config() }}
      SELECT 1 AS dummy
    `
    const dummyStageAssets = Source.data('/foo/bar/dummy/dummy.sql', dummyFile)
    const bucketDeploymentSoures = [dummyStageAssets, projectAssets]

    const bucketDeployment = new BucketDeployment(this, 'ProjectDeploy', {
      sources: bucketDeploymentSoures,
      destinationBucket: projectBucket,
      retainOnDelete: false,
    })

@moltar
Copy link
Contributor

moltar commented Dec 15, 2022

Which causes the following diff:

[~] Custom::CDKBucketDeployment Foo/ProjectDeploy/CustomResource ProjectDeployCustomResourceBCA514E4 
 └─ [~] SourceMarkers
     └─ @@ -1,4 +1,3 @@
        [ ] [
        [-]   {},
        [ ]   {}
        [ ] ]

MrArnoldPalmer pushed a commit that referenced this pull request Dec 16, 2022
mergify bot pushed a commit that referenced this pull request Dec 16, 2022
…urces (#23364)

Follow up to #23321. There is an interesting edge case where if there is _any_ source that has source markers then _all_ sources have to have source markers. This is due to the way the custom resource logic works

https://github.com/aws/aws-cdk/blob/02d0876bbb196e9fbeb32d977e7cf65229c8559d/packages/%40aws-cdk/aws-s3-deployment/lib/lambda/index.py#L64

https://github.com/aws/aws-cdk/blob/02d0876bbb196e9fbeb32d977e7cf65229c8559d/packages/%40aws-cdk/aws-s3-deployment/lib/lambda/index.py#L137


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Jan 20, 2023
PR aws#22857 is introducing a use case where we need to be able to add additional sources after the `BucketDeployment` resource is created.

This PR adds an `addSource` method and changes all the sources evaluation within the construct to be lazy.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Jan 20, 2023
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Feb 22, 2023
PR aws#22857 is introducing a use case where we need to be able to add additional sources after the `BucketDeployment` resource is created.

This PR adds an `addSource` method and changes all the sources evaluation within the construct to be lazy.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Feb 22, 2023
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. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants