Skip to content
This repository was archived by the owner on Jan 28, 2025. It is now read-only.

Conversation

@pradyumna1
Copy link
Contributor

@pradyumna1 pradyumna1 commented Sep 9, 2020

As per AWS docs, bucket name can consist of periods. If an S3 bucket is being used as origin, we need to extract the string before s3 in <bucket-name>.s3.amazonaws.com. I came up with this simple solution as a quick fix, however, we can also use regex for extracting anything before s3.amazonaws.com from the hostname.
For eg. const bucketName = hostname.slice().replace(/(.*)\.s3\.amazonaws\.com/g, '$1')

@dphang
Copy link
Collaborator

dphang commented Sep 9, 2020

Seems reasonable if it always starts with s3, maybe just add some simple tests if possible (unless already covered by other tests).

@danielcondemarin
Copy link
Contributor

I'd agree tests for this are needed to avoid regression later.

@danielcondemarin
Copy link
Contributor

#761 looks related. I might jump later in this branch or create a new one to tidy this up.

@codecov
Copy link

codecov bot commented Nov 12, 2020

Codecov Report

Merging #596 (8877ed4) into master (f730097) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
+ Coverage   80.14%   80.18%   +0.04%     
==========================================
  Files          57       58       +1     
  Lines        1838     1842       +4     
  Branches      405      405              
==========================================
+ Hits         1473     1477       +4     
  Misses        307      307              
  Partials       58       58              
Impacted Files Coverage Δ
...ponents/aws-cloudfront/lib/getBucketNameFromUrl.js 100.00% <100.00%> (ø)
...s-components/aws-cloudfront/lib/getOriginConfig.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f730097...8877ed4. Read the comment docs.

@dphang
Copy link
Collaborator

dphang commented Mar 2, 2021

Closing due to inactivity for some time, if needed please reopen new PR with tests. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants