chore: add new interfaces for Assets #12700

skinny85 · 2021-01-25T21:56:01Z

In V2, we want to get rid of the @aws-cdk/assets module, as it's considered deprecated in V1.
Unfortunately, the module contains an interface, CopyOptions, that is used, through interface inheritance, in the public API of many stable CDK modules like ECS, Lambda, ECR, CodeBuild, etc.

While we have a CopyOptions interface in @aws-cdk/core, it unfortunately shares the same name, follow, with the property in the "old" CopyOptions. But the two different follow properties have different types.

For that reason, if we're going to remove the "old" CopyOptions using JSII's "strip deprecated" option, we can't use the "new" CopyOptions in the inheritance hierarchy alongside the "old" CopyOptions, as the two definitions of follow would conflict.

Because of that, create a new FileCopyOptions interface which renames the follow property to followSymlinks. Also add a FileFingerprintOptions interface that does a similar trick to the FingerprintOptions interface (which extends CopyOptions), which is used in the public API of modules that use Docker assets.

Also extract a few module-private interfaces to avoid duplication of properties between all of these interfaces.

After this change, an interface from one of the non-deprecated assets libraries (S3 or ECR) using FileOptions from @aws-cdk/assets will look like this:

// this is in @aws-cdk/aws-s3-assets
export interface AssetOptions extends assets.CopyOptions, cdk.FileCopyOptions, cdk.AssetOptions {
  // ...

Then, when we enable stripping the deprecated elements using JSII on the V2 branch, this will be turned to:

export interface AssetOptions extends cdk.FileCopyOptions, cdk.AssetOptions {
  // ...

Allowing us to deprecate the @aws-cdk/assets module, and not ship it with aws-cdk-lib.

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

In V2, we want to get rid of the @aws-cdk/assets module, as it's considered deprecated in V1. Unfortunately, the module contains an interface, CopyOptions, that is used, through interface inheritance, in the public API of many stable CDK modules like ECS, Lambda, ECR, CodeBuild, etc. While we have a CopyOptions interface in @aws-cdk/core, it unfortunately shares the same name, `follow`, with the property in the "old" CopyOptions. But the two different `follow` properties have different types. For that reason, if we're going to remove the "old" CopyOptions using JSII's "strip deprecated" option, we can't use the "new" CopyOptions in the inheritance hierarchy alongside the "old" CopyOptions, as the two definitions of `follow` would conflict. For that reason, create a new FileCopyOptions interface which renames the `follow` property to `followSymlinks`. Also add a FileFingerprintOptions interface that does a similar trick to the FingerprintOptions interface (which extends CopyOptions), which is used in the public API of modules that use Docker assets. Also extract a few module-private interfaces to avoid duplication of properties between all of these interfaces.

gitpod-io · 2021-01-25T21:56:05Z

nija-at

cc @eladb who is more familiar with this part of the CDK.

nija-at · 2021-01-26T13:30:23Z

packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts

 import * as cxapi from '@aws-cdk/cx-api';
 import { Construct } from 'constructs';

 /**
 * Options for DockerImageAsset
 */
-export interface DockerImageAssetOptions extends assets.FingerprintOptions {


I assume changes to this file are not related to your PR description, yes?

I don't understand what you're asking, sorry. They are very much related (in fact, the description explicitly calls out FingerprintOptions).

nija-at · 2021-01-26T13:37:55Z

packages/@aws-cdk/core/lib/fs/options.ts

+export interface CopyOptions extends FileOptions {
+  /**
+   * A strategy for how to handle symlinks.
+   *
+   * @default SymlinkFollowMode.NEVER
+   */
+  readonly follow?: SymlinkFollowMode;
+}
+
+/**
+ * Options applied when copying directories into the staging location.
 */
-export interface FingerprintOptions extends CopyOptions {
+export interface FileCopyOptions extends FileOptions {


Do we need both of these or can we deprecate the former?

Again, sorry, I don't understand what you're asking here (deprecate which interface?).

You don't have to be sorry. It's ok.

I was asking about CopyOptions and FileCopyOptions. They look exactly the same.

Sure, we can deprecate CopyOptions. It's used in a bunch of public classes, so it will require changes to augment CopyOptions with FileCopyOptions in all of these usages, and it's not linked to deprecating @aws-cdk/assets directly.

Let me know if you want to make this part of this PR, or tackle it separately after this is merged.

Can you get @eladb's eyes on this? He owns and has built this area and it'd best for him to take a look at this PR and assess this change.

I'll defer to him on what the right thing here is.

eladb

LGTM, but before I approve, can you please add to the PR description the final state that we will have in V2 after the all the deprecations are removed from the library?

eladb · 2021-01-28T09:13:55Z

packages/@aws-cdk/assets/lib/fs/options.ts

@@ -10,6 +10,7 @@ export interface CopyOptions {
   * A strategy for how to handle symlinks.
   *
   * @default Never
+   * @deprecated use `followSymlinks` instead


Where is followSymlinks defined?

It's defined in FileCopyOptions in @aws-cdk/core.

packages/@aws-cdk/core/lib/fs/options.ts

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

skinny85 · 2021-01-28T22:21:51Z

LGTM, but before I approve, can you please add to the PR description the final state that we will have in V2 after the all the deprecations are removed from the library?

@eladb done! Let me know if the description makes sense now.

mergify · 2021-01-31T08:57:09Z

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

aws-cdk-automation · 2021-01-31T09:35:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 35fef7e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-01-31T09:35:27Z

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

This reverts commit 1a9f2a8.

Needs reverting because of aws/jsii#2256 . This reverts commit 1a9f2a8. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

In V2, we want to get rid of the `@aws-cdk/assets` module, as it's considered deprecated in V1. Unfortunately, the module contains an interface, `CopyOptions`, that is used, through interface inheritance, in the public API of many stable CDK modules like ECS, Lambda, ECR, CodeBuild, etc. While we have a `CopyOptions` interface in `@aws-cdk/core`, it unfortunately shares the same name, `follow`, with the property in the "old" `CopyOptions`. But the two different `follow` properties have different types. For that reason, if we're going to remove the "old" `CopyOptions` using JSII's "strip deprecated" option, we can't use the "new" `CopyOptions` in the inheritance hierarchy alongside the "old" `CopyOptions`, as the two definitions of `follow` would conflict. Because of that, create a new `FileCopyOptions` interface which renames the `follow` property to `followSymlinks`. Also add a `FileFingerprintOptions` interface that does a similar trick to the `FingerprintOptions` interface (which extends `CopyOptions`), which is used in the public API of modules that use Docker assets. Also extract a few module-private interfaces to avoid duplication of properties between all of these interfaces. After this change, an interface from one of the non-deprecated assets libraries (S3 or ECR) using `FileOptions` from `@aws-cdk/assets` will look like this: ```ts // this is in @aws-cdk/aws-s3-assets export interface AssetOptions extends assets.CopyOptions, cdk.FileCopyOptions, cdk.AssetOptions { // ... ``` Then, when we enable stripping the deprecated elements using JSII on the V2 branch, this will be turned to: ```ts export interface AssetOptions extends cdk.FileCopyOptions, cdk.AssetOptions { // ... ``` Allowing us to deprecate the `@aws-cdk/assets` module, and not ship it with `aws-cdk-lib`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…s#12832) Needs reverting because of aws/jsii#2256 . This reverts commit 1a9f2a8. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This is a re-submit of the PR aws#12700, which had to be reverted because of JSII issue aws/jsii#2256. Since that issue has been fixed in JSII version `1.23.0`, which is what we currently use, re-introduce the changes from that PR.

This is a re-submit of the PR #12700, which had to be reverted because of JSII issue aws/jsii#2256. Since that issue has been fixed in JSII version `1.23.0`, which is what we currently use, re-introduce the changes from that PR. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This is a re-submit of the PR aws#12700, which had to be reverted because of JSII issue aws/jsii#2256. Since that issue has been fixed in JSII version `1.23.0`, which is what we currently use, re-introduce the changes from that PR. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This is a re-submit of the PR #12700, which had to be reverted because of JSII issue aws/jsii#2256. Since that issue has been fixed in JSII version `1.23.0`, which is what we currently use, re-introduce the changes from that PR. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

skinny85 requested a review from nija-at January 25, 2021 21:56

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 25, 2021

skinny85 added 2 commits January 25, 2021 14:00

Use followSymlinks inside @aws-cdk/aws-s3-assets.Asset.

eaf0f9f

Use followSymlinks inside @aws-cdk/aws-ecr-assets.DockerImageAsset.

d4710fc

nija-at requested a review from eladb January 26, 2021 13:25

nija-at reviewed Jan 26, 2021

View reviewed changes

eladb reviewed Jan 28, 2021

View reviewed changes

Obtains -> Options in CopyOptions JSDocs

11b7c13

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

skinny85 requested a review from eladb January 28, 2021 22:21

eladb approved these changes Jan 31, 2021

View reviewed changes

Merge branch 'master' into chore/deprecate-assets

35fef7e

mergify bot merged commit 1a9f2a8 into aws:master Jan 31, 2021

skinny85 deleted the chore/deprecate-assets branch February 1, 2021 21:16

skinny85 added a commit to skinny85/aws-cdk that referenced this pull request Feb 2, 2021

revert: revert "chore: add new interfaces for Assets (aws#12700)"

a01f887

This reverts commit 1a9f2a8.

MrArnoldPalmer mentioned this pull request Feb 2, 2021

can a diamond dependency be used aws/jsii#2256

Closed

4 tasks

skinny85 mentioned this pull request Mar 3, 2021

chore: add new interfaces for Assets #13356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add new interfaces for Assets #12700

chore: add new interfaces for Assets #12700

skinny85 commented Jan 25, 2021 •

edited

Loading

gitpod-io bot commented Jan 25, 2021 •

edited

Loading

nija-at left a comment

nija-at Jan 26, 2021

skinny85 Jan 26, 2021

nija-at Jan 26, 2021

skinny85 Jan 26, 2021

nija-at Jan 26, 2021 •

edited

Loading

skinny85 Jan 26, 2021

nija-at Jan 27, 2021

eladb left a comment

eladb Jan 28, 2021

skinny85 Jan 28, 2021 •

edited

Loading

skinny85 commented Jan 28, 2021

mergify bot commented Jan 31, 2021

aws-cdk-automation commented Jan 31, 2021

mergify bot commented Jan 31, 2021

chore: add new interfaces for Assets #12700

chore: add new interfaces for Assets #12700

Conversation

skinny85 commented Jan 25, 2021 • edited Loading

gitpod-io bot commented Jan 25, 2021 • edited Loading

nija-at left a comment

Choose a reason for hiding this comment

nija-at Jan 26, 2021

Choose a reason for hiding this comment

skinny85 Jan 26, 2021

Choose a reason for hiding this comment

nija-at Jan 26, 2021

Choose a reason for hiding this comment

skinny85 Jan 26, 2021

Choose a reason for hiding this comment

nija-at Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

skinny85 Jan 26, 2021

Choose a reason for hiding this comment

nija-at Jan 27, 2021

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

eladb Jan 28, 2021

Choose a reason for hiding this comment

skinny85 Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

skinny85 commented Jan 28, 2021

mergify bot commented Jan 31, 2021

aws-cdk-automation commented Jan 31, 2021

AWS CodeBuild CI Report

mergify bot commented Jan 31, 2021

skinny85 commented Jan 25, 2021 •

edited

Loading

gitpod-io bot commented Jan 25, 2021 •

edited

Loading

nija-at Jan 26, 2021 •

edited

Loading

skinny85 Jan 28, 2021 •

edited

Loading