Skip to content
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

S3UTILS-146: Recover from specific listing parsing errors #302

Merged

Conversation

tmacro
Copy link
Contributor

@tmacro tmacro commented Oct 20, 2023

Adds a util function that will attempt to reparse a listing response to recover from specific errors.
If a value fails to parse (only timestamps and ints presently) then the original string value is returned.

@bert-e
Copy link
Contributor

bert-e commented Oct 20, 2023

Hello tmacro,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

});
});

it('should pass through original error if the XML fails to parse', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need a done callback to make this test async as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strangely it seems to work without, but I'll add one anyway for consistency.

* extractKeys - extract keys from data using the provided extractors
*
* @param {object} data - data to extract keys from
* @param {object<string, Extractor>} keys - Mapping of keys to extractors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Extractor is a function type, we could want to document here what the function should take as arguments, and what it should return.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extractor and ExtractResult are defined above using JSDoc typdefs

@scality scality deleted a comment from bert-e Oct 23, 2023
@bert-e
Copy link
Contributor

bert-e commented Oct 23, 2023

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • 2 peers

utils/safeList.js Outdated Show resolved Hide resolved
utils/safeList.js Show resolved Hide resolved
Comment on lines 7 to 15
{
description: 'should parse empty response',
response: {
ListVersionsResult: {
Name: 'my-bucket',
IsTruncated: false,
MaxKeys: 1000,
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a test with an empty response would make sense here, to make sure we respond correctly

Copy link
Contributor

@williamlardier williamlardier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (maybe you want to bump the version before merging?)

@scality scality deleted a comment from bert-e Oct 27, 2023
@tmacro tmacro force-pushed the improvement/S3UTILS-146/error_resistant_listing branch from 9c841bd to 10e3739 Compare October 30, 2023 18:07
@tmacro tmacro force-pushed the improvement/S3UTILS-146/error_resistant_listing branch from 10e3739 to 3f11858 Compare October 30, 2023 18:24
@tmacro
Copy link
Contributor Author

tmacro commented Oct 30, 2023

/approve

@bert-e
Copy link
Contributor

bert-e commented Oct 30, 2023

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

  • ✔️ development/1.14

The following branches will NOT be impacted:

  • development/1.13
  • development/1.4

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

  • Any commit you add on the source branch will trigger a new cycle after the
    current queue is merged.
  • Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

@bert-e
Copy link
Contributor

bert-e commented Oct 30, 2023

I have successfully merged the changeset of this pull request
into targetted development branches:

  • ✔️ development/1.14

The following branches have NOT changed:

  • development/1.13
  • development/1.4

Please check the status of the associated issue S3UTILS-146.

Goodbye tmacro.

@bert-e bert-e merged commit 3f11858 into development/1.14 Oct 30, 2023
12 checks passed
@bert-e bert-e deleted the improvement/S3UTILS-146/error_resistant_listing branch October 30, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants