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

Add subgraph push option to specify endpoint #130

Merged
merged 3 commits into from
Jan 12, 2021
Merged

Conversation

JakeDawkins
Copy link
Contributor

@JakeDawkins JakeDawkins commented Dec 17, 2020

This is just an implementation with a placeholder public_url until we land on a concrete name in #129

Updated the name to routing-url

@JakeDawkins JakeDawkins linked an issue Dec 17, 2020 that may be closed by this pull request
@JakeDawkins JakeDawkins added feature 🎉 new commands, flags, functionality, and improved error messages blocked ⚠️ labels Dec 17, 2020
Comment on lines +67 to +70
(https://www.apollographql.com/docs/federation/managed-federation/overview/). If
You're running a service which hasn't been deployed yet or isn't using managed
federation, you may pass a placeholder url or leave the flag empty
(i.e. `--routing-url=""`).
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like something that would benefit from some guidance in Rover itself.

I propose that if they pass a --routing-url to a subgraph push invocation that doesn't need it (in the case of a non-deployed service, or unmanaged federation), Rover should return an error that lets them know that what they passed won't be used. I think this approach would eliminate some of the confusion our users may have about the usage of the flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think I missed Josh's original comment here (or just forgot since it's been a while), but it seems valuable here:

If they are pushing partial schemas to the registry then it will be managed federation. Unmanaged federation doesn't go through the registry.
Unmanaged federation directly get the subgraph schemas by querying the urls at runtime and fetching them via introspection and composing it directly in the gateway. The registry is bypassed in this mode.


If this is the case, then perhaps we don't need to mention unmanaged federation in this section of the docs? Perhaps my confusion is coming from the suggestion of the --routing-url="" here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EverlastingBugstopper There's no way for the registry or rover to know if a gateway is running in managed federation mode though unfortunately.

Managed federation is chosen at runtime on the gateway. it's just a matter of where the gateway is getting its config from: either a list of service urls or from studio. So there's no way for us to figure this out here ☹️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could make this flag optional, but it's not optional in the API. So we'd have to pass the empty string through to studio anyway

/// (often a deployed service). May be left empty ("") or a placeholder url
/// if not running a gateway in managed federation mode
#[structopt(long)]
routing_url: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably parse this in as a Url instead of a String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do this, but we'd have to make the flag optional in that case, so we have the ability to omit it.

@EverlastingBugstopper
Copy link
Contributor

It seems that the flag has been added to Rover's API surface in addition to docs (🥳 ), but I don't actually see any changes to rover-client. Do we need to actually use that value from structopt or is it already in there and I'm missing something?

@JakeDawkins JakeDawkins merged commit 657b7af into main Jan 12, 2021
@JakeDawkins JakeDawkins deleted the jake/push-url branch January 12, 2021 22:45
lrlna pushed a commit that referenced this pull request Jan 28, 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 this pull request may close these issues.

Add subgraph push option to specify endpoint
3 participants