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

fix(lib-storage): add missing return keys #2700

Merged
merged 11 commits into from
May 6, 2022
Merged

Conversation

samchungy
Copy link
Contributor

Issue

Resolves #2694

Description

  • Add Bucket, Key and Location fields to the return output of lib-storage when PUT is used.

Testing

How was this change tested?

  • Unit Tests
  • Compared Output of 3.25 with 3.27.

Additional context

I took the format of the location key from a v3.25 Upload and used it to form the logic for the string creation. Location: 'https://kickoff-dev-serverlessdeploymentbucket-fzcuxfqyzlsl.s3.ap-southeast-2.amazonaws.com/test',


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2021

Codecov Report

Merging #2700 (c110b33) into main (f6a3ef5) will decrease coverage by 16.45%.
The diff coverage is 100.00%.

❗ Current head c110b33 differs from pull request most recent head 19dabdf. Consider uploading reports for the commit 19dabdf to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2700       +/-   ##
===========================================
- Coverage   75.19%   58.73%   -16.46%     
===========================================
  Files         474      571       +97     
  Lines       20721    30629     +9908     
  Branches     4755     7543     +2788     
===========================================
+ Hits        15581    17991     +2410     
- Misses       5140    12638     +7498     
Impacted Files Coverage Δ
lib/lib-storage/src/Upload.ts 86.48% <100.00%> (+0.90%) ⬆️
...ages/middleware-sdk-s3/src/throw-200-exceptions.ts 90.00% <0.00%> (-3.34%) ⬇️
private/aws-protocoltests-ec2/src/endpoints.ts 100.00% <0.00%> (ø)
private/aws-protocoltests-json/src/endpoints.ts 100.00% <0.00%> (ø)
private/aws-protocoltests-query/src/endpoints.ts 100.00% <0.00%> (ø)
private/aws-protocoltests-ec2/src/runtimeConfig.ts 100.00% <0.00%> (ø)
private/aws-protocoltests-json-10/src/endpoints.ts 100.00% <0.00%> (ø)
private/aws-protocoltests-restxml/src/endpoints.ts 100.00% <0.00%> (ø)
...rivate/aws-protocoltests-json/src/runtimeConfig.ts 100.00% <0.00%> (ø)
...rivate/aws-protocoltests-restjson/src/endpoints.ts 100.00% <0.00%> (ø)
... and 109 more

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 6f50571...19dabdf. Read the comment docs.

@wallind
Copy link

wallind commented Sep 22, 2021

any idea if this is slotted to make its way into a release at some point? Not to be impatient just wondering since the last activity was 26 days ago


let Location: string;
if (this.client.config.forcePathStyle) {
Location = `${endpoint.protocol}//${endpoint.hostname}/${locationBucket}/${locationKey}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some playing around with endpoint.path -> but couldn't quite figure out it's behavior, not sure if this needs adjusting.

@samchungy
Copy link
Contributor Author

any idea if this is slotted to make its way into a release at some point? Not to be impatient just wondering since the last activity was 26 days ago

Sorry - no clue 🤷‍♂️ I'm just a contributor not a maintainer

@samchungy samchungy requested a review from a team as a code owner December 6, 2021 23:37
@wallind
Copy link

wallind commented Dec 12, 2021

Sorry - no clue 🤷‍♂️ I'm just a contributor not a maintainer

fair enough haha. Exciting to see some movement on this. If you end up needing a hand hit me back and maybe I'll take a look at this. I'm tired of having to skip the Renovate PRs around this dependency 🙃

@Narretz
Copy link

Narretz commented Jan 26, 2022

@Linkgoron @ajredniwja can this PR get some attention again? It's actually embarassing that this migration option for the upload has been broken for so long.

@Linkgoron
Copy link
Contributor

@Linkgoron @ajredniwja can this PR get some attention again? It's actually embarassing that this migration option for the upload has been broken for so long.

I'm not a part of the AWS team...

@wallind
Copy link

wallind commented Jan 27, 2022

@Linkgoron @ajredniwja can this PR get some attention again? It's actually embarassing that this migration option for the upload has been broken for so long.

I wouldn't have phrased it this way but in sentiment I agree. Is there anything I can do to help expedite this as I'd really like to remove the version lock I have to get around it.

Is it just that we're waiting on maintainers to come back through?

@AlbanDurrheimer
Copy link

AlbanDurrheimer commented Feb 17, 2022

Would it be possible to approve this PR? This is really annoying behavior...
@Linkgoron @trivikr @ajredniwja

@Linkgoron
Copy link
Contributor

Linkgoron commented Feb 17, 2022

Would it be possible to approve this PR? This is really annoying behavior... @Linkgoron @trivikr @ajredniwja

@AlbanDurrheimer As I've already stated elsewhere, please don't ping me... I'm not a part of the team (nor was I ever) and I don't have any merge capabilities or anything like that. Sadly, it was my contribution that caused this bug, which is why I commented in the issue and in the PR, but otherwise I am unrelated to AWS.

@ryanto
Copy link

ryanto commented Mar 14, 2022

Thanks for fixing this!

I maintain a wrapper library around S3 (next-s3-upload) that would benefit a ton from lib-storage. However, the library depends on the Location return from AWS. From my understanding it looks like I wouldn't be able to access Location until this PR is merged.

Is there anything I can do to help get this PR merged? Or, in the mean time, is there any advice for accessing Location?

Thanks again for fixing this!

@kuhe kuhe self-assigned this May 6, 2022
@kuhe kuhe added the pr/needs-review This PR needs a review from a Member. label May 6, 2022
@kuhe kuhe merged commit cbab94e into aws:main May 6, 2022
@wallind
Copy link

wallind commented May 6, 2022

HUZZZAHHHHHHH!

krunk

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr/needs-review This PR needs a review from a Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lib-storage Upload working but result is missing
9 participants