-
Notifications
You must be signed in to change notification settings - Fork 2
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
Count items crashes if a bucket has a website configuration #322
Conversation
Hello williamlardier,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
CountItems/CountWorker.js
Outdated
@@ -30,6 +30,11 @@ class CountWorker { | |||
if (!this.client.client) { | |||
return callback(new Error('NotConnected')); | |||
} | |||
// 'fromObj' expects that the website configuration is an instance of | |||
// WebsiteConfiguration As it it not used in CountItems, we nullify it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would be not be a WebsiteConfiguration
? There is probably another root cause, would be best to fix it instead of masking it with a band-aid...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not band-aid: the function we use, fromObj
, loads an object and expects the website configuration to be of the right instance, it is not the deSerialize
method, where we accept a non-instanciated JSON object. The latter could be used, but we would need to serialize the bucket metadata object. I chose to keep the same function, but adapt the object to pass the class assert
s... (less computations)
why would be not be a WebsiteConfiguration ?
Because this object comes from MongoDB, and has no specific class inside, it's just an object with fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also definitely not update arsenal for the count-items job, that we really need to drop today...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
b63f150
to
9716663
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
/create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
/approve |
The website configuration object, when calling "fromObj" must be of type WebsiteConfiguration, otherwise, an assert will fail, and this will cause a crash of count items.
9716663
to
e96a3d1
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: approve, create_integration_branches |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue S3UTILS-168. Goodbye williamlardier. The following options are set: approve, create_integration_branches |
This bug is fixed by nullifying nay website configuration before creating the Arsenal object, as we do not need anything related to websites in count items.
Issue: S3UTILS-168