-
Notifications
You must be signed in to change notification settings - Fork 406
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
Add error catching for s3-pull batch insert #2250
Conversation
Just curious -- have we ever considered adding localstack to our docker-compose dev containers? It's pretty handy for local s3 testing, especially. |
I just have two questions: And (just for my curiosity) is there a convention in Spoke to use |
@lastminutediorama No, we haven't considered using localstack before but should, it looks pretty handy! I'll add this note to the issue I created about adding tests for the S3 Pull feature. There is also no convention for using The try/catch here: https://github.com/MoveOnOrg/Spoke/blob/fc267eafbbc07d6d383ddab4bbc17810d7c75883/src/workers/job-processes.js#L150-L166 Would catch error in the In which case it wouldn't catch the error. Also, in the try/catch, it will check if Probably have to dig more into the aws-sdk library to figure it out. Thank you for being so thorough and advising in your PR reviews! |
@lastminutediorama will add unit tests before merging |
Fixes #2236
Description
Originally, I wanted to add tests for this change, but then I found that there currently aren't any existing tests for the s3-pull feature. I started trying to write tests for it, and discovered that it's going to be a complicated task due to the need to mock the
AWS.S3
function: https://github.com/MoveOnOrg/Spoke/blob/2e8c479ef7e4071bc903bdf882334488b50f2910/src/extensions/contact-loaders/s3-pull/index.js#L92This PR is a small change that adds a line of error catching, so I don't think a test is necessary for this. I created a new feature request to add tests to the s3-pull extension: https://github.com/MoveOnOrg/Spoke/issues/2249
Checklist: