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

cmd/snap: add ability to register "snap routine" commands #7589

Merged
merged 6 commits into from
Jan 17, 2020

Conversation

jhenstridge
Copy link
Contributor

@jhenstridge jhenstridge commented Oct 11, 2019

@chipaca suggested I split this out from PR #7588 for independent review.

I want to add a snap subcommand that is not intended for use by humans. I also don't want to reserve command names that could be useful for future human-facing commands. The command is intended to offer an API that the caller can rely on too, so the `snap debug namespace seems inappropriate.

This PR implements a proposal from @zyga to add a new namespace for such commands. After discussion in this PR, the decision was made to namespace these commands as snap routine.

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

I still like it. Thank you :)

@chipaca chipaca requested a review from pedronis October 11, 2019 11:06
@chipaca chipaca added the Needs Samuele review Needs a review from Samuele before it can land label Oct 11, 2019
Copy link
Contributor

@chipaca chipaca left a comment

Choose a reason for hiding this comment

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

I like this approach. I think we should think about moving our other internal commands under this.

I've flagged @pedronis so he can give his input on it though.

@pedronis
Copy link
Collaborator

@chipaca @jhenstridge

I like this approach. I think we should think about moving our other internal commands under this.

we have a lot of hidden commands ATM (even ignoring debug ones), but is not clear there is a simple path forward to move them to "internal ..." without problems of backward compatibility, they are used either by services or other packages. We would need to think that through, otherwise it seems we should simply continue as we did so far and add internal command without "internal" but just hidden.

@jhenstridge
Copy link
Contributor Author

One thing to note is that putting things under internal potentially makes them more discoverable since the individual subcommands are not hidden. For example:

$ snap internal --help
...
Available commands:
  portal-info  Return information about a process

I can easily remove the snap internal stuff from the portal-info PR though, if that's the decision though.

@pedronis
Copy link
Collaborator

pedronis commented Oct 15, 2019

@chipaca, potential commands to move would be:

advise-snap, auto-import, handle-link, userd

they are all used though by some piece of packaging under data/ so as I said the backward compatibility story is not very clean

@pedronis pedronis added ⛔ Blocked and removed Needs Samuele review Needs a review from Samuele before it can land labels Oct 16, 2019
@chipaca
Copy link
Contributor

chipaca commented Oct 16, 2019

@jhenstridge WRT them being hidden, they should all be hidden, both the 'internal' command and each of its sub-commands. Probably needs a tweak on registerCommands (may be as simple as registerCommand propagating .Hidden from baseCmd).

@pedronis I like it because it gives structure to something that right now is rather messy (internal commands are split between random toplevels and debug). Sadly indeed we need to keep a bunch of the toplevels for backwards compatibility, but having internal would stop the spread of this mess, containing it. At the same time (but in a separate PR) we could move all internal commands in here (whether debug or internal wins for internal-debug-commands is debatable), and keep the current toplevels as aliases.

Alternatively we could say 'all future internal commands go under debug', and that would satisfy my need to contain the mess :-)

@pedronis
Copy link
Collaborator

@jhenstridge what/from where will portal-info be called? will it controlled from a piece of config we ship? or hard-coded into an upstream?

@jhenstridge
Copy link
Contributor Author

The call will be hard coded into an upstream protect we don't control (xdg-desktop-portal). The upstream is friendly, but we won't have as much control over when updates are pushed to non Ubuntu distros.

@pedronis
Copy link
Collaborator

@jhenstridge thanks, @chipaca @jhenstridge it seems we need more a term/verb that means interfacing from/to other programs than internal, then at least:

advise-snap, handle-link and portal-info

could go under that

@jhenstridge
Copy link
Contributor Author

@jhenstridge WRT them being hidden, they should all be hidden, both the 'internal' command and each of its sub-commands. Probably needs a tweak on registerCommands (may be as simple as registerCommand propagating .Hidden from baseCmd).

It is not obvious to me why we'd want this: we don't currently hide individual snap debug commands, and this seems similar. The sub-commands will not show up in the output of snap --help, and if they find out about the a hidden command namespace and run e.g. snap debug --help then they probably want to know about what's in that command namespace.

@chipaca @jhenstridge it seems we need more a term/verb that means interfacing from/to other programs than internal, then at least:

Maybe snap api would fit? That seems to cover the type of commands we're talking about, and the abbreviation is unlikely to be confused with the existing "snap interface" concept.

@pedronis
Copy link
Collaborator

@jhenstridge WRT them being hidden, they should all be hidden, both the 'internal' command and each of its sub-commands. Probably needs a tweak on registerCommands (may be as simple as registerCommand propagating .Hidden from baseCmd).

It is not obvious to me why we'd want this: we don't currently hide individual snap debug commands, and this seems similar. The sub-commands will not show up in the output of snap --help, and if they find out about the a hidden command namespace and run e.g. snap debug --help then they probably want to know about what's in that command namespace.

debug is not mentioned in snap help / snap --help ATM but it could. This new command wouldn't/shouldn't. debug commands themselves: some are hidden, some are not, the logic there depends whether they exist only for tests, whether they are useful invoked by a user directly, how annoying/strange is their xp etc.

Maybe snap api would fit? That seems to cover the type of commands we're talking about, and the abbreviation is unlikely to be confused with the existing "snap interface" concept.

one issue here is that afaict, cc @chipaca, hidden commands are still auto-completed, I'm not sure whether that's intentional or a bug that is hard to fix? and whether hidden vs autocomplete should be orthogonal.

with that in mind my current thinking would be for something like:

  • xpi cross program interface, external program interface
  • xctl cross control, external control

@jhenstridge
Copy link
Contributor Author

I can see how api could be a problem when completion is taken into account, given that there are real commands that start with "a", and that it would be close to the top of the list when typing snap <tab>. This also counts as a strike against what's currently in the PR, since internal shares a five character prefix with commonly used user facing commands.

If hidden commands can't be removed from completion results, a name starting with "x" has the benefit of being less prominent and not sharing a prefix with existing user facing commands. xpi may be meaningful to long time Mozilla users, where it was the file extension used for XPInstall browser extension packages. But that seems to be falling out of use with modern extensions usually just being packaged as a .zip file.

@chipaca
Copy link
Contributor

chipaca commented Oct 29, 2019

Hidden commands are auto-completed, go-flags does not filter them out. We could probably do that filtering ourselves; go-flags does have an extension mechanism for this, even though it's rather crude.
We could also upstream a fix, if we consider it a bug.

@pedronis
Copy link
Collaborator

Hidden commands are auto-completed, go-flags does not filter them out. We could probably do that filtering ourselves; go-flags does have an extension mechanism for this, even though it's rather crude.
We could also upstream a fix, if we consider it a bug.

Having a different autocomplete flag that is considered true for not hidden, and default to false for hidden for but can be set to true if wanted would be ideal. This would give us slightly cleaner options to pursue this.

I'm not sure we want to upstream a hidden == no autocomplete behavior without overrides.

@chipaca how much work would it be just doing it on our side?

@chipaca
Copy link
Contributor

chipaca commented Oct 29, 2019

@pedronis,

@chipaca how much work would it be just doing it on our side?

I can have a PR up by 4pm today :-)

@chipaca
Copy link
Contributor

chipaca commented Oct 29, 2019

@pedronis #7688

@pedronis
Copy link
Collaborator

@jhenstridge @chipaca #7690 makes the default for hidden commands to not complete anymore. This gives us more liberty to pick a verb/adjective/acronym that makes the most sense here.

@jhenstridge
Copy link
Contributor Author

If completion is no longer an issue, I'm partial to snap api still. I can update the PR to use whatever you guys would prefer.

@pedronis
Copy link
Collaborator

pedronis commented Nov 1, 2019

If completion is no longer an issue, I'm partial to snap api still. I can update the PR to use whatever you guys would prefer.

My issue with api is that it hints to a strong public contract, which is true/applies for portal-info, but the other candidate commands are glued via config snippets, their stability is more tied to the fact that those snippets are updated by packaging which moves much slower than the core/snapd snaps, than anything else.

So I'm looking more for something more generic that just means behavior on behalf of some other program (external or internal to snapd) vs user: now that completion is out of the way we can go for some actual word, so what I'm thinking right now is:

snap routine advise-snap
snap routine handle-link
snap routine portal-info

(even snap routine auto-import would fit)

@chipaca, @jhenstridge what do you think?

@chipaca
Copy link
Contributor

chipaca commented Nov 1, 2019

@pedronis I'm terrible with names™, so I won't be binding on this. Having said that, routine does have a connotation of something that happens periodically (a routine doctor's visit, a routine oil change). I don't, however, have a better suggestion.

@pedronis
Copy link
Collaborator

pedronis commented Nov 1, 2019

@pedronis I'm terrible with names™, so I won't be binding on this. Having said that, routine does have a connotation of something that happens periodically (a routine doctor's visit, a routine oil change). I don't, however, have a better suggestion.

it has traditionally been used for "subprogram" as well, independently of call regularity

I think it should be fine, the fact is slightly quirky make it so it's unlikely we will want to use it for something user oriented.

@jhenstridge I would go ahead with routine, as we said it should be hidden, it can have a bare help though - if we want - listing what it contains.

As we said initially we want to put: advise-snap and handle-link under it, but keep those at top level as aliases as well.

@jhenstridge
Copy link
Contributor Author

I've updated the branch to use "snap routine" instead now, and work with the completion changes. I haven't added secondary registrations of handle-link and advise-snap though: do you want that change here, or keep that as a separate PR?

@pedronis pedronis removed their request for review November 1, 2019 12:08
@pedronis
Copy link
Collaborator

pedronis commented Nov 1, 2019

I've updated the branch to use "snap routine" instead now, and work with the completion changes. I haven't added secondary registrations of handle-link and advise-snap though: do you want that change here, or keep that as a separate PR?

at this point you should work with @chipaca and @mvo5 to push this forward, as I mentioned those commands will need to change a couple of bits of packaging (I don't know with what consequences)

@pedronis pedronis changed the title cmd/snap: add ability to register "snap internal" commands cmd/snap: add ability to register "snap routine" commands Nov 1, 2019
@pedronis
Copy link
Collaborator

pedronis commented Nov 1, 2019

@jhenstridge please do adjust the PR description as well, I changed summary myself

@jhenstridge
Copy link
Contributor Author

Just to confirm: the initial comment in the PR was edited as requested.

@pedronis pedronis merged commit 5da13d4 into canonical:master Jan 17, 2020
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.

4 participants