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

Better errors for using subgraph * commands on non-federated graphs #121

Closed
JakeDawkins opened this issue Dec 11, 2020 · 13 comments · Fixed by #459
Closed

Better errors for using subgraph * commands on non-federated graphs #121

JakeDawkins opened this issue Dec 11, 2020 · 13 comments · Fixed by #459
Assignees
Labels
feature 🎉 new commands, flags, functionality, and improved error messages
Milestone

Comments

@JakeDawkins
Copy link
Contributor

JakeDawkins commented Dec 11, 2020

I added a RoverClientError::ExpectedFederatedGraph error case in #117 . I think it'd be good to use that as many places as possible around the subgraph commands in rover-client. We could then match against those cases in rover and instruct users the correct commands to use (and maybe even use the flags passed to subgraph * commands to give them the exact graph * command they should run).

@JakeDawkins JakeDawkins added the feature 🎉 new commands, flags, functionality, and improved error messages label Dec 11, 2020
@abernix abernix added this to the 🐣 0.1.0 milestone Dec 15, 2020
@lrlna lrlna modified the milestones: 0.0.2, 0.0.3 Feb 19, 2021
@JakeDawkins
Copy link
Contributor Author

JakeDawkins commented Feb 19, 2021

Just a note:

we probably want to support running subgraph check on a monolithic graph. Graphs are monoliths by default, and when a user pushes another subgraph, they become a federated graph.

So we'd want to make sure people can run composition checks against a monolithic graph, to make sure the graph would compose if pushed to. But it would still be nice if we could tell the user that was the case. Something like

> This graph is currently unfederated, but pushing a subgraph would change it into a federated graph. These checks reflect that composition

or something...

@EverlastingBugstopper
Copy link
Contributor

TIL! That's pretty nifty that you can just start in on federation like that without having to start from the ground up.

@JakeDawkins
Copy link
Contributor Author

JakeDawkins commented Feb 22, 2021

hmm... actually, looking back now, I think that's not entirely true. You can overwrite an existing schema with a federated one, but I don't think it actually builds off the schema that was already there 🤔 I definitely misled myself while hacking around with a graph lol

@lrlna lrlna modified the milestones: March 9, March 23, April 20 Mar 5, 2021
@lrlna lrlna modified the milestones: April 13, April 27 Apr 8, 2021
@lrlna
Copy link
Member

lrlna commented Apr 14, 2021

This issue could use a list of errors we would need to handle for this.

@EverlastingBugstopper
Copy link
Contributor

Should have better errors for invalid schemas

@JakeDawkins
Copy link
Contributor Author

Should have better errors for invalid schemas

I think this would be better suited in another issue. I think we have enough errors/unexpected cases that this issue should be just for the graph/subgraph errors

@JakeDawkins
Copy link
Contributor Author

Just going through trying to find weird behavior to fix/figure out...

  • running subgraph check on a monolithic graph works just fine for some reason?? The checks page in studio even shows a composition step. I don't think that should be the case for monolithic graphs, right?

@EverlastingBugstopper
Copy link
Contributor

I think this would be better suited in another issue.

sure, that's fine!

running subgraph check on a monolithic graph works just fine for some reason??

that is strange!! what was the full command you ran? the link you sent is broken for me (does it work with just any random subgraph name?)

@JakeDawkins
Copy link
Contributor Author

JakeDawkins commented Apr 19, 2021

running subgraph check on a monolithic graph works just fine for some reason??

that is strange!! what was the full command you ran? the link you sent is broken for me (does it work with just any random subgraph name?)

You'd probably have to be signed in with SSO and in sudo mode to see that check, but yeah. Any ol' subgraph name will do 🙃

demo git:(master) ✗ rover subgraph check My-Graph-2ykhrl -s monolith.graphql --name mono
Checked the proposed subgraph against My-Graph-2ykhrl@current
Compared 1 schema changes against 0 operations
┌────────┬─────────────┬─────────────────────────────────┐
│ Change │    Code     │           Description           │
├────────┼─────────────┼─────────────────────────────────┤
│ PASS   │ FIELD_ADDED │ type `Query`: field `add` added │
└────────┴─────────────┴─────────────────────────────────┘

View full details at https://studio-staging.apollographql.com/service/My-Graph-2ykhrl/checks/6d1e9753-1d16-4305-99a6-1730af0ede71?schemaTag=current&graphCompositionID=512d91da-ac4a-478b-9b05-81d65d521e4a

@EverlastingBugstopper
Copy link
Contributor

yeah seems like the API should return an error there - my guess is the query receives and then promptly discards the subgraph name, when in reality (IMO) it should reject the query entirely

@JakeDawkins
Copy link
Contributor Author

up next: looks like pushing a subgraph to a non-federated graph just overwrites the existing schema, and converts the graph to a federated graph?

$ rover subgraph publish My-Graph-2ykhrl -s monolith.graphql --name mono --routing-url http://localhost:4000
Publishing SDL to My-Graph-2ykhrl:current (subgraph: mono) using credentials from the default profile.
A new subgraph called 'mono' for the 'My-Graph-2ykhrl' graph was created
The gateway for the 'My-Graph-2ykhrl' graph was updated with a new schema, composed from the updated 'mono' subgraph

@JakeDawkins
Copy link
Contributor Author

We can also add a better error for subgraph introspect on non-fed graphs

@EverlastingBugstopper
Copy link
Contributor

up next: looks like pushing a subgraph to a non-federated graph just overwrites the existing schema, and converts the graph to a federated graph?

yeah... this doesn't seem quite right either, at least not without like a --force flag or something. i'd assume you'd want some way to convert a non-federated graph to a federated graph but i'd hate to overwrite something like this on accident.

@JakeDawkins JakeDawkins linked a pull request Apr 21, 2021 that will close this issue
3 tasks
@JakeDawkins JakeDawkins self-assigned this May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🎉 new commands, flags, functionality, and improved error messages
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants