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

Add dumps features #616

Merged
merged 2 commits into from
Oct 12, 2020
Merged

Add dumps features #616

merged 2 commits into from
Oct 12, 2020

Conversation

neupauer
Copy link
Contributor

This PR adds features to manage dumps introduced in v0.15

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Thanks a lot :) Could you add the two new functions at the bottom of the README?

Copy link
Member

@eskombro eskombro left a comment

Choose a reason for hiding this comment

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

Great PR, thanks!

Just one question... I don't understand how the exceptions are handled in the tests (not very familiar with this js errors). Let me explain: I have the impression that errors thrown are relying on the "message" that the server send! If that's the case, I think it should check the errorCode instead of the error message, since error codes are meant to be stable, while error messages can be changed by MeilISearch anytime!

Nothing else to add for now, thanks for this contribution :)

@bidoubiwa
Copy link
Contributor

bidoubiwa commented Oct 12, 2020

Hey @eskombro this is because I used the following jest method:

await expect(SpecificFunction).rejects.toThrowError(ErrorMessage)

Which uses the printed output of the error, so in our case, the message field. This should be changed but we need to find the appropriate jest method. Could you create an issue about it?
@neupauer copy/pasted how I did the error handling in the other tests.

@curquiza
Copy link
Member

curquiza commented Oct 12, 2020

@neupauer awesome work!! Really clear, thank you for that!

Tips: I see you do this PR from your master branch which we do not recommend 🙂 For example, if we have refused your PR, you would have had a broken master branch compared to ours. Let's keep your PR as it is now, it's okay, but next time, I recommend you creating a separate branch 😉
See here our contribution guidelines.

curquiza
curquiza previously approved these changes Oct 12, 2020
@neupauer
Copy link
Contributor Author

@bidoubiwa I updated the README.md

@bidoubiwa, @eskombro, @curquiza Thank you for your feedback.

- add `createDump`
- add `getDumpStatus`

Signed-off-by: Peter Neupauer <[email protected]>
@bidoubiwa bidoubiwa merged commit ac443fb into meilisearch:master Oct 12, 2020
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