fix(compose): follow-up to #2101 #2105
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glasser
commented
Sep 6, 2024
| let mut resolved_supergraph_config: SupergraphConfig = subgraph_definitions.into(); | ||
|
|
||
| let mut fed_two_subgraph_names = Vec::new(); | ||
| for subgraph_definition in resolved_supergraph_config.get_subgraph_definitions()? { |
There was a problem hiding this comment.
This call to get_subgraph_definitions() returns error if anything in the config lacks a routing URL. I could have just changed this to iterate over the SubgraphConfigs but I would have to do an assertion that every schema source was of type Sdl. So I thought it would be simpler to just do the parsing on the SDL before we put it in the SupergraphConfig instead of afterwards. (Technically this does mean that some error cases we do some extra work before returning the error that we didn't do before.)
jonathanrainer
approved these changes
Sep 9, 2024
jonathanrainer
force-pushed
the
glasser/fix-2101
branch
from
eb362fe
to
d528b91
Compare
dotdat
approved these changes
Sep 9, 2024
The improvements in #2101 did not quite work in all cases, due to some "enhancements" that the `resolve_supergraph_yaml` (which reads the `--config` file) applies which are overzealous in the case where the YAML file is an "override" to a graph ref. First, `resolve_supergraph_yaml` sets a default `federation_version` (based on whether or not any subgraph SDLs contain the `@link` directive) if one is not provided. The YAML file's federation version correctly takes precedence over a remote `federation_version`, which means it's not appropriate for us to calculate this default if the file lacks a `federation_version` but we have one from another source. This PR adds a parameter to `resolve_supergraph_yaml` which suppresses this defaulting in the case where you have also specified `--graph-ref` and read a federation version from GraphOS (or if you've specified it explicitly on the command line: previously this case "worked" in that the CLI option took precedence, but it would print a warning first telling you to specify the version). Secondly, we want to no longer treat a lack of a `routing_url` for a subgraph in the YAML file as an error, so that the improvement in #2101 which lets you merge local SubgraphConfigs with `routing_url: None` with remote SubgraphConfigs that have routing URLs works. But `resolve_supergraph_yaml` required every subgraph definition to have a routing URL (and in fact used the `SubgraphDefinition` struct as an intermediate data type, which requires the routing URL to exist). We refactor to no longer use that as an intermediate data type so that it's OK for the routing URL to still be None (like it is in the `SupergraphConfig` type that the function returns). (You may wonder if it's safe that the `SupergraphConfig` returned from `resolve_supergraph_yaml` can now have None in its routing URLs when that was not possible before. But it already was the case that a graph read from GraphOS lacked routing URLs; this requirement was only enforced here for local configuration. The lack of a routing URL gets caught later and turned into an error when `Compose::exec` calls `supergraph_config.get_subgraph_definitions()`.)
glasser
force-pushed
the
glasser/fix-2101
branch
from
d528b91
to
f58e60d
Compare
Merged
jonathanrainer
added a commit
that referenced
this pull request
Sep 10, 2024
## 🐛 Fixes - **Avoid misleading warning when `--output` is not specified - @glasser #2100** In the release of v0.26.1 logic was added to disable the output flag if the Federation version was less than 2.9, however this was being printed even when the `--output` flag was not supplied. This has been corrected. - **Improve `--graph-ref` option - @glasser #2101** In the release of v0.26.0 the `--graph-ref` option was added to `supergraph compose` as well as `rover dev`. However, the behaviour when `--graph-ref` was used in conjunction with `--config` did not work as documented. This is now fixed. Furthermore, both `rover dev` and `supergraph compose`, when using only the `--graph-ref` option, respect the graph ref's Federation version. - **Further improve `--graph-ref` option - @glasser #2105** Improves on the above by fixing some corner cases that prevented #2101 from working as intended ## 🛠 Maintenance - **Update `eslint` to v9.10.0 - @jonathanrainer #2106** - **Update `concurrently` to v9.0.0 - @jonathanrainer #2108** - **Update `manylinux` CI Docker Image to v2024.09.09 - @jonathanrainer #2110** - **Update Rust to v1.81.0 - @jonathanrainer #2107** - **Pass GitHub Tag to GitHub Actions Workflow - @glasser #2109** - **Add `tower` for use with HTTP/GraphQL clients - @dotdat #2067** ## 📚 Documentation - **Fix Glossary links - @Meschreiber @pnodet #2114**
aaronArinder
pushed a commit
that referenced
this pull request
Sep 10, 2024
- **Avoid misleading warning when `--output` is not specified - @glasser In the release of v0.26.1 logic was added to disable the output flag if the Federation version was less than 2.9, however this was being printed even when the `--output` flag was not supplied. This has been corrected. - **Improve `--graph-ref` option - @glasser #2101** In the release of v0.26.0 the `--graph-ref` option was added to `supergraph compose` as well as `rover dev`. However, the behaviour when `--graph-ref` was used in conjunction with `--config` did not work as documented. This is now fixed. Furthermore, both `rover dev` and `supergraph compose`, when using only the `--graph-ref` option, respect the graph ref's Federation version. - **Further improve `--graph-ref` option - @glasser #2105** Improves on the above by fixing some corner cases that prevented #2101 from working as intended - **Update `eslint` to v9.10.0 - @jonathanrainer #2106** - **Update `concurrently` to v9.0.0 - @jonathanrainer #2108** - **Update `manylinux` CI Docker Image to v2024.09.09 - @jonathanrainer - **Update Rust to v1.81.0 - @jonathanrainer #2107** - **Pass GitHub Tag to GitHub Actions Workflow - @glasser #2109** - **Add `tower` for use with HTTP/GraphQL clients - @dotdat #2067** - **Fix Glossary links - @Meschreiber @pnodet #2114**
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The improvements in #2101 did not quite work in all cases, due to some "enhancements" that the
resolve_supergraph_yaml(which reads the--configfile) applies which are overzealous in the case where the YAML file is an "override" to a graph ref.First,
resolve_supergraph_yamlsets a defaultfederation_version(based on whether or not any subgraph SDLs contain the@linkdirective) if one is not provided. The YAML file's federation version correctly takes precedence over a remotefederation_version, which means it's not appropriate for us to calculate this default if the file lacks afederation_versionbut we have one from another source. This PR adds a parameter toresolve_supergraph_yamlwhich suppresses this defaulting in the case where you have also specified--graph-refand read a federation version from GraphOS (or if you've specified it explicitly on the command line: previously this case "worked" in that the CLI option took precedence, but it would print a warning first telling you to specify the version).Secondly, we want to no longer treat a lack of a
routing_urlfor a subgraph in the YAML file as an error, so that the improvement in #2101 which lets you merge local SubgraphConfigs withrouting_url: Nonewith remote SubgraphConfigs that have routing URLs works. Butresolve_supergraph_yamlrequired every subgraph definition to have a routing URL (and in fact used theSubgraphDefinitionstruct as an intermediate data type, which requires the routing URL to exist). We refactor to no longer use that as an intermediate data type so that it's OK for the routing URL to still be None (like it is in theSupergraphConfigtype that the function returns).(You may wonder if it's safe that the
SupergraphConfigreturned fromresolve_supergraph_yamlcan now have None in its routing URLs when that was not possible before. But it already was the case that a graph read from GraphOS lacked routing URLs; this requirement was only enforced here for local configuration. The lack of a routing URL gets caught later and turned into an error whenCompose::execcallssupergraph_config.get_subgraph_definitions().)