-
Notifications
You must be signed in to change notification settings - Fork 19
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
do not initialize fields in ObjectMD that are not used in S3C #2268
Conversation
Hello kerkesni,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
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 |
dfac121
to
47eea32
Compare
ecc43dc
to
e99daf4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development/8.1 #2268 +/- ##
================================================
Coverage 64.90% 64.91%
================================================
Files 215 215
Lines 17298 17299 +1
Branches 3514 3514
================================================
+ Hits 11228 11229 +1
Misses 6055 6055
Partials 15 15 ☔ View full report in Codecov by Sentry. |
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.
-
How can we make sure this does not break everything? If we used the model consistently, it would be easy to adress.... but all over the place we access the MD directly, so there may be cases where we don't properly handle the absence of a field...
-
Should not be merge on 8.1 : best to target 8.2 to avoid regressions... (and this is getting merged in backbeat anyway, so we should be fine)
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
@@ -183,7 +183,7 @@ export default class ObjectMD { | |||
'content-length': 0, | |||
'content-type': '', | |||
'content-md5': '', | |||
'content-language': '', | |||
'content-language': 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.
is it worth keeping these (undefiend) fields in the default value?
since:
- the model is defined above (so does not help with 'documenting')
- undefined will not be serialized
- when this type is used, methods should be used to access the data and handle the missing optional fields
Is there a way we are testing this? Edit: Code looks good, but I am not sure if it works fine, so can we test this e2e as well (if not already) |
e99daf4
to
0a2388b
Compare
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue ARSN-411. Goodbye kerkesni. |
We set fields that are not used in S3C to undefined to avoid serializing and unecessarilty store them in the metadata. This could affect sizing and the load on the platform.
Issue: ARSN-411