-
Notifications
You must be signed in to change notification settings - Fork 254
✨(CLDSRV-813) update CloudServer XML ListObjectsV2 to support optional attributes #6043
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
base: feature/CLDSRV-812/list-objects-v2-optional-permissions
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (73.91%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files
@@ Coverage Diff @@
## feature/CLDSRV-812/list-objects-v2-optional-permissions #6043 +/- ##
===========================================================================================
- Coverage 84.36% 84.34% -0.02%
===========================================================================================
Files 204 204
Lines 12935 12957 +22
===========================================================================================
+ Hits 10912 10928 +16
- Misses 2023 2029 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1fde211 to
a8eba02
Compare
| for (const version of versions) { | ||
| if (version.value) { | ||
| version.value.uploadId = undefined; | ||
| version.value.restoreStatus = undefined; |
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 do we need to clear RestoreStatus from all of these tests?
RestoreStatus is only set by cold storage, so will not be set in most case : so seems best to keep this "as is" and handle the cold-storage tests
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.
The issue is that we call directly metadata.listObject in tests. Regarding arsenal PR, the restoreStatus attribute will be returned as soon as we have an archive field.
And we have a diff. The issue is that the versionBefore do a fakeMetadataArchive that update the object. So the version before contains it. This is not done in the second one (versionAfter) and so we have this diff. Regarding the test, that make sense and our API don't change, here we have this diff just because we don't call our API but internal arsenal function.
|
|
||
| for (const version of versions) { | ||
| if (version.value) { | ||
| version.value.restoreStatus = undefined; |
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 do we need to clear RestoreStatus in every test?
all APIs should behave the same as before, so you should not need to tweak the tests
(and resetting restoreStatus may be expected -if anywhere- only in tests where you do actually invoke the list object v2 api with optional attributes)
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.
| assert.strictEqual(err, null); | ||
| assert.strictEqual(result.ListBucketResult.$.xmlns, 'http://s3.amazonaws.com/doc/2006-03-01/'); | ||
| done(); | ||
| }); |
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.
would be nice to have some functional tests as well : i.e. tests using the AWS sdk to retrieve the values from a running cloudserver service.
As an extra benefit, it would also provide an exemple of how to use the API.
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.
That's done, thanks for the suggestion. It was a good idea 🙏
3711fb9 to
037e011
Compare
037e011 to
9ba243d
Compare
…e and switch to a subobject usermetadata Issue: CLDSRV-813
b8457d5 to
0d9a18a
Compare
0d9a18a to
3ceb257
Compare
| } | ||
| }); | ||
|
|
||
| it('should return an XML if the header is only RestoreStatus even without permission', async () => { |
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('should return an XML if the header is only RestoreStatus even without permission', async () => { | |
| it('should always return an XML when the header is RestoreStatus, async () => { |
Related issues
https://scality.atlassian.net/browse/CLDSRV-813