Fix dev mode when you don't specify a configuration file #1808
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
bnjjj
requested review from
garypen,
Geal,
BrynCooke,
abernix and
o0Ignition0o
This comment has been minimized.
This comment has been minimized.
garypen
approved these changes
Sep 16, 2022
o0Ignition0o
approved these changes
Sep 16, 2022
BrynCooke
reviewed
Sep 16, 2022
| @@ -279,12 +279,20 @@ impl ConfigurationSource { | |||
| /// Convert this config into a stream regardless of if is static or not. Allows for unified handling later. | |||
| fn into_stream(self) -> impl Stream<Item = Event> { | |||
| match self { | |||
| ConfigurationSource::Static(instance) => { | |||
| ConfigurationSource::Static(mut instance) => { | |||
| if instance.dev.unwrap_or_default() { | |||
There was a problem hiding this comment.
Possibly this logic should be here: https://github.com/apollographql/router/blob/main/apollo-router/src/configuration/mod.rs#L368
@o0Ignition0o WDTY? You were looking at this recently.
There was a problem hiding this comment.
Hmm still weird to have a mutation in the validate method. But I can add this validate call to my tests for dev mode. We will be more safe
There was a problem hiding this comment.
Yeah, not the validate method.
But ConfigurationSource still feels like the wrong place. Checking
There was a problem hiding this comment.
Possibly in the constructor here before the call to validate: https://github.com/apollographql/router/blob/main/apollo-router/src/configuration/mod.rs#L260
Then we can get rid of the multiple times this is called in ConfigurationSource.
Also it means that dev mode is set up before validation takes place. Setting it after means we potentially miss validation errors that could be caused by the activation of dev mode.
There was a problem hiding this comment.
I will open an issue regarding this validation problem. I tried several solutions but each of them has an issue. I need to dig more into it. I will merge this PR to solve the root issue, we also have validate called in the tests so we're safe for now but I agree we should find a better way to implement it
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Merged
o0Ignition0o
pushed a commit
that referenced
this pull request
Sep 16, 2022
> **Note** > We're almost to 1.0! We've got a couple relatively small breaking changes to the configuration for this release (none to the API) that should be relatively easy to adapt to and a number of bug fixes and usability improvements. ## ❗ BREAKING ❗ ### Change `headers` propagation configuration ([PR #1795](#1795)) While it wasn't necessary today, we want to avoid a necessary breaking change in the future by proactively making room for up-and-coming work. We've therefore introduced another level into the `headers` configuration with a `request` object, to allow for a `response` (see [Issue #1284](#1284)) to be an _additive_ feature after 1.0. A rough look at this should just be a matter of adding in `request` and indenting everything that was inside it: ```patch headers: all: + request: - remove: named: "test" ``` The good news is that we'll have `response` in the future! For a full set of examples, please see the [header propagation documentation](https://www.apollographql.com/docs/router/configuration/header-propagation/). By @bnjjj in #1795 ### Bind the Sandbox on the same endpoint as the Supergraph, again ([Issue #1785](#1785)) We have rolled back an addition that we released in this week's `v1.0.0-rc.0` which allowed Sandbox (an HTML page that makes requests to the `supergraph` endpoint) to be on a custom socket. In retrospect, we believe it was premature to make this change without considering the broader impact of this change which ultimately touches on CORS and some developer experiences bits. Practically speaking, we may not want to introduce this because it complicates the model in a number of ways. For the foreseeable future, Sandbox will continue to be on the same listener address as the `supergraph` listener. It's unlikely anyone has really leaned into this much already, but if you've already re-configured `sandbox` or `homepage` to be on a custom `listen`-er and/or `path` in `1.0.0-rc.0`, here is a diff of what you should remove: ```diff sandbox: - listen: 127.0.0.1:4000 - path: / enabled: false homepage: - listen: 127.0.0.1:4000 - path: / enabled: true ``` Note this means you can either enable the `homepage`, or the `sandbox`, but not both. By @o0Ignition0o in #1796 ## 🚀 Features ### Automatically check "Return Query Plans from Router" checkbox in Sandbox ([Issue #1803](#1803)) When loading Sandbox, we now automatically configure it to toggle the "Request query plans from Router" checkbox to the enabled position which requests query plans from the Apollo Router when executing operations. These query plans are displayed in the Sandbox interface and can be seen by selecting "Query Plan Preview" from the drop-down above the panel on the right side of the interface. By @abernix in #1804 ## 🐛 Fixes ### Fix `--dev` mode when no configuration file is specified ([Issue #1801](#1801)) ([Issue #1802](#1802)) We've reconciled an issue where the `--dev` mode flag was being ignored when running the router without a configuration file. (While many use cases do require a configuration file, the Router actually doesn't _need_ a confguration in many cases!) By @bnjjj in #1808 ### Respect `supergraph`'s `path` for Kubernetes deployment probes ([Issue #1787](#1787)) If you've configured the `supergraph`'s `path` property using the Helm chart, the liveness and readiness probes now utilize these correctly. This fixes a bug where they continued to use the _default_ path of `/` and resulted in a startup failure. By @damienpontifex in #1788 ### Get variable default values from the query for query plan condition nodes ([PR #1640](#1640)) The query plan condition nodes, generated by the `if` argument of the `@defer` directive, were not using the default value of the variable passed in as an argument. This _also_ fixes _default value_ validations for non-`@defer`'d queries. By @Geal in #1640 ### Correctly hot-reload when changing the `supergraph`'s `listen` socket ([Issue #1814](#1814)) If you change the `supergraph`'s `listen` socket while in `--hot-reload` mode, the Router will now correctly pickup the change and bind to the new socket. By @o0Ignition0o in #1815 ## 🛠 Maintenance ### Improve error message when querying non existent field [Issue #1816](#1816) When querying a non-existent field you will get a better error message: ```patch { "errors": [ { - "message": "invalid type error, expected another type than 'Named type Computer'" + "message": "Cannot query field \"xxx\" on type \"Computer\"" } ] } ``` By @bnjjj in #1817 ### Update `apollo-router-scaffold` to use the published `apollo-router` crate [PR #1782](#1782) Now that `apollo-router` is released on [crates.io](https://crates.io/crates/apollo-router), we have updated the project scaffold to rely on the published crate instead of Git tags. By @o0Ignition0o in #1782 ### Refactor `Configuration` validation [Issue #1791](#1791) Instantiating `Configuration`s is now fallible, because it will run consistency checks on top of the already run structure checks. By @o0Ignition0o in #1794 ### Refactor response-formatting tests [#1798](#1798) Rewrite the response-formatting tests to use a builder pattern instead of macros and move the tests to a separate file. By @Geal in #1798 ## 📚 Documentation ### Add `rustdoc` documentation to various modules ([Issue #799](#799)) Adds documentation for: - `apollo-router/src/layers/instrument.rs` - `apollo-router/src/layers/map_first_graphql_response.rs` - `apollo-router/src/layers/map_future_with_request_data.rs` - `apollo-router/src/layers/sync_checkpoint.rs` - `apollo-router/src/plugin/serde.rs` - `apollo-router/src/tracer.rs` By @garypen in #1792 ### Fixed `docs.rs` publishing error from our last release During our last release we discovered for the first time that our documentation wasn't able to compile on the [docs.rs](https://docs.rs) website, leaving our documentation in a [failed state](https://docs.rs/crate/apollo-router/1.0.0-rc.0/builds/629200). While we've reconciled _that particular problem_, we're now being affected by [this](https://docs.rs/crate/router-bridge/0.1.7/builds/629895) internal compiler errors (ICE) that [is affecting](rust-lang/rust#101844) anyone using `1.65.0-nightly` builds circa today. Since docs.rs uses `nightly` for all builds, this means it'll be a few more days before we're published there. With thanks to @SimonSapin in apollographql/federation-rs#185 Co-authored-by: Coenen Benjamin <benjamin.coenen@hotmail.com>
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.
close #1801
close #1802