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

cli2.0: subcommands framework + conan user command #7372

Merged
merged 16 commits into from Jul 23, 2020

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Jul 17, 2020

Changelog: Feature: Complete cli2.0 framework to handle sub-commands and add conan user command for cli 2.0
Docs: omit

Base of discussion for:

  • Implementation of commands with subcommands in cli 2.0.
  • Implementation of conan user command.

With the current proposal adding commands with subcommands and formatters for these subcommands would look something like this: (outdated, see: #7372 (comment))

def output_user_list_json(info, out):
    ...

def output_user_list_cli(info, out):
    ...

list_formatters = {"cli": output_user_list_cli,
                   "json": output_user_list_json}

subcommands = [{"name": "list", "help": "List users and remotes they are associated to"},
               {"name": "add", "help": "Add user authentication for a remote"},
               {"name": "remove", "help": "Remove associated user from remote"},
               {"name": "update", "help": "Update the current user for a remote"}]

formatters = {"list": list_formatters}


@conan_command(group="Misc commands", formatters=formatters, subcommands=subcommands)
def user(*args, conan_api, parser, subparsers, **kwargs):
    # list, add, remove, update
    """
    Authenticates against a remote with user/pass, caching the auth token.

    Useful to avoid the user and password being requested later. e.g. while
    you're uploading a package.  You can have one user for each remote.
    Changing the user, or introducing the password is only necessary to
    perform changes in remote packages.
    """
    # list subcommand
    subparsers["list"].add_argument(...

    # add subcommand
    subparsers["add"].add_argument(...

    # remove subcommand
    subparsers["remove"].add_argument(...
    ....
    args = parser.parse_args(*args)
    info = {}
    if args.subcommand == "list":
        info=...
    return info

Also, the new proposal of the user command is done using subcommands:

Conan v1:

➜  cli2.0 conan user --help
usage: conan user [-h] [-c] [-p [PASSWORD]] [-r REMOTE] [-j JSON] [-s] [name]

Authenticates against a remote with user/pass, caching the auth token.

Useful to avoid the user and password being requested later. e.g. while
you're uploading a package.  You can have one user for each remote.
Changing the user, or introducing the password is only necessary to
perform changes in remote packages.

positional arguments:
  name                  Username you want to use. If no name is provided it
                        will show the current user

optional arguments:
  -h, --help            show this help message and exit
  -c, --clean           Remove user and tokens for all remotes
  -p [PASSWORD], --password [PASSWORD]
                        User password. Use double quotes if password with
                        spacing, and escape quotes if existing. If empty, the
                        password is requested interactively (not exposed)
  -r REMOTE, --remote REMOTE
                        Use the specified remote server
  -j JSON, --json JSON  json file path where the user list will be written to
  -s, --skip-auth       Skips the authentication with the server if there are
                        local stored credentials. It doesn't check if the
                        current credentials are valid or not

Conan v2:

usage: conan user [-h] {list,add,remove,update} ...

Authenticates against a remote with user/pass, caching the auth token.

Useful to avoid the user and password being requested later. e.g. while
you're uploading a package.  You can have one user for each remote.
Changing the user, or introducing the password is only necessary to
perform changes in remote packages.

positional arguments:
  {list,add,remove,update}
                        sub-command help
    list                List users and remotes they are associated to
    add                 Add user authentication for a remote
    remove              Remove associated user from remote
    update              Update the current user for a remote

optional arguments:
  -h, --help            show this help message and exit

Also removing --skip-auth: #5443 (comment)

@czoido czoido marked this pull request as draft Jul 17, 2020
@memsharded
Copy link
Member

@memsharded memsharded commented Jul 17, 2020

IMO, the declarative syntax of

list_formatters = {"cli": output_user_list_cli,
                   "json": output_user_list_json}

subcommands = [{"name": "list", "help": "List users and remotes they are associated to"},
               {"name": "add", "help": "Add user authentication for a remote"},
               {"name": "remove", "help": "Remove associated user from remote"},
               {"name": "update", "help": "Update the current user for a remote"}]

formatters = {"list": list_formatters}

Is going to be more of an inconvenience than a time saver, and will probably have unexpected rough edges.
I think that being completely explicit in each command about subcommands and formatters is not a big deal.

I like very much the proposal for the conan user with subcommands. Probably needs a clean subcommand that removes all entries. Also we need to talk about the functionality of each subcommand, right now (1.X), this command will not really authenticate in some cases, we probably want to be less ambiguous in 2.0.

@czoido
Copy link
Contributor Author

@czoido czoido commented Jul 17, 2020

Is going to be more of an inconvenience than a time saver, and will probably have unexpected rough edges.
I think that being completely explicit in each command about subcommands and formatters is not a big deal.

I will work in a more explicit alternative and see how it looks compared to this so we can have more perspective.

I like very much the proposal for the conan user with subcommands. Probably needs a clean subcommand that removes all entries. Also we need to talk about the functionality of each subcommand, right now (1.X), this command will not really authenticate in some cases, we probably want to be less ambiguous in 2.0.

conan user --clean behaviour could be done with: conan user remove --remotes "*"

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Jul 17, 2020

IMO, we are focusing too much on the technical implementation, but...

We cannot change the philosophy for the subcommands. If commands are a function inserted via a decorator, subcommands should follow the same pattern:

# This is a first level command (because "Misc commands" is 0-level group)
@conan_command(group="Misc commands", formatters=formatters)
def user(*args, conan_api, parser, **kwargs):
    ...

# This is a subcommand because I'm adding it to a _group_ of a command.
@conan_command(group="user", formatters=formatters)
def user_list(*args, conan_api, parser, **kwargs):
    ...

...maybe group should be renamed to parent, or they can be different arguments, both would work and won't need another page of documentation.

Also, probably it will be useful (sometimes highly desirable) to add a name argument, but it is just a very easy detail:

@conan_command(group="user", formatters=formatters, name="list")
def user_list(*args, conan_api, parser, **kwargs):
   ...

@czoido
Copy link
Contributor Author

@czoido czoido commented Jul 17, 2020

Thanks @jgsogo I like the idea you are proposing, I will work in an implementation taking that comments into account.

@czoido
Copy link
Contributor Author

@czoido czoido commented Jul 20, 2020

I have change the implementation, now you can use conan_command and conan_subcommand decorators (could be done with just a decorator and passing parameters, but I preferred splitting in two).
The implementation of one command with subcommands is something like this:

def output_user_list_json(info, out):
    ...

def output_user_list_cli(info, out):
    ...

@conan_subcommand(formatters={"cli": output_user_list_cli, "json": output_user_list_json})
def user_list(*args, conan_api, parser, subparser):
    """
    List users and remotes they are associated to.
    """
    subparser.add_argument("-r", "--remotes", action=Extender, nargs="?"...
    args = parser.parse_args(*args)
    info = conan_api.list_users(...
    return info

@conan_subcommand()
def user_add(*args, conan_api, parser, subparser):
    ...

@conan_subcommand()
def user_remove(*args, conan_api, parser, subparser):
    ...

@conan_subcommand()
def user_update(*args, conan_api, parser, subparser):
    ...

@conan_command(group="Misc commands")
def user(*args, conan_api, parser, **kwargs):
    """
    Authenticates against a remote with user/pass, caching the auth token.

    Useful to avoid the user and password being requested later. e.g. while
    you're uploading a package.  You can have one user for each remote.
    Changing the user, or introducing the password is only necessary to
    perform changes in remote packages.
    """
    return

The subcommands have to be named like <command>_my_subcommand and the name of the subcommand for the cli will be: my-subcommand just like the main commands.

Copy link
Member

@memsharded memsharded left a comment

The framework for subcommands is looking fantastic, great work!

On the specifics of the conan user commands and subcommands, maybe it would be good to discuss and understand the existing rough edges and the new proposal, and outline the future behavior and implementation.

@czoido czoido changed the title cli2.0: conan user command cli2.0: subcommands framework + conan user command Jul 21, 2020
@czoido czoido marked this pull request as ready for review Jul 22, 2020
@czoido czoido requested a review from danimtb Jul 22, 2020
Copy link
Member

@memsharded memsharded left a comment

Looking great, can be merged.

conans/cli/command.py Outdated Show resolved Hide resolved
conans/cli/command.py Outdated Show resolved Hide resolved
conans/cli/commands/user.py Outdated Show resolved Hide resolved
conans/cli/commands/user.py Outdated Show resolved Hide resolved
conans/cli/commands/user.py Outdated Show resolved Hide resolved
@memsharded memsharded added this to the 1.28 milestone Jul 23, 2020
@danimtb danimtb merged commit ae212d8 into conan-io:develop Jul 23, 2020
2 checks passed
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.

None yet

4 participants