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

[s3-assets] some fields in AssetOptions are deprecated, but is that intended? #9447

Closed
acomagu opened this issue Aug 5, 2020 · 11 comments
Closed
Labels
@aws-cdk/aws-s3-assets closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. documentation This is a problem with documentation. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@acomagu
Copy link
Contributor

acomagu commented Aug 5, 2020

The exclude and follow fields in AssetOptions interface were marked deprecated, because of #7708. But the pull doesn't seem to have the intension to deprecate them. If not, they should be fixed.


This is a 📕 documentation issue

@acomagu acomagu added documentation This is a problem with documentation. feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Aug 5, 2020
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Aug 5, 2020
@nija-at nija-at added @aws-cdk/aws-s3-assets @aws-cdk/core Related to core CDK functionality and removed @aws-cdk/aws-lambda Related to AWS Lambda @aws-cdk/core Related to core CDK functionality labels Aug 6, 2020
@nija-at nija-at assigned eladb and unassigned nija-at Aug 6, 2020
@nija-at nija-at changed the title [aws-lambda] some fields in AssetOptions are deprecated, but is that intended? [s3-assets] some fields in AssetOptions are deprecated, but is that intended? Aug 6, 2020
@iliapolo iliapolo removed feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Aug 9, 2020
@iliapolo
Copy link
Contributor

iliapolo commented Aug 9, 2020

@acomagu These properties are deprecated in favor of using the core library:

/**
* Obtains applied when copying directories into the staging location.
*/
export interface CopyOptions {
/**
* A strategy for how to handle symlinks.
*
* @default SymlinkFollowMode.NEVER
*/
readonly follow?: SymlinkFollowMode;
/**
* Glob patterns to exclude from the copy.
*
* @default - nothing is excluded
*/
readonly exclude?: string[];

This is intended, as CopyOptions from the assets module (which AssetOptions in s3-assets extend from) will be removed in future major versions.

@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 9, 2020
@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Aug 17, 2020
@github-actions github-actions bot added closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Aug 21, 2020
@acomagu
Copy link
Contributor Author

acomagu commented Aug 23, 2020

@iliapolo Sorry for late reply.

So now AssetOptions should extend CopyOptions from core module, shouldn't it? The documentation says the exclude field is deprecated, but the field WON'T be removed from AssetOptions because it still exists in the core module one, right?

@acomagu
Copy link
Contributor Author

acomagu commented Aug 23, 2020

From user perspective, they think that they should not use exclude field any more because it's marked as deprecated. But actually no plan to remove the field from AssetOptions and we don't need to worry about using it.

@iliapolo
Copy link
Contributor

@acomagu Yeah I see it can be confusing. I'll make the deprecation notice a little clearer. Thanks!

@iliapolo iliapolo removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 25, 2020
@danielsharvey
Copy link
Contributor

Should this be re-opened? The deprecation notice remains confusing...

@nija-at nija-at reopened this Nov 6, 2020
@SomayaB SomayaB removed the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Nov 16, 2020
@SomayaB SomayaB added needs-triage This issue or PR still needs to be triaged. feature-request A feature should be added or improved. labels Nov 16, 2020
@iliapolo iliapolo added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Nov 23, 2020
@eladb
Copy link
Contributor

eladb commented Feb 25, 2021

I believe @skinny85 spent some time on this, right?

@eladb eladb assigned skinny85 and unassigned eladb and iliapolo Feb 25, 2021
@mjwbenton
Copy link

Is there a plan for migrating other packages (e.g. aws-s3-deployment) that use aws-s3-assets right now to core? I don't think I can find an issue related to it.

Isn't it possible to make that step without it being a breaking change, and therefore side step the issue with the confusing deprecation notices?

@skinny85
Copy link
Contributor

skinny85 commented Mar 3, 2021

Here's the summary of the situation, and how we will change it:

  1. Currently, the AssetOptions interface from the @aws-cdk/aws-s3-assets module extends the deprecated CopyOptions interface from the deprecated module @aws-cdk/assets.
  2. We will add a new, non-deprecated interface FileCopyOptions to @aws-cdk/core. The above mentioned AssetOptions interface will now extend both CopyOptions and FileCopyOptions. This is done in PR chore: add new interfaces for Assets #13356.
  3. In V2, we will automatically strip all deprecated features from our code. Which means the old CopyOptions will be removed, and only the new FileCopyOptions will remain, thus getting rid of the confusing deprecations.

Hope this clears this up!

Thanks,
Adam

@mjwbenton
Copy link

Gotcha, thanks for the explanation.

@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3-assets closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. documentation This is a problem with documentation. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

8 participants