Handle 404 from Scuba backend #5968
Conversation
For Veeam reoutes, this will set 0 as the metric. The value is correct: if scuba returns a 404 error, it means there is no metric for the resource. Issue: CLDSRV-758
Hello williamlardier,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## development/9.1 #5968 +/- ##
===================================================
+ Coverage 83.72% 84.00% +0.27%
===================================================
Files 191 191
Lines 12233 12238 +5
===================================================
+ Hits 10242 10280 +38
+ Misses 1991 1958 -33
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| it('GET capacity.xml should return 200 when scubaclient returns 404 (post-install scenario)', done => { | ||
| // This test simulates the post-install scenario where scubaclient returns 404 | ||
| // because no metrics are available yet. By not calling scuba.incrementBytesForBucket, | ||
| // the mock scuba server will return 404 for this bucket. |
There was a problem hiding this comment.
Should we do something to make sure it's the case ? My point is that if someone change the mock server, this test will continue to pass 🤷
There was a problem hiding this comment.
Already tracked by this ticket: https://scality.atlassian.net/browse/CLDSRV-528
A bit too early to add scuba yet, we need to drop count items first
| // This test reproduces the post-install scenario where scubaclient returns 404 | ||
| // because no metrics are available yet |
There was a problem hiding this comment.
I'm not sure it's very pertinent to put it here. You describe this case already in the lib. If we want to know why we have this case, we can check the code already.
|
|
||
| getVeeamFile(request, response, bucketMd, log); | ||
|
|
||
| // Give async callback time to execute |
There was a problem hiding this comment.
As you use this logic in each test, we can create a function const giveAsyncCallbackTimeToExecute = setImmediate and the in the test
giveAsyncCallbackTimeToExecute(() => {
...
});
Like that your logic will be clean in each tests
| getVeeamFile(request, response, bucketMd, log); | ||
|
|
||
| setImmediate(() => { | ||
| // For 500 errors, we should return error to client |
There was a problem hiding this comment.
| // For 500 errors, we should return error to client |
You already mention that in the it
| getVeeamFile(request, response, bucketMd, log); | ||
|
|
||
| setImmediate(() => { | ||
| // For connection errors, we should return error to client |
| getVeeamFile(request, response, bucketMd, log); | ||
|
|
||
| setImmediate(() => { | ||
| // Metadata errors are returned via responseXMLBody |
There was a problem hiding this comment.
| // Metadata errors are returned via responseXMLBody |
| listVeeamFiles(request, response, null, log); | ||
|
|
||
| setImmediate(() => { | ||
| // Should return NoSuchBucket error |
| }); | ||
| }); | ||
|
|
||
| it('GET system.xml should return 200 even when scubaclient is down', done => { |
There was a problem hiding this comment.
this new test is not directly related to the issue, right ?
It's just testing the logic in veeam/get.js that is not using scuba because of the !isSystemXML(request) right ?
There was a problem hiding this comment.
Nothing much to add from Edouard's review, I still very much prefer functional tests over mocks, and I find mocks hard to read, but it's better than nothing
Nits : some variables could be replaced by constants in veeam-route.js : 'test-bucket', 'getBucket'.
I also prefer to not use default parameter for the object key in createRequest, but not very important
Co-authored-by: Edouard COMTET <edouard.comtet@gmail.com>
4 participants
For Veeam reoutes, this will set 0 as the metric. The value is correct: if scuba returns a 404 error, it means there is no metric for the resource.
Issue: CLDSRV-758