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

feat(sdk): add signed url #4065

Merged
merged 32 commits into from
Oct 3, 2023
Merged

Conversation

kavinpanneer
Copy link
Contributor

@kavinpanneer kavinpanneer commented Sep 3, 2023

Stumbled on it while reading the docs and think it reads a bit better without the additional "a".

  • Title matches Winglang's style guide
  • Description explains motivation and solution
  • Tests added (always)
  • Docs updated (only required for features)
  • Added pr/e2e-full label if this feature requires end-to-end testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.

Checklist

  • Title matches Winglang's style guide
  • Description explains motivation and solution
  • Tests added (always)
  • Docs updated (only required for features)
  • Added pr/e2e-full label if this feature requires end-to-end testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.

Stumbled on it while reading the docs and think it reads a bit better without the additional "a".

- [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [x] Description explains motivation and solution
- [x] Tests added (always)
- [x] Docs updated (only required for features)
- [x] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
@kavinpanneer kavinpanneer requested a review from a team as a code owner September 3, 2023 16:59
@kavinpanneer kavinpanneer changed the title chore(docs): remove double word from tests heading (#4063) Feat: Added signed url feature for sdk Sep 3, 2023
@revitalbarletz
Copy link

@subh-cs FYI.

@staycoolcall911
Copy link
Contributor

Hey @kavinpanneer - can you please change the PR title according to these guidelines?

Also - the PR description has 2 checklists - can you please delete one and add a description of what your PR does?
Please include "Fixes #1383 " in your PR description - it will make sure that once the PR is merged, the issue will be closed.

@kavinpanneer kavinpanneer changed the title Feat: Added signed url feature for sdk feat(sdk): Added signed url for sdk Sep 4, 2023
Copy link
Contributor

@tsuf239 tsuf239 left a comment

Choose a reason for hiding this comment

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

The biggest part to be addressed is the signedUrl for the sim. Currently, it doesn't block access after expiring time, one way to solve it is
1- Create an express server to serve the publicUrl & signedUrl files, There is an issue with that, I'll attach the link soon,
2- In the signedUrl method, calculate the time it will be expired and hash it along in the key
3- Create an endpoint to serve the files- it should decode the URL to extract the key and expiry time, if the key was deleted already or the Date.now() is bigger than the expiry date- throw an error, otherwise serve the file (serving a deleted file should be added to the test)

Sorry for making the issue a bit more complicated, If you can think of an alternative approach, let me know :)
Also, please let me know if any help is needed!

Edit: decided to move the sim implementation to: #4143, so just the other little comments and you're ready to go!

examples/tests/sdk_tests/bucket/signedUrl.w Outdated Show resolved Hide resolved
examples/tests/sdk_tests/bucket/signedUrl.w Outdated Show resolved Hide resolved
libs/wingsdk/src/shared-aws/permissions.ts Outdated Show resolved Hide resolved
libs/wingsdk/src/target-sim/bucket.inflight.ts Outdated Show resolved Hide resolved
libs/wingsdk/test/shared-aws/bucket.inflight.test.ts Outdated Show resolved Hide resolved
libs/wingsdk/test/shared-aws/bucket.inflight.test.ts Outdated Show resolved Hide resolved
libs/wingsdk/test/shared-aws/bucket.inflight.test.ts Outdated Show resolved Hide resolved
libs/wingsdk/src/shared-aws/bucket.inflight.ts Outdated Show resolved Hide resolved
libs/wingsdk/test/target-sim/bucket.test.ts Outdated Show resolved Hide resolved
@kavinpanneer kavinpanneer changed the title feat(sdk): Added signed url for sdk feat(sdk): add signed url Sep 8, 2023
@tsuf239
Copy link
Contributor

tsuf239 commented Sep 11, 2023

I talked to @revitalbarletz, and we decided to decrease the scope of the task a little bit- so this issue will be for implementing the signedUrl method for the tf-aws target, and the implementation for the sim target will be a part of #4143 :)
for now please revert the changes for the sim- throw a "Not implemented yet" error in the inflight method, and conditionally skip the the assertion on the test written in wing (see

if (util.env("WING_TARGET") != "sim") {
assert(http.get(publicUrl).body == "Foo");
}
)

Copy link
Contributor

@itssubhodiproy itssubhodiproy left a comment

Choose a reason for hiding this comment

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

I think these are the few changes should be done, although I'm open discuss different approaches.

libs/wingsdk/src/cloud/bucket.ts Outdated Show resolved Hide resolved
libs/wingsdk/src/shared-aws/bucket.inflight.ts Outdated Show resolved Hide resolved
libs/wingsdk/src/shared-aws/bucket.inflight.ts Outdated Show resolved Hide resolved
libs/wingsdk/src/shared-aws/bucket.inflight.ts Outdated Show resolved Hide resolved
libs/wingsdk/src/target-sim/bucket.inflight.ts Outdated Show resolved Hide resolved
libs/wingsdk/test/shared-aws/bucket.inflight.test.ts Outdated Show resolved Hide resolved
libs/wingsdk/test/target-sim/bucket.test.ts Outdated Show resolved Hide resolved
kavinpanneer and others added 4 commits September 23, 2023 17:32
Co-authored-by: Subhodip Roy <75121304+subh-cs@users.noreply.github.com>
Co-authored-by: Subhodip Roy <75121304+subh-cs@users.noreply.github.com>
Copy link
Contributor

@tsuf239 tsuf239 left a comment

Choose a reason for hiding this comment

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

I think it's ready! 👏 👏 👏
let's take care of the merge conflict :)
(I'm checking if the tests pass on aws and will review again shortly)

@tsuf239
Copy link
Contributor

tsuf239 commented Oct 1, 2023

@kavinpanneer it seems like you need to run pnpm install again in your working branch- the lockfile is conflicted
(then I'll be able to run the aws tests and approve if passes)

@monadabot monadabot added the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Oct 2, 2023
Copy link
Contributor

@tsuf239 tsuf239 left a comment

Choose a reason for hiding this comment

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

Approved! Great job!!

@tsuf239 tsuf239 removed the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Oct 3, 2023
@tsuf239
Copy link
Contributor

tsuf239 commented Oct 3, 2023

sdk tests are all passing:
image

@mergify
Copy link
Contributor

mergify bot commented Oct 3, 2023

Thanks for contributing, @kavinpanneer! This PR will now be added to the merge queue, or immediately merged if feat-sdk-signedurl is up-to-date with main and the queue is empty.

@mergify mergify bot merged commit 6b0b357 into winglang:main Oct 3, 2023
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.34.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants