Make graph_name input consistent across commands

[JakeDawkins]

Definitely a duplicate of #99, but I'm going to close that since there is discussion here 🙃

The schema fetch command requires the graph id to be passed as a positional arg, where the other commands like push require it be passed as a flag --graph-name.

rover schema fetch [OPTIONS] <GRAPH_NAME>
rover schema push [OPTIONS] <SCHEMA_PATH> --graph-name <graph-name>

Maybe we should rethink what things we pass as positional args vs which are passed as flags.

Maybe it's not a big deal that we pass graph-name as a flag one place and a positional arg in other places. If people think that, feel free to say so! I'll provide some options in design below if you do agree that we need to make it consistent

[JakeDawkins]

Option 1: Make graph-name a flag always

schema push

USAGE:
    rover schema push [OPTIONS] <SCHEMA_PATH> --graph-name <graph-name>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
        --graph-name <graph-name>    ID of graph in Apollo Studio to push to
    -l, --log <log-level>             [default: debug]  [possible values: error, warn, info,
                                     debug, trace]
        --profile <profile-name>     Name of configuration profile to use [default: default]
        --variant <variant>          Name of graph variant in Apollo Studio to push to [default: current]

ARGS:
    <SCHEMA_PATH>    Path of .graphql/.gql schema file to push

schema fetch

USAGE:
    rover schema fetch [OPTIONS] --graph-name <graph-name>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -l, --log <log-level>            [default: debug]  [possible values: error, warn, info,
                                    debug, trace]
        --profile <profile-name>    Name of configuration profile to use [default: default]
        --variant <variant>         Name of graph variant in Apollo Studio to fetch from [default: current]
        --graph-name <graph-name>    ID of graph in Apollo Studio to fetch from

Option 2: Make a graph id always a positional argument

(this could always be expanded to support a variant later like graph@variant)

schema push

USAGE:
    rover schema push [OPTIONS] <GRAPH_NAME> <SCHEMA_PATH>
FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -l, --log <log-level>             [default: debug]  [possible values: error, warn, info,
                                     debug, trace]
        --profile <profile-name>     Name of configuration profile to use [default: default]
        --variant <variant>          Name of graph variant in Apollo Studio to push to [default: current]

ARGS:
    <GRAPH_NAME>    ID of graph in Apollo Studio to fetch from
    <SCHEMA_PATH>    Path of .graphql/.gql schema file to push

schema fetch

USAGE:
    rover schema fetch [OPTIONS] <GRAPH_NAME>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -l, --log <log-level>            [default: debug]  [possible values: error, warn, info,
                                    debug, trace]
        --profile <profile-name>    Name of configuration profile to use [default: default]
        --variant <variant>         Name of graph variant in Apollo Studio to fetch from [default: current]
ARGS:
    <GRAPH_NAME>    ID of graph in Apollo Studio to fetch from

I think this option seems pretty reasonable, especially if we can settle on a graph@variant identifier before a 1.0 release. This way, we'd be able to drop 2 flags.

I think the logic of this design approach is that the primary thing being worked on in each of these commands is the graph. So specifying the graph as the primary argument makes sense in my mind.

If we did this, I think I'd feel better if we moved the schema_path arg in schema fetch to a flag. I just don't love the idea of having multiple positional args. Maybe that's an unnecessary worry as long as they're always in the same order (graph id first for ex). I think this change could look like this:

schema push

USAGE:
    rover schema push [OPTIONS] <GRAPH_NAME> --schema <schema-path>
FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -l, --log <log-level>             [default: debug]  [possible values: error, warn, info,
                                     debug, trace]
        --profile <profile-name>     Name of configuration profile to use [default: default]
        --variant <variant>          Name of graph variant in Apollo Studio to push to [default: current]
        --schema <schema-path> Path of .graphql/.gql schema file to push OR stdin (use `-`)
ARGS:
    <GRAPH_NAME>    ID of graph in Apollo Studio to fetch from

[zionts]

@JakeDawkins, I'm partial to the --graph flag. I think that the added -name not only makes it longer to write, but actually leads to more confusion rather than less, given that graphs have both a "title" and and "ID" and notably no "name"

[zionts]

I think that having a --graph flag everywhere feels quite reasonable to me, and I would also suggest we consider whether a profile should have a graph: ____ in it so that users can omit the flag when appropriate?

[EverlastingBugstopper]

I think Option 2 makes the most sense, you'll always need a graph name so it makes the most sense for it to be a positional argument.

I think I'd feel better if we moved the schema_path arg in schema fetch to a flag

This also makes sense, especially if we are going to support input from stdin since in that case, schema path is not required.

Generally I'd say it's not super great to have multiple required positional arguments and we should do our best to keep those down. However, I also think there is a time and place for multiple positionals - but only when each of them MUST be specified every time that command is run.

[zionts]

I like having a positional argument when it is the only required argument; otherwise it feels like a strange choice and a challenge for the user to remember.

[JakeDawkins]

This also makes sense, especially if we are going to support input from stdin since in that case, schema path is not required

@EverlastingBugstopper I was talking with Jesse about this, and I'm not sure how to design the stdin option. He suggested using - to explicitly indicate stdin, and in that case you'd still need to use the --schema - flag usage.

I'm not super familiar with stdin usage in other CLI tools. Do they usually require - usage to explicitly define stdin, or do they generally allow it to be used without explicitly saying it's being used (like in #87)?

[EverlastingBugstopper]

Oh that's interesting! I've never seen a tool require changes to arguments in order to take stdin (doesn't mean it doesn't exist though!)

Generally I've seen it used in place of arguments that take a file path. (I'd say we would want an error message if someone tried to pass stdin AND specify a schema path)

[JakeDawkins]

@abernix can you elaborate here? It's not directly relevant to this issue, but I could see it affecting design in this issue 😄 Based on my understanding of our conversation, you were talking about supporting a use of stdin like

echo "type Query { hello: String }" | rover schema push --graph-name test --schema -

and not like

echo "type Query { hello: String }" | rover schema push --graph-name test

right? Or were you saying we could support the second usage as long as we also supported the first?

[JakeDawkins]

From my understanding so far, I think we like the idea of graph-name as a positional arg as long as it's the only one required?

(if we keep it as a flag, I could definitely support renaming it to just --graph/-g

[abernix]

Just quoting my comment from #99 (comment) for its relevance:

As of v0.0.1-rc.0 (release) there are variations in the way that "graph + variant" are provided as rover parameters. For example, the rover schema fetch command expects a <GRAPH_NAME> positional parameter with a --variant flag:

$ rover schema fetch
error: The following required arguments were not provided:
    <GRAPH_NAME>

USAGE:
    rover schema fetch <GRAPH_NAME> --log <log-level> --profile <profile-name> --variant <variant>

... while the rover partial delete command expects a --graph-name and a --graph-variant as flagged parameters (with no positional parameters):

$ rover partial delete
error: The following required arguments were not provided:
    --graph-name <graph-name>
    --service-name <service-name>

USAGE:
    rover partial delete --graph-name <graph-name> --log <log-level> --profile <profile-name> --service-name <service-name> --variant <variant>

... and the rover schema push command (which is in the same subcommand categorization as rover schema fetch in the first example from above), expects similarly flagged parameters but a <SCHEMA_PATH> as the lone positional parameter:

$ ./target/debug/rover schema push --help
rover-schema-push 0.0.1-rc.0
⬆️  Push a schema to Apollo Studio from a local file

USAGE:
    rover schema push [OPTIONS] <SCHEMA_PATH> --graph-name <graph-name>

I actually believe that rover schema fetch is the outlier here but, if <graph-name> is as important as I think it is to most of the commands in the partial and schema commands, I would claim that it should be the model and that we should always have the <graph-name> as the first positional parameter.

Thoughts? Discussion? (If this issue is closed and we resolve this later, we should at least align on one pattern, which I think means fixing rover schema fetch.)

[StephenBarlow]

I like the graph being positional because of the symmetry with Git, which I believe users will appreciate. The graph name effectively becomes the "remote" that you're interacting with.

[abernix]

@abernix can you elaborate here? It's not directly relevant to this issue, but I could see it affecting design in this issue 😄 Based on my understanding of our conversation, you were talking about supporting a use of stdin like

echo "type Query { hello: String }" | rover schema push --graph-name test --schema -

and not like

echo "type Query { hello: String }" | rover schema push --graph-name test

right? Or were you saying we could support the second usage as long as we also supported the first?

Sure! I would claim that it is common convention to use - when a parameter can accept a path to a file OR input from STDIN. In the case where multiple flags might accept file or STDIN, this helps indicate precisely where the STDIN is meant to go, but it's also just explicit. If there is only one parameter that accepts STDIN, I believe it could support a shorthand notation — though I wouldn't say that it should, particularly since it can get ambiguous when adding additional file arguments.

Just off the very top of my head, curl (e.g., echo body | curl --data @-), cat (e.g., echo second_input | cat first_input.txt - third_input.txt, tar (e.g., echo contents | tar cvf -, and kubectl (e.g., kubectl apply -f -) use this convention, but I'm certain there are plenty.

Getting a bit out into left-field, but hopefully relatedly: While it's less common to see in practice, the - is merely common convention/shorthand for using /dev/stdin as the "file"; if we didn't use the - convention, in most programs, users can still get away with passing in STDIN to a file parameter through the /dev/stdin, or lower level file descriptor /dev/fd/0 (file descriptor zero, which /dev/stdin is symlinked to in *nix). More exotically, multiple input streams can be piped around by opening additional file descriptors (like 3) and using, e.g., /dev/fd/3. (0 is STDIN, 1 is STDOUT, and 2 is STDERR).

Personally, I find the second example above (without --schema -) slightly less comfortable due to its lack of explicitness and divergence from explicit required arguments, but that's my own feeling.

[JakeDawkins]

Okay, I think it sounds like we're in agreement on having the graph as a positional arg on all commands.

Remaining questions/proposals:

• drop the variant flag now, in favor of using a single identifier graph@variant
• There's been less talk on the file path for schema push, but I propose we move it to a schema flag for now to avoid having multiple positional args.
• For the question around explicit vs implicit stdin references, I'll make notes in Support stdin for commands #87 for but I think it's less relevant for this, since this right now, since that flag doesn't support stdin YET (although that's next on my list)

Does this all sound reasonable @StephenBarlow @zionts @EverlastingBugstopper @abernix?

[StephenBarlow]

I rather like baking variant into the positional arg exclusively. It elevates variants to where they should be in people's heads.

A separate bikeshed that I reckon is outside the scope of this issue: are we sold on @ being the separator between graph and variant? To me, it suggests a historical version number of the graph. Floating : or / just for the sake of it

[JakeDawkins]

I like the idea of dropping the variant flag too, especially if the long-term plan is to do that anyway. So I think I'd like to officially propose that idea unless there's objection from a product/future perspective

As for the separator, I think we can change that pretty easily later if we want. But I can agree, that I think a different separator may be useful. Especially if we get into the idea of non-unique graph names and need to do something like org/graph:variant someday 🤷‍♂️ 🚲 🏠 this all seems to be a part of a decision record elsewhere, so I'll step back from this comment 😆

[abernix]

Does this all sound reasonable

Agree with all three checkmarks of yours @JakeDawkins

I rather like baking variant into the positional arg exclusively. It elevates variants to where they should be in people's heads.

💯%, and to be honest, I would even say that requiring the inclusion the (today's) default value of current is also maybe not unreasonable because explicitness, but no hard feelings here. I suppose we could have it be optional and just let it default to current still, though I believe that we might have talked about moving away from current and making it required by default in the new CLI seems like it could actually enable clarity on that transition path (allowing those who still use current to keep using it).

A separate bikeshed that I reckon is outside the scope of this issue: are we sold on @ being the separator between graph and variant? To me, it suggests a historical version number of the graph. Floating : or / just for the sake of it

I think this depends on the final assessment; While the decision record has recommendations it is still as of yet, inconclusive.