-
Notifications
You must be signed in to change notification settings - Fork 41
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
Experimental: Use zfs permissions instead of zfs shell and sudo #32
base: master
Are you sure you want to change the base?
Experimental: Use zfs permissions instead of zfs shell and sudo #32
Conversation
Can you look at the merge conflict fix and see if the code is adequate for merging? Thanks. |
@@ -59,25 +59,25 @@ def __init__(self,host="localhost", trust=False, sshcipher=None, properties=None | |||
def _get_poolset(self): | |||
if self._dirty: | |||
properties = [ 'creation' ] + self._properties | |||
stdout2 = subprocess.check_output(self.command + ["zfs", "list", "-Hpr", "-o", ",".join( ['name'] + properties ), "-t", "all"]) | |||
stdout2 = subprocess.check_output(self.command + ["/sbin/zfs", "list", "-Hpr", "-o", ",".join( ['name'] + properties ), "-t", "all"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not encode the path to zfs
. Let the PATH
resolution find it.
@@ -5,6 +5,18 @@ | |||
| <img width="164" height="164" title="" alt="" src="doc/bitcoin.png" /> | | |||
| [1Cw9nZu9ygknussPofMWCzmSMveusTbQvN](bitcoin:1Cw9nZu9ygknussPofMWCzmSMveusTbQvN) | | |||
|
|||
## Experimental version of zfs-tools: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to maintain support for zfs-shell
since not everyone is ready to migrate to ZFS permissions, and people deploy from this repo.
If needed, we can add to zfs-shell
some sort of canary command that the client can then query, and if that fails, revert back to using zfs-shell
instead of ZFS permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, sorry, the last weeks I was too busy with other things .
I wonder how we could make zfs-shell optional.
Case "zfs-shell":
zfs-shell should be installed and registered as a shell. We might even need the sudo rules.
ztools should invoke zfs-shell as a helper script.
Case "zfs-permissions":
zfs-shell should not be installed (or at least) not registered. Sudo rules should not be installed or at least not activated.
ztools should invoke zpool and zfs directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about creating two Debian packages out of a single source package, e.g. "zfs-tools" and optional "zfs-shell" (providing zfs-shell and sudo rules). zfs-tools could suggest or even recommend zfs-shell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to take a look at a second round! |
Experimental: Use zfs permissions instead of zfs shell and sudo
Note: pr23 and pr24 have to be merged before.