apollographql / rover

The CLI for Apollo GraphOS
https://rover.apollo.dev
Other
404 stars 84 forks source link

rover-client: consistent error handling #729

Open EverlastingBugstopper opened 3 years ago

EverlastingBugstopper commented 3 years ago

We have some nice error messages for invalid graph names/variants, but they are not applied evenly to all operations - I'd like to modify the rover-client framework just a bit to make it easier to work with top-level requirements like graphs and make sure that our error messages are consistent across all of the different operations.

setchy commented 3 years ago

I was about to raise a new GH Issue but found this ticket. Here is an observation I had this morning


We are using managed federation and this morning I was experimenting with introducing a new variant name for use via the rover subgraph check command.

If I provide a GRAPH_REF (graph-id@variant-name) that has a valid graph-id, but invalid variant-name, I observed the following error message

error[E007]: The graph `DummyManagedGraph` is a non-federated graph. This operation is only possible for federated graphs.
              Try running the command on a valid federated graph, or use the appropriate `rover graph` command instead of `rover subgraph`.

I was kind of expecting to see an error more along the lines of

Because this DummyManagedGraph is actually a federated graph after all 😉

Similar error when using rover subgraph publish DummyManagedGraph@newVariantName

error[E007]: The graph `DummyManagedGraph` is a non-federated graph. This operation is only possible for federated graphs.
              If you are sure you want to convert a non-federated graph to a subgraph, you can re-run the same command with a `--convert` flag.
EverlastingBugstopper commented 3 years ago

Yeah @setchy - this is a known issue unfortunately. We introduced the --convert flag specifically to help people from accidentally converting a non-federated graph to a federated graph as there is no real way to reverse this. Unfortunately it also had the unintended side effect of giving a confusing error message when a variant doesn't exist!

I believe that what you're referring to is tracked in #661. For now if you do just pass --convert it should create the new variant for you on publish, even though the semantics are a bit wonky.

setchy commented 3 years ago

Thanks @EverlastingBugstopper - I've used the --convert flag to create the new variants.

661 does cover the second part of my feedback. Couldn't see a specific ticket for the first part (ie: error on the rover subgraph check DummyManagedGraph@missingVariantName)

EverlastingBugstopper commented 3 years ago

Ah! Yes the variant checking should definitely be happening prior to checking for if it's a federated graph or not. We'll be sure to fix that as part of this issue.