-
Notifications
You must be signed in to change notification settings - Fork 88
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
compose from running services, registered graphs or subgraphs #519
Conversation
let url = &subgraph_data.routing_url.clone().ok_or_else(|| { | ||
let err = anyhow!("No routing_url found for schema file."); | ||
let mut err = RoverError::new(err); | ||
err.set_suggestion(Suggestion::ValidComposeRoutingUrl); | ||
err | ||
})?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This calls for us to probably create a RoverCliError type so we can set these outside our actual commands. I'll do this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this and was able to generate a supergraph composition from multiple sources. Very nice! 🎉
Note I haven't reviewed the Rust code for correctness, even if I've left a couple notes within it.
I did run into a couple cases where my routing URL was not emitted into the supergraph schema:
- When fetching from Studio Registry, I think we only get the
name
and thesdl
right now. I have an actual comment on code below about this. - When fetching subgraph SDL via introspection from a running GraphQL endpoint.
For the first case, I think using the value from the registry would be ideal. In the absence of that functionality, I wonder if we should just guard and require that routing_url
be defined in the YAML?
For both of these cases, I think the presence of the routing_url
in the YAML config should be passed to the composition function in the url
property. Currently, the following YAML configuration, produces a supergraph without (routing) URLs (url
) in the output:
YAML Config
subgraphs:
accounts:
routing_url: https://localhost:4001/
schema:
file: ./accounts.graphql
reviews:
# I HAVE SPECIFIED THIS! I expected it in the output.
routing_url: http://localhost:4002/
schema:
subgraph_url: http://localhost:4002/ # this specifically runs subgraph introspection to obtain the SDL
products:
# SEE I HAVE THIS COMMENTED OUT.
# I expected it to come from the registry's `url` property.
# ... but see the 'inventory' service below for a related thought.
# routing_url: http://localhost:4003/
schema:
graphref: My-Graph-2-w515ae@current
subgraph: products
inventory:
# I have NOT commented this out. It still does emit to the
# supergraph (see output below; I don't think it's getting passed
# to the harmonizer compose function?). I think when specifying
# 'routing_url' we should pass it. And I think it's okay if we
# just firmly require the presence of `routing_url` for now and
# relax it later?
routing_url: http://localhost:4004/
schema:
graphref: My-Graph-2-w515ae@current
subgraph: inventory
... resulted in:
Note this is a partial output / snipped
enum join__Graph {
ACCOUNTS @join__graph(name: "accounts" url: "https://localhost:4001/")
INVENTORY @join__graph(name: "inventory" url: "")
PRODUCTS @join__graph(name: "products" url: "")
REVIEWS @join__graph(name: "reviews" url: "")
}
As a secondary note, I left some comments about our support for a true introspection response as input to composition, which I think we can leave out for now.
This is looking real nice though and the errors I personally encountered were top notch.
CHANGELOG.md
Outdated
subgraph_url: https://example.com/people | ||
reviews: | ||
schema: | ||
url: https://reviews.example.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today, we can only compose federated subgraphs into a supergraph. I can imagine that working in a future version, but today, we necessitate the presence of federated directives (e.g., @key
, @requires
, etc.) to dictate/inform the composition.
src/command/supergraph/compose.rs
Outdated
SchemaSource::Introspection { url } => { | ||
// given an introspection URL, use graph introspect to | ||
// obtain SDL and add it to subgraph_definition. | ||
let client = Client::new(&url.to_string()); | ||
|
||
let introspection_response = graph::introspect::run(&client, &HashMap::new())?; | ||
let schema = introspection_response.result; | ||
|
||
let subgraph_definition = SubgraphDefinition::new(subgraph_name, "", &schema); | ||
subgraphs.push(subgraph_definition); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you find a schema on which this worked? Perhaps there's a federated implementation out there that could properly encode the nuances of federation into the introspection response, but I think the official introspection format is still lacking the depth to convey both definitions of and implementations of (on schema types) directives other than @deprecated
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to get it to work by just introspecting https://countries.trevorblades.com/!! o.O wild!
Co-authored-by: Jesse Rosenberger <[email protected]>
Runs supergraph composition from various sources: local graphql file, introspection SDL, subgraph introspection SDL, registered subgraph SDL. An example file that would compose:
To run this PR locally
(if you don't have rust installed:
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
)
install this particular branch of rover:
cargo install --git https://github.com/apollographql/rover --branch lrlna/rover-449
Current composition specific errors include:
No valid routing_url found
Provided schema file is not valid
Because we are reusing our
introspection
andfetch
work the following errors come for free:closes #449