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

fix(compose): follow-up to #2101 #2105

Merged
merged 1 commit into from
Sep 9, 2024
Merged

fix(compose): follow-up to #2101 #2105

merged 1 commit into from
Sep 9, 2024

Commits on Sep 9, 2024

  1. fix(compose): follow-up to #2101

    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 committed Sep 9, 2024
    Configuration menu
    Copy the full SHA
    f58e60d View commit details
    Browse the repository at this point in the history