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

use functions to handle bindfs umount tool output more flexibly #27

Conversation

lachie
Copy link
Contributor

@lachie lachie commented Nov 29, 2017

The output of my OSX bindfs umount was slightly different to that
expected by vg localInstall, so activation failed.

Not yet tested on fusermount.

The output of my OSX bindfs umount was slightly different to that
expected by vg localInstall, so activation failed.

Not yet tested on fusermount.
@codecov-io
Copy link

Codecov Report

Merging #27 into master will decrease coverage by 0.45%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #27      +/-   ##
=======================================
- Coverage   52.45%   52%   -0.46%     
=======================================
  Files          28    27       -1     
  Lines         671   673       +2     
=======================================
- Hits          352   350       -2     
- Misses        220   224       +4     
  Partials       99    99
Impacted Files Coverage Δ
internal/workspace/localInstall.go 39.44% <0%> (-0.9%) ⬇️
main.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69cbc98...4b1e41d. Read the comment docs.

@JelteF
Copy link
Contributor

JelteF commented Dec 5, 2017

Thanks for the PR. It looks fine to me, I always considered parsing the output of the command a bit fragile but couldn't think of a better way. Could you make some small unit tests that check the behaviour of these new functions you created?

@JelteF JelteF mentioned this pull request Dec 15, 2017
@JelteF JelteF closed this in #30 Dec 15, 2017
JelteF added a commit that referenced this pull request Dec 15, 2017
@JelteF
Copy link
Contributor

JelteF commented Dec 15, 2017

I just added the tests myself. Thanks for the PR and reporting the issue.

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.

3 participants