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

feat: rover supergraph fetch #485

Merged
merged 8 commits into from
May 6, 2021
Merged

Conversation

EverlastingBugstopper
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper commented Apr 28, 2021

fixes #452

ran into some pretty interesting edge cases that I think are handled pretty well, with test cases to cover them.

the happy path will just return the latest successful composed supergraphSDL. we also do the usual detection for proper graph@variant combinations.

When schema_tag is null, and the variant they passed is valid, it means there has been no successful composition for this supergraph quite yet, so we return the latest composition error.

$ rover supergraph fetch bad-bad-supergraph
Fetching supergraph SDL from bad-bad-supergraph@current using credentials from the default profile.
error: Unknown type: "Personnnn".
error: Unknown type "Personnnn". Did you mean "Person"?
error: Type Query must define one or more fields.
error: No supergraph SDL exists for "bad-bad-supergraph" because its subgraphs failed to compose.
        Try resolving the composition errors in your subgraph(s), and publish them with the `rover subgraph publish` command.

If both compositionPublish and mostRecentCompositionPublish are null, and the graph@variant exists, I figured it was safe to assume that users were attempting to run supergraph fetch on a non-federated graph. This might be a semi-reliable way to extend our other queries to check if they are running supergraph or subgraph commands on non-federated graphs as part of #121. Unless of course this is an invalid assumption on my part which should be addressed before merging this.

@EverlastingBugstopper EverlastingBugstopper added feature 🎉 new commands, flags, functionality, and improved error messages needs design ✏️ labels Apr 28, 2021
@EverlastingBugstopper EverlastingBugstopper added this to the May 11 - GA milestone Apr 28, 2021
@EverlastingBugstopper
Copy link
Contributor Author

I've updated the top-level comment with the latest changes to use the schemaTag.compositionResult.supergraphSdl API like Josh suggested.

@EverlastingBugstopper
Copy link
Contributor Author

i also ran into something strange where the graphql-client library we're using required that we query for __typename under compositionResult - it seems to think it's a union with possible types CompositionValidationResult | CompositionPublishResult - is this something we will need to detect/treat differently?

@JakeDawkins
Copy link
Contributor

For future reference, from slack:

Yeah, I'm not surprised that's needed. apollo-client adds __typename automatically, but it needs that to know which of the possible interface implementations the result of compositionResult is.

interesting yeah i see there are two possible implementations there, both of them have the one field that we actually need - do you see any reason to treat those two types differently?

Not on our end, but it makes sense why graphql-client would need it. In a world where you're asking for a field that only one of the two has, it'd need to know the type to check it. Now could it recognize that the field is available on both types, and not require you to use __typename here? Probablyyy. But that'd just be an optimization that it makes sense why they wouldn't have added

@EverlastingBugstopper EverlastingBugstopper changed the title [wip] feat: rover supergraph fetch feat: rover supergraph fetch Apr 29, 2021
@jsegaran jsegaran removed their request for review April 30, 2021 15:35
Copy link
Member

@lrlna lrlna left a comment

Choose a reason for hiding this comment

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

This looks good to me!

I added #499 to address querying errors, so it can be done outside of this PR.

When schema_tag is null and we detect that the variant they passed is valid, we return this error

I am into the showing the latest composition error here. Something to think about is that our errors currently do not show any additional output aside form error, suggestion. So we will have to think about how to append additional output from a visual and logical perspective. But I think that's also something that can be done outside of this PR, so we don't block merging this in for too long.

crates/rover-client/src/error.rs Outdated Show resolved Hide resolved
@@ -18,6 +18,7 @@ use crate::utils::table::{self, cell, row};
#[derive(Clone, PartialEq, Debug)]
pub enum RoverStdout {
DocsList(HashMap<&'static str, &'static str>),
SupergraphSdl(String),
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely a nit, feel free to disregard 😄 I wonder if we'd be fine using the SDL variant here?

Or better yet, I know we have a lot of variants that just print a string with a label above it. Maybe we could instead use a LabeledString(String, String) variant or something to make everything consistent and to keep us from having to print_descriptor and print_content over and over again.

If we ever did something like syntax highlighting SDL in terminal outputs, I could imagine having more specific variants like SDL be useful, but I'm not seeing much usefulness out of breaking them up into multiple variants rihgt now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmmm, i'd like to keep this as is for now since they do have a separate descriptor - can we handle a refactor like this in a separate issue/PR combo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally, just a passing thought!

@EverlastingBugstopper
Copy link
Contributor Author

so the composition errors that could be returned here could be quite long and span multiple lines, it might look something like this as a first pass:

error: There is no successful composition for supergraph "my-graph". The latest composition errors are:
COMPOSITION_ERROR 1
COMPOSITION_ERROR 2
COMPOSITION_ERROR 3
        Try fixing these composition errors and running `rover subgraph publish`

thoughts?

@EverlastingBugstopper EverlastingBugstopper changed the title feat: rover supergraph fetch feat: rover supergraph fetch May 3, 2021
@lrlna
Copy link
Member

lrlna commented May 4, 2021

@EverlastingBugstopper I am happy with wording/output. My own nit is on the layout: what do you think about giving it a new line in between each composition error, sort of like we have when we run --log=debug? This might be especially useful for the long errors you were mentioning.

error: There is no successful composition for supergraph "my-graph". The latest composition errors are:

COMPOSITION_ERROR 1

COMPOSITION_ERROR 2

COMPOSITION_ERROR 3

        Try fixing these composition errors and running `rover subgraph publish`

debug output with new lines for example:
Screen Shot 2021-05-04 at 10 25 19

@EverlastingBugstopper
Copy link
Contributor Author

Sure! That seems fine to me - we print out composition errors in rover supergraph compose and rover subgraph publish, I'd imagine we would want to style those with newlines as well?

@EverlastingBugstopper
Copy link
Contributor Author

I made it so the composition errors would look like they do everywhere else in the CLI as a first pass - if we decide we want the new lines added I figure we can change that all at once as a part of solving #508

@EverlastingBugstopper EverlastingBugstopper merged commit 693fe91 into main May 6, 2021
@EverlastingBugstopper EverlastingBugstopper deleted the avery/supergraph-fetch branch May 6, 2021 19:02
EverlastingBugstopper added a commit that referenced this pull request May 6, 2021
lrlna added a commit that referenced this pull request May 7, 2021
Co-authored-by: Irina Shestak <shestak.irina@gmail.com>
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 this pull request may close these issues.

rover supergraph fetch
4 participants