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

feat(lib): Use lsblk directly, parse json output #254

Closed
wants to merge 2 commits into from

Conversation

jhermsmeier
Copy link
Contributor

@jhermsmeier jhermsmeier commented Mar 8, 2018

This switches from using a bash script on linux to shelling out
to lsblk directly, using its --json option to obtain & parse
machine-readable output – removing the need for external script
files entirely.

NOTE: This branch is based on diskutil-plist, which has hence has been selected as merge-base. This will automatically resolve to master once #253 is merged.

Change-Type: minor
Depends on: #253
Connects To: #231, #165

This changes to shelling out to `diskutil` directly, instead of
using an intermediate script, and uses the `-plist` option to obtain
and parse the machine-readable output.

Change-Type: minor
This switches from using a bash script on linux to shelling out
to `lsblk` directly, using its `--json` option to obtain & parse
machine-readable output – removing the need for external script
files entirely.

Change-Type: minor
@lurch
Copy link
Contributor

lurch commented Mar 8, 2018

removing the need for external script files entirely.

I guess the stuff in the README about external scripts & yaml needs to be updated then? 😉

@jhermsmeier
Copy link
Contributor Author

Huh? This was unexpected...

@Shou
Copy link

Shou commented Mar 9, 2018

Yeah, that is weird, I was in the middle of reviewing too...

@jhermsmeier
Copy link
Contributor Author

I'll rebase and open a new PR; needs some more work from what it looks like anyways, as it blew on the Linux boxes on Travis – maybe --json isn't supported on older versions of lsblk? Didn't find much info as to feature history, but was apparently added in 2015.

.map((c) => {
return c.label;
});
subLabels = Array.from(new Set(subLabels));
Copy link

Choose a reason for hiding this comment

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

This is to remove duplicates right? Maybe we can add a comment?

error.stderr = stderr;
} else {
try {
// data = JSON.parse(stdout)
Copy link

Choose a reason for hiding this comment

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

Should we remove this?

@Shou
Copy link

Shou commented Mar 9, 2018

Here are my comments anyway :)

@lurch
Copy link
Contributor

lurch commented Mar 9, 2018

but was apparently added in 2015.

...which means it probably won't work on the Ubuntu 12.04 that I think Etcher still supports? :-/

@jhermsmeier
Copy link
Contributor Author

@lurch yeah, also didn't work on Travis at all – see #255, which also adds parsing the "old" --pairs output.

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