[CLOSED] ZFS Provider #55

pjnorton · 2016-04-05T19:39:28Z

Issue by TauZero
Wed Mar 16 19:20:44 2016
Originally opened as mistifyio/mistify#55

List any issues affected/resolved by this pr. https://github.com/blog/1506-closing-issues-via-pull-requests :
Resolves #40

Description:
Adds a ZFS Provider for core zfs functionality. See the RegisterTasks method in zfs.go for a list of handlers that it exposes. Tests included, with updated travis file.

This change is

TauZero included the following code: https://github.com/mistifyio/mistify/pull/55/commits

The text was updated successfully, but these errors were encountered:

pjnorton · 2016-04-05T19:39:29Z

Comment by mmlb
Fri Mar 18 19:05:36 2016

Reviewed 36 of 36 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

cmd/zfs-provider/main.go, line 5 [r1] (raw file):
should we bring this into /mistify?

cmd/zfs-provider/main.go, line 35 [r1] (raw file):
why don't you print the error?

coordinator/cmd/coordinator-cli/main.go, line 65 [r1] (raw file):
argmap is basically a way to make a prefix tree?

coordinator/cmd/coordinator-cli/main.go, line 69 [r1] (raw file):
debug line sneak in?

coordinator/cmd/coordinator-cli/main.go, line 127 [r1] (raw file):
thoughts on stream-in / stream-out or just /in and /out?
I think it'd be good to have them named something similar.

providers/zfs/dataset.go, line 21 [r1] (raw file):
Second sentence isn't really relevant as user documentation no? Should it just be a comment inside the func?

providers/zfs/exists.go, line 12 [r1] (raw file):
can't be type ExistsResult bool?

providers/zfs/zfs.go, line 53 [r1] (raw file):
missing FromJSON

Comments from the review on Reviewable.io

pjnorton · 2016-04-05T19:39:29Z

Comment by TauZero
Fri Mar 18 19:51:33 2016

Review status: 34 of 36 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

cmd/zfs-provider/main.go, line 5 [r1] (raw file):
Definitely. Opened an issue for it mistifyio/mistify#59

cmd/zfs-provider/main.go, line 35 [r1] (raw file):
IIRC when I had originally made the proof of concept provider, all of the startup related errors were logged near where they occurred, so logging the err again here was producing redundant log entries.

coordinator/cmd/coordinator-cli/main.go, line 65 [r1] (raw file):
Yeah, creates an arbitrary nested map. Turning foo.bar.baz=bang and foo.asdf=qwerty into

map[string]interface{}{
  "foo": map[string]interface{}{
    "asdf": "qwerty",
    "bar": map[string]interface{}{
      "baz": "bang",
    }
  }
}

Which then can be json'd and sent along.

coordinator/cmd/coordinator-cli/main.go, line 69 [r1] (raw file):
Done.

coordinator/cmd/coordinator-cli/main.go, line 127 [r1] (raw file):
Response isn't usually a stream. In every case except zfs-send, there's no streaming involved on the response. Also keep in mind that the endpoint paths here don't particularly matter; nothing outside this cli tool needs to be configured to use them; they're provided to the coordinator in the request.

providers/zfs/dataset.go, line 21 [r1] (raw file):
The specifics, your right. My goal was to show that it wasn't just a shortcut to the mountpoint property. I guess I could say "Mountpoint returns the resolved mountpoint of the dataset" and make the second sentence a comment in the method. Thoughts?

providers/zfs/exists.go, line 12 [r1] (raw file):
I hesitate to do basic type returns like that. Makes it harder to add properties later (though less of an issue with monorepo). Thoughts?

providers/zfs/zfs.go, line 53 [r1] (raw file):
Whoops. golint didn't pick up on it because it's not exported. Fixed.

Comments from the review on Reviewable.io

pjnorton · 2016-04-05T19:39:29Z

Comment by mmlb
Fri Mar 18 19:56:27 2016

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

providers/zfs/dataset.go, line 21 [r1] (raw file):
sgtm

providers/zfs/exists.go, line 12 [r1] (raw file):
hmm yeah monorepo makes it fineish though not if it may be used by someone else out side the repo. being forward compatible is a good thing, so lets keep it.

Comments from the review on Reviewable.io