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

[Merged by Bors] - Use file name for the external commands (fixes #889) #1008

Closed
wants to merge 2 commits into from

Conversation

avinassh
Copy link
Contributor

Ran into this issue myself, I thought I'd send a patch. Here is the new output now:

$ cargo run

Fluvio command-line interface

fluvio-cli [FLAGS] [OPTIONS] <SUBCOMMAND>

FLAGS:
        --tls                   Enable TLS
        --enable-client-cert    TLS: use client cert
    -h, --help                  Prints help information

OPTIONS:
    -c, --cluster <host:port>          Address of cluster
        --domain <domain>              Required if client cert is used
        --ca-cert <ca-cert>
            Path to TLS ca cert, required when client cert is enabled

        --client-cert <client-cert>    Path to TLS client certificate
        --client-key <client-key>      Path to TLS client private key
    -P, --profile <profile>

SUBCOMMANDS:
    consume         Read messages from a topic/partition
    produce         Write messages to a topic/partition
    topic           Manage and view Topics
    partition       Manage and view Partitions
    profile         Manage Profiles, which describe linked clusters
    cluster         Install or uninstall Fluvio clusters
    install         Install Fluvio plugins
    update          Update the Fluvio CLI
    version         Print Fluvio version information
    fluvio-cloud    Cloud Operations
    help            Prints this message or the help of the given
                    subcommand(s)

Copy link
Contributor

@nicholastmosher nicholastmosher left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @avinassh, it's greatly appreciated!

External subcommands have filenames with the prefix fluvio-, such as fluvio-cloud, but the name of the subcommand that should show up in the help menu should be everything after the prefix. In the fluivo-cloud example, cloud should be displayed in help. I think str::strip_prefix("fluvio-") would work nicely here.

One nit: could you use a match statement instead of if-let/else?

Thanks!

@avinassh
Copy link
Contributor Author

avinassh commented Apr 24, 2021

External subcommands have filenames with the prefix fluvio-, such as fluvio-cloud, but the name of the subcommand that should show up in the help menu should be everything after the prefix

Is this always guaranteed and I can call unwrap on str::strip_prefix OR should I handle the None case as well?

Also, I noticed one more thing:

$ cargo run -- cloud --help

USAGE:
    fluvio-cloud <SUBCOMMAND>

// snipped

I believe this needs to be changed as well, but I am not able to find where exactly this extension is located in Github org

@nicholastmosher
Copy link
Contributor

I think the representation in the help menu is a different issue than this. Fixing it would probably require editing the help menu template used by structopt/clap. Also just FYI, fluvio-cloud currently belongs to a private repo which is why you wouldn't have found it

@avinassh
Copy link
Contributor Author

I think the representation in the help menu is a different issue than this.

yeah, actually you are right. btw do you could tell me about the other thing:

Is this always guaranteed and I can call unwrap on str::strip_prefix OR should I handle the None case as well?

@nicholastmosher
Copy link
Contributor

@avinassh in the current implementation, it should be guaranteed, yes: https://github.com/infinyon/fluvio/blob/master/src/cli/src/install/mod.rs#L43-L58

I'd say let's move forward using that unwrap, and if anything changes such that this would actually panic, we'll consider those changes a bug and we'll update accordingly at that time.

@avinassh
Copy link
Contributor Author

Nice! I have updated the PR.

$ cargo run -- --help

// snipped

SUBCOMMANDS:
    consume      Read messages from a topic/partition
    produce      Write messages to a topic/partition
    topic        Manage and view Topics
    partition    Manage and view Partitions
    profile      Manage Profiles, which describe linked clusters
    cluster      Install or uninstall Fluvio clusters
    install      Install Fluvio plugins
    update       Update the Fluvio CLI
    version      Print Fluvio version information
    cloud        Cloud Operations
    help         Prints this message or the help of the given subcommand(s)

Copy link
Contributor

@nicholastmosher nicholastmosher left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @avinassh!

@sehz sehz added this to the 0.8.1 milestone Apr 27, 2021
@nicholastmosher
Copy link
Contributor

bors r+

bors bot pushed a commit that referenced this pull request Apr 27, 2021
Ran into this issue myself, I thought I'd send a patch. Here is the new output now:

```shell 
$ cargo run

Fluvio command-line interface

fluvio-cli [FLAGS] [OPTIONS] <SUBCOMMAND>

FLAGS:
        --tls                   Enable TLS
        --enable-client-cert    TLS: use client cert
    -h, --help                  Prints help information

OPTIONS:
    -c, --cluster <host:port>          Address of cluster
        --domain <domain>              Required if client cert is used
        --ca-cert <ca-cert>
            Path to TLS ca cert, required when client cert is enabled

        --client-cert <client-cert>    Path to TLS client certificate
        --client-key <client-key>      Path to TLS client private key
    -P, --profile <profile>

SUBCOMMANDS:
    consume         Read messages from a topic/partition
    produce         Write messages to a topic/partition
    topic           Manage and view Topics
    partition       Manage and view Partitions
    profile         Manage Profiles, which describe linked clusters
    cluster         Install or uninstall Fluvio clusters
    install         Install Fluvio plugins
    update          Update the Fluvio CLI
    version         Print Fluvio version information
    fluvio-cloud    Cloud Operations
    help            Prints this message or the help of the given
                    subcommand(s)
```
@bors bors bot changed the title Use file name for the external commands (fixes #889) [Merged by Bors] - Use file name for the external commands (fixes #889) Apr 27, 2021
@bors bors bot closed this Apr 27, 2021
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.

CLI: fluvio --help should display extension executable names rather than pretty names
3 participants