-
Notifications
You must be signed in to change notification settings - Fork 252
🥅(CLDSRV-779) Manage error on putBucketReplication properly #5996
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: development/9.0
Are you sure you want to change the base?
🥅(CLDSRV-779) Manage error on putBucketReplication properly #5996
Conversation
Hello darkisdude,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.0 #5996 +/- ##
===================================================
+ Coverage 83.37% 83.40% +0.03%
===================================================
Files 189 189
Lines 12197 12199 +2
===================================================
+ Hits 10169 10175 +6
+ Misses 2028 2024 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
81b303c to
90d6b97
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
90d6b97 to
b872051
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 |
| }); | ||
| }); | ||
|
|
||
| describe('aws-node-sdk test putBucketReplication CORS', () => { |
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.
Are these new tests related to your changes 🤔
You changed getReplicationConfiguration, but the tests aren't calling this function
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.
Yes and no. This PR resolve a 500 errors that we have. Changes in the other PR fix an another issue. If Arsenal is merged after this one, I'll bump arsenal in an another PR to have this change as soon as possible 🙏
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 concur : would be nice to also have a unit test calling getReplicationConfiguration() in case of error...
| request.httpRequest.headers.Origin = 'http://example.com'; | ||
| }); | ||
| request.send(err => { | ||
| assertError(err, 'MalformedXML'); |
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.
Should you add some asserts on err.response.headers ? I ask because the test is called "should return cors headers on malformed xml", but it feels more like a "should return malformedXML on malformed replication configuration" 🤔
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.
did you really mean to add this file?
(we don't generally have these files, maybe worth discussing before adding them here - so we also add them elsewhere... somehow also related to the discussion/analysis on coding style https://scality.atlassian.net/browse/OS-1014)
Pull request template
Description
Motivation and context
Why is this change required? What problem does it solve?
Related issues
Please use the following link syntaxes #600 to reference issues in the
current repository
Checklist
Add tests to cover the changes
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Sign your work
In order to contribute to the project, you must sign your work
https://github.com/scality/Guidelines/blob/master/CONTRIBUTING.md#sign-your-work
Thank you again for contributing! We will try to test and integrate the change
as soon as we can.