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.

Sign up for GitHub

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

Jump to bottom

issue#97 - Stream depending on params for S3.getObject #236

Merged
merged 2 commits into from
Jun 14, 2021
Merged

issue#97 - Stream depending on params for S3.getObject #236

merged 2 commits into from
Jun 14, 2021

Conversation

moish83
Copy link
Contributor

@moish83 moish83 commented Jun 14, 2021

No description provided.

@moish83
Copy link
Contributor Author

moish83 commented Jun 14, 2021

@nelsonic, this PR should resolve the #97 issue.
Who can review?

@nelsonic
Copy link
Member

Hi @moish83 indeed the code appears to address #97 any chance you can add some test(s) to keep Travis CI happy? 🙏

@nelsonic nelsonic added enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person labels Jun 14, 2021
@moish83
Copy link
Contributor Author

moish83 commented Jun 14, 2021

@nelsonic, yes, I'll add tests.
The travis CI failed on something unrelated to the change (some missing module).

@moish83
Copy link
Contributor Author

moish83 commented Jun 14, 2021

@nelsonic, added test scenario.
When you approve and merge, will a new version gets published automatically?

@nelsonic
Copy link
Member

Travis-CI is mostly happy:
https://travis-ci.org/github/dwyl/aws-sdk-mock/builds/774535824
image

Just one of the builds is failing:
https://travis-ci.org/github/dwyl/aws-sdk-mock/jobs/774535825#L235
image

Looks like this is just an issue with Tap running on node.js v.16.3.0
Specifically: https://www.npmjs.com/package/jackspeak 🤷‍♂️

Think it's safe to merge this, but we may need to disable v16 for now. 💭

@moish83
Copy link
Contributor Author

moish83 commented Jun 14, 2021

Sounds good to me

nelsonic
nelsonic approved these changes Jun 14, 2021
View reviewed changes
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@moish83 thanks for this addition/fix + test. 👍

@nelsonic
Copy link
Member

Merging now. Will tidy up CI on localhost then push changes up.

@nelsonic nelsonic merged commit f78025f into dwyl:master Jun 14, 2021
@nelsonic nelsonic mentioned this pull request Jun 14, 2021
Closed
@nelsonic
Copy link
Member

@moish83 aws-sdk-mock@5.2.0 on NPM now contains your code. 🚀 (Thanks again!)

nelsonic added a commit that referenced this pull request Jun 14, 2021
@nelsonic
Add support for streaming S3 Objects #236 & update sinon+aws-sdk #237
5f4a1f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nelsonic nelsonic nelsonic approved these changes

Assignees
No one assigned
Labels
enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@moish83 @nelsonic