many: implement snapctl command. #1667
Conversation
The `snaptool` command is the method of communication between hooks and snapd. This commit adds the command as well as relevant sections of the REST API, all the way down to the HookManager. The relevant HookManager functionality will be completed in a subsequent commit. Updates: #1586465 Signed-off-by: Kyle Fazzari <kyle@canonical.com>
|
We've agreed online to rename "snaptool" to "snapdo". Suggestions will follow that idea. |
| } | ||
|
|
||
| // RunSnaptool requests a snaptool run for the given context and arguments. | ||
| func (client *Client) RunSnaptool(context string, args []string) (stdout string, stderr string, err error) { |
There was a problem hiding this comment.
With the new name, we can have client.Do(options)client.SnapCtl(options).
Let's please introduce a DoOptionsSnapCtlOptions with Context and Args for now. It'll grow Stdin, and probably evolve further to support files/etc at some point.
There was a problem hiding this comment.
Ah, as the result, let's please do (stdout, stderr []byte, err error), which is more typical for these values.
There was a problem hiding this comment.
Actually, snapctl feels nicer. Allows us to have noun getters without it being awkward (e.g. "snapctl plugs" vs. "snapdo plugs"). Will adapt the points made after that.
There was a problem hiding this comment.
Done.
Ah, as the result, let's please do (stdout, stderr []byte, err error), which is more typical for these values.
Note that []byte is json-encoded as a base64 string, so I had to convert shuffle between []byte and string in order to do this.
|
LGTM with those notes. |
kyrofa
changed the title
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
|
LGTM once tests go green. Thanks for the changes. |
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
kyrofa
force-pushed
the
feature/1586465/snaptool
branch
from
a57364f
to
72fca05
Compare
| @@ -1,4 +1,5 @@ | |||
| /usr/bin/snap | |||
| /usr/bin/snapctl | |||
There was a problem hiding this comment.
I wonder if we want this in PATH or rather hide it a bit more into e.g. /usr/lib/snapd/ ?
There was a problem hiding this comment.
We probably at least want it in PATH for the hook being executed, since that's where we anticipate it being called (at least for now-- not sure what the long-term vision for this tool is). We could still put it in /usr/lib/snapd/ but then getting it the path would require snap run to add that to the environment. What do you think? @niemeyer do you have any thoughts?
There was a problem hiding this comment.
/usr/lib/snapd sounds a bit nicer, as it means hiding something that unusable for most people, and also having tab-completion on bare "snap" completing the space.
There was a problem hiding this comment.
Ah, but agree regarding your point @kyrofa, that we want this in PATH for the hooks. We need to put /usr/lib/snapd in path, or move snapctl inside a subdir there and have that in PATH.
There was a problem hiding this comment.
Alright, I've put snapctl in /usr/lib/snapd/util, and added that directory to the PATH for both apps and hooks via snap run.
There was a problem hiding this comment.
After discussion on telegram, scratched that-- just put it in /usr/lib/snapd.
|
Looks good, just one question inline. |
Make sure that directory is in the PATH for hooks and apps. Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
kyrofa
force-pushed
the
feature/1586465/snaptool
branch
from
c5e900d
to
7e26f05
Compare
The
snapctlcommand is the method of communication between hooks and snapd. This PR makes more progress on LP: #1586465 by adding the command as well as relevant sections of the REST API, all the way down to the HookManager. The relevant HookManager functionality will be completed in a subsequent PR.