fix(aws-s3-assets): support asset url with two extension name like tar.gz #20874

ntachukwu · 2022-06-25T10:00:24Z

using aws-s3-assets to upload data artifacts of extension tar.gz returns an uploaded asset renamed to <random Id>.gz.

This PR proposes that the AssetStaging Object should be able to check if the uploaded artifact is a tar.gz or any other archive tar file with a compression extension and return the appropriate extension name as stagedPath.

closes #12699

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional 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

gitpod-io · 2022-06-25T10:00:31Z

kellertk · 2022-06-27T13:34:34Z

packages/@aws-cdk/core/lib/asset-staging.ts

+    } else {
+      let extensionName: string = path.extname(this.sourcePath);
+      const sourceName: string = path.basename(this.sourcePath).replace(extensionName, '');
+      const doubleArchive = ARCHIVE_EXTENSIONS.includes(path.extname(sourceName)) ? true : false;


Array.prototype.includes() already returns true or false. This ternary construction is unnecessary.

kellertk · 2022-06-27T13:36:10Z

packages/@aws-cdk/core/lib/asset-staging.ts

-    const stagedPath = this.stagingDisabled
-      ? this.sourcePath
-      : path.resolve(this.assetOutdir, renderAssetFilename(assetHash, path.extname(this.sourcePath)));
+    let stagedPath: string;


Is it possible to refactor the implementation without using let and without the code becoming too difficult to read? You want to use const if at all possible.

I could create a method that checks for double extensions and returns an extension string? This will replace path.extname(this.sourcePath)

kellertk

Instead of overloading ARCHIVE_EXTENSIONS, would it be possible to genericize the implementation? So you can use any arbitrary number of extensions on a file an they'd all be preserved.

…nsion sources

Pull request has been modified.

ntachukwu · 2022-06-27T16:35:14Z

Instead of overloading ARCHIVE_EXTENSIONS, would it be possible to genericize the implementation? So you can use any arbitrary number of extensions on a file an they'd all be preserved.

The idea behind overloading ARCHIVE_EXTENSIONS was to prevent invalid extension names as the basename might contain multiple dots e.g /path/to/artifact/artifact.da.vinci.monalisa.tar.gz which could be mistaken for extension names. Is this not something to be concerned about?

indrora · 2022-07-08T14:27:16Z

Looks great! I would love to see a unit test or two for the new function just to make sure that it handles some common cases. path/to/artifact/artifact.da.vinci.monalisa.tar.gz absolutely deserves a spot in those tests because it's a wonderfully ambiguous one. A file with no extension (for instance Dockerfile and bob) should return something useful... Verify that.

A nitpick: one common extension for compressed tar archives is .tgz and the typical .tar should be in there too for uncompressed tarballs.

…two extension names

pahud · 2022-07-13T00:32:13Z

LGTM

indrora

Looks fantastic.

Pull request has been modified.

mergify · 2022-07-14T18:12:24Z

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 · 2022-07-14T19:19:47Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 1ad4e08
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-07-14T19:20:07Z

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).

…r.gz (aws#20874) using aws-s3-assets to upload data artifacts of extension `tar.gz` returns an uploaded asset renamed to `<random Id>.gz`. This PR proposes that the AssetStaging Object should be able to check if the uploaded artifact is a `tar.gz` or any other archive tar file with a compression extension and return the appropriate extension name as stagedPath. closes aws#12699 ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-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*

add condition to handle asset url with two extension name like tar.gz

94b21ee

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Jun 25, 2022

aws-cdk-automation requested a review from a team June 25, 2022 10:00

add test for tar.gz in staging.test for assets/test

24bbde4

kellertk reviewed Jun 27, 2022

View reviewed changes

kellertk previously requested changes Jun 27, 2022

View reviewed changes

refactor stageByCopying to use a function that checks for double exte…

0600f94

…nsion sources

ntachukwu requested a review from kellertk June 27, 2022 16:15

fix typo in extNameWithDoubleExtCheck function

3671487

ntachukwu added 5 commits July 8, 2022 00:53

change implementatioin for getting extension name

b7fc745

Merge branch 'aws:main' into feat-s3-asset-for-tar-gz

bd2ca22

fix typo

a39d867

return string values in getExtension for loop instead of it's index

fb9d85c

use for...of instead of for...in to loop through ARCHIVE_EXTENSIONS

998bb7f

ntachukwu added 6 commits July 11, 2022 18:23

add tests for asset-staging constructs with tar, tar.gz, tgz assets

b3703e4

Merge branch 'main' into feat-s3-asset-for-tar-gz

b26bccb

Merge branch 'aws:main' into feat-s3-asset-for-tar-gz

7596209

perform more tests to cover negative instances of asset-staging with …

2c94c5e

…two extension names

add compressed archives to archive folder for tests

6183e59

add tests for unusual basename files

8d820f9

ntachukwu changed the title ~~feat(aws-s3-assets): support asset url with two extension name like tar.gz~~ fix(aws-s3-assets): support asset url with two extension name like tar.gz Jul 13, 2022

Merge branch 'main' into feat-s3-asset-for-tar-gz

2aa3ac3

indrora previously approved these changes Jul 14, 2022

View reviewed changes

Merge branch 'main' into feat-s3-asset-for-tar-gz

f99a64d

indrora removed request for a team and kellertk July 14, 2022 17:39

indrora approved these changes Jul 14, 2022

View reviewed changes

Merge branch 'main' into feat-s3-asset-for-tar-gz

1ad4e08

mergify bot merged commit 673b0d1 into aws:main Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(aws-s3-assets): support asset url with two extension name like tar.gz #20874

fix(aws-s3-assets): support asset url with two extension name like tar.gz #20874

ntachukwu commented Jun 25, 2022 •

edited

Loading

gitpod-io bot commented Jun 25, 2022

kellertk Jun 27, 2022

kellertk Jun 27, 2022

ntachukwu Jun 27, 2022

kellertk left a comment

ntachukwu commented Jun 27, 2022

indrora commented Jul 8, 2022

pahud commented Jul 13, 2022

indrora left a comment

mergify bot commented Jul 14, 2022

aws-cdk-automation commented Jul 14, 2022

mergify bot commented Jul 14, 2022

fix(aws-s3-assets): support asset url with two extension name like tar.gz #20874

fix(aws-s3-assets): support asset url with two extension name like tar.gz #20874

Conversation

ntachukwu commented Jun 25, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jun 25, 2022

kellertk Jun 27, 2022

Choose a reason for hiding this comment

kellertk Jun 27, 2022

Choose a reason for hiding this comment

ntachukwu Jun 27, 2022

Choose a reason for hiding this comment

kellertk left a comment

Choose a reason for hiding this comment

ntachukwu commented Jun 27, 2022

indrora commented Jul 8, 2022

pahud commented Jul 13, 2022

indrora left a comment

Choose a reason for hiding this comment

mergify bot commented Jul 14, 2022

aws-cdk-automation commented Jul 14, 2022

AWS CodeBuild CI Report

mergify bot commented Jul 14, 2022

ntachukwu commented Jun 25, 2022 •

edited

Loading