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

apollo-federation-types: merge routing_url in SupergraphConfig::merge_subgraphs #574

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

glasser
Copy link
Member

@glasser glasser commented Sep 5, 2024

This function was added a few months ago in #541 and as far as I know is only used in Rover for the supergraph compose and dev commands to combine configuration from --graph-ref and
--supergraph-config/--config (and the feature does not actually work for supergraph compose due to the issue fixed in apollographql/rover#2101).

The use case here is to let you run composition or dev against a GraphOS graph with some local changes, and so "I just want to change the SDL and nothing else" is a reasonable use case. This change means that if an override specifies SDL but no routing URL for a subgraph, Rover will continue to use the routing URL fetched from GraphOS for that subgraph, which seems reasonable.

While this is a "backwards incompatible" change, this crate is only intended for use by the Rover CLI and in the one case where this was called (rover dev --graph-ref REF --supergraph-config CONFIG) this seems like a clear improvement.

…e_subgraphs`

This function was added a few months ago in #541 and as far as I know is
only used in Rover for the `supergraph compose` and `dev` commands to
combine configuration from `--graph-ref` and
`--supergraph-config`/`--config` (and the feature does not actually work
for `supergraph compose` due to the issue fixed in
apollographql/rover#2101).

The use case here is to let you run composition or dev against a GraphOS
graph with some local changes, and so "I just want to change the SDL and
nothing else" is a reasonable use case. This change means that if an
override specifies SDL but no routing URL for a subgraph, Rover will
continue to use the routing URL fetched from GraphOS for that subgraph,
which seems reasonable.

While this is a "backwards incompatible" change, this crate is only
intended for use by the Rover CLI and in the one case where this was
called (`rover dev --graph-ref REF --supergraph-config CONFIG`) this
seems like a clear improvement.
@glasser glasser requested review from a team as code owners September 5, 2024 23:20
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Seems reasonable, has a test, and the test fails without the changes.

@tninesling tninesling merged commit 0ab853d into main Sep 6, 2024
12 checks passed
@tninesling tninesling deleted the glasser/merge-routing-url branch September 6, 2024 17:41
glasser added a commit to apollographql/rover that referenced this pull request Sep 6, 2024
This pulls in apollographql/federation-rs#574
which improves the logic used to merge remote and local config such that
if your local config does not specify a `routing_url` for a subgraph
that also exists in the remote config, the remote config's URL is used
instead of deleting it.

Update the docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants