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.

Sign up for GitHub

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

Jump to bottom

Update rmrfSync method #23854

Merged
merged 8 commits into from
Oct 22, 2018
Merged

Update rmrfSync method #23854

merged 8 commits into from
Oct 22, 2018

Conversation

liza-mae
Copy link
Contributor

@liza-mae liza-mae commented Oct 4, 2018

Update rmrfSync file for windows.

Part of issue #21315

@liza-mae liza-mae self-assigned this Oct 4, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@liza-mae liza-mae requested a review from tylersmalley October 5, 2018 00:39
@tylersmalley
Copy link
Contributor

Can you elaborate on the issue with this implementation on Windows?

@liza-mae
Copy link
Contributor Author

So the original implementation on windows failed pretty often with a permission issue, so I tried to update the method and use this package which seemed to work better but still sometimes fails with a device busy. So I ended up putting a catch around it, which I have not updated yet in this PR.

tylersmalley
tylersmalley reviewed Oct 16, 2018
View reviewed changes
package.json Outdated
@@ -278,6 +278,7 @@
"expect.js": "0.3.1",
"faker": "1.1.0",
"fetch-mock": "^5.13.1",
"fs-extra": "^7.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just be added to the packages/kbn-test/package.json

@tylersmalley
Copy link
Contributor

@spalger is going to look into this, and suggests using del package if anything as it's used elsewhere in our repo.

@liza-mae liza-mae requested a review from spalger October 16, 2018 18:38
@liza-mae
Copy link
Contributor Author

Do we have example of del? I can try it see if it works consistently.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger
Copy link
Contributor

spalger commented Oct 19, 2018

All of the places that rmrfSync() is used are async functions, so we can actually switch these over to use del() async, which I would suggest.

@spalger spalger force-pushed the liza-fix/win-rmfile branch from 6d3a637 to 8625451 Compare October 19, 2018 22:51
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@liza-mae liza-mae merged commit 5d64140 into elastic:master Oct 22, 2018
@liza-mae liza-mae deleted the liza-fix/win-rmfile branch October 22, 2018 18:12
@liza-mae liza-mae mentioned this pull request Oct 22, 2018
Merged
liza-mae added a commit to liza-mae/kibana that referenced this pull request Oct 22, 2018
@liza-mae
Update rmrfSync method (elastic#23854)
3f0d736 
* Update rmrfSync method

* Add catch statement

* Fix lint issues

* switch to del package

* remove old rmrf_sync module

* Fix CI failure del is not a function error

* Fix spaces
@liza-mae liza-mae mentioned this pull request Oct 22, 2018
Merged
liza-mae added a commit that referenced this pull request Oct 22, 2018
@liza-mae
Update rmrfSync method (#23854) (#24351)
1feba0f 
* Update rmrfSync method

* Add catch statement

* Fix lint issues

* switch to del package

* remove old rmrf_sync module

* Fix CI failure del is not a function error

* Fix spaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spalger spalger spalger approved these changes

@tylersmalley tylersmalley tylersmalley approved these changes

Assignees

@liza-mae liza-mae

Labels
test_infra test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liza-mae @elasticmachine @tylersmalley @spalger