Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRead data from notifications #171
Conversation
|
I think this generally looks good. I noticed there aren't any new tests; do existing tests cover the new code? I've left style consistency comments inline; these aren't critical but very desirable. Question for verification testing: What's your preferred testing workflow so I can try it out? Did you run the sync server locally, and with what AWS permissions. |
| @@ -15,6 +15,9 @@ const EXPIRED_CREDENTIAL_ERRORS = [ | |||
| /The provided token has expired\./, | |||
| /Invalid according to Policy: Policy expired\./ | |||
| ] | |||
| const SQS_MAX_LIST_MESSAGES_COUNT = 10 | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
SergeyZhukovsky
Oct 17, 2017
Author
Member
yep sure, but do you think that I should move the upper block there as well?
const PUT_CONCURRENCY = 100
const S3_MAX_RETRIES = 1
const EXPIRED_CREDENTIAL_ERRORS = [
/The provided token has expired./,
/Invalid according to Policy: Policy expired./
]
This comment has been minimized.
This comment has been minimized.
| sslEnabled: true | ||
| }) | ||
|
|
||
| return {s3, postData, expiration, bucket, region, SQS, SNS} |
This comment has been minimized.
This comment has been minimized.
ayumi
Oct 16, 2017
Contributor
minor: capitalization on SQS vs sqs, in this repo we're using lowercase for instances and SQS for class names / constructors. can you adjust for consistency with s3?
| TopicConfigurations: [ | ||
| { | ||
| Events: [ | ||
| 's3:ObjectCreated:Post' |
This comment has been minimized.
This comment has been minimized.
ayumi
Oct 16, 2017
Contributor
minor: maybe it's not important now, but in the future maybe we need to subscribe to ObjectDeleted as well (happens when user hits Reset Sync which deletes everything from S3)
This comment has been minimized.
This comment has been minimized.
SergeyZhukovsky
Oct 17, 2017
Author
Member
That subscription is: our s3 bucket is allowed to post notification to SNS topic. Probably it is a good idea to delete the corresponding SNS and SQS in that case. Thanks, I'll do it.
| callback(error, null) | ||
| } else if (data) { | ||
| for (let message of data.Messages) { | ||
| var key = '' |
This comment has been minimized.
This comment has been minimized.
| @@ -95,6 +95,20 @@ class UserAwsCredentialGenerator { | |||
| "Effect": "Allow", | |||
| "Action": "s3:DeleteObject", | |||
| "Resource": "${this.arnS3Prefix()}/*" | |||
| }, | |||
| { | |||
This comment has been minimized.
This comment has been minimized.
ayumi
Oct 16, 2017
Contributor
does this work? i'm curious if the sync server AWS IAM profile needs to be updated to have to permission to do this as well. iirc the child credentials can only authorize a subset of the parent permissions.
This comment has been minimized.
This comment has been minimized.
SergeyZhukovsky
Oct 17, 2017
Author
Member
I haven't checked the Rest Sync, but I see that I haven't implemented the delete of SNS and SQS. I'll fix that tomorrow.
|
Thank you for the review. I see some critical things in my implementation that need to be fixed, especially with a Reset Sync functionality. I'm going to work on that tomorrow. |
| @@ -169,11 +199,216 @@ RequestUtil.prototype.list = function (category, startAt, maxRecords) { | |||
| } | |||
| if (startAt) { options.StartAfter = `${prefix}/${startAt}` } | |||
| return this.withRetry(() => { | |||
| return s3Helper.listObjects(this.s3, options, !!maxRecords) | |||
| if (!startAt || startAt === 0 || ((new Date()).getTime() - startAt) > | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -96,13 +96,13 @@ const startSync = (requester) => { | |||
| return jsRecords | |||
| } | |||
|
|
|||
| ipc.on(messages.FETCH_SYNC_RECORDS, (e, categoryNames, startAt, limitResponse) => { | |||
| ipc.on(messages.FETCH_SYNC_RECORDS, (e, categoryNames, startAt, limitResponse, platform) => { | |||
This comment has been minimized.
This comment has been minimized.
diracdeltas
Oct 17, 2017
Member
please document the platform parameter in client/constants/messages.js
This comment has been minimized.
This comment has been minimized.
diracdeltas
Oct 17, 2017
Member
on second thought, is this necessary at all? at least on laptop, the background page should be able to get the platform via window.navigator.userAgent
This comment has been minimized.
This comment has been minimized.
SergeyZhukovsky
Oct 17, 2017
Author
Member
it could be ignored on laptop. I added it to manage notifications like history or preferences records on mobile. We don't sync them for now there, but if I don't poll and delete them Amazon could keep returning me them again and again. It is just a temporary till we have those records sync on mobile as well.
|
I think this would be okay to test on staging. I'm 90% sure old clients who don't support SQS+SNS will continue to work. |
|
I think I configured everything on staging, I'll ping you when I finish fixes after the review. |
|
@SergeyZhukovsky this looks good for testing on staging. before merging to master i think we should make sure tests cover SNS/SQS functionality (set up a queue, upload record, read from queue) also if there were server IAM permission changes needed, we should write them down in our terraform AWS config |
SergeyZhukovsky commentedOct 12, 2017
Implemented a logic for SNS->SQS notifications.