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 dev mode when you don't specify a configuration file #1808

Merged
merged 5 commits into from
Sep 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions NEXT_CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,20 +72,26 @@ By [@o0Ignition0o](https://github.com/o0Ignition0o) in https://github.com/apollo

## 🚀 Features

### Automatically check "Return Query Plans from Router" checkbox in Sandbox ([Issue #](https://github.com/apollographql/router/issues/1803))
### Automatically check "Return Query Plans from Router" checkbox in Sandbox ([Issue #1803](https://github.com/apollographql/router/issues/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](https://github.com/abernix) in https://github.com/apollographql/router/pull/1804

## 🐛 Fixes

### Respect supergraph path for kubernetes deployment probes (#1787)
### Fix dev mode when you don't specify a configuration file ([Issue #1801](https://github.com/apollographql/router/issues/1801)) ([Issue #1802](https://github.com/apollographql/router/issues/1802))

Previously the dev mode was ignored if you ran the router without a configuration file.

By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/1808

### Respect supergraph path for kubernetes deployment probes ([Issue #1787](https://github.com/apollographql/router/issues/1787))

For cases where you configured the `supergraph.path` for the router when using the helm chart, the liveness
and readiness probes continued to use the default path of `/` and so the start failed.

By @damienpontifex in #1788
By [@damienpontifex](https://github.com/damienpontifex) in https://github.com/apollographql/router/pull/1788


## 🛠 Maintenance
Expand Down
4 changes: 2 additions & 2 deletions apollo-router/src/configuration/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ pub struct Configuration {

// Dev mode
#[serde(skip)]
dev: Option<bool>,
pub(crate) dev: Option<bool>,
}

impl<'de> serde::Deserialize<'de> for Configuration {
Expand Down Expand Up @@ -365,7 +365,7 @@ impl Configuration {
}

impl Configuration {
fn validate(self) -> Result<Self, ConfigurationError> {
pub(crate) fn validate(self) -> Result<Self, ConfigurationError> {
// Sandbox and Homepage cannot be both enabled
if self.sandbox.enabled && self.homepage.enabled {
return Err(ConfigurationError::InvalidConfiguration {
Expand Down
45 changes: 41 additions & 4 deletions apollo-router/src/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, not the validate method.
But ConfigurationSource still feels like the wrong place. Checking

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

instance.enable_dev_mode();
}
stream::iter(vec![UpdateConfiguration(instance)]).boxed()
}
ConfigurationSource::Stream(stream) => {
stream.map(|x| UpdateConfiguration(Box::new(x))).boxed()
}
ConfigurationSource::Stream(stream) => stream
.map(|mut x| {
if x.dev.unwrap_or_default() {
x.enable_dev_mode();
}
UpdateConfiguration(Box::new(x))
})
.boxed(),
ConfigurationSource::File {
path,
watch,
Expand Down Expand Up @@ -731,6 +739,7 @@ mod tests {
_ => panic!("the event from the stream must be UpdateConfiguration"),
};
assert!(cfg.supergraph.introspection);
assert!(!cfg.homepage.enabled);
assert!(cfg.sandbox.enabled);
assert!(cfg.plugins().iter().any(
|(name, val)| name == "experimental.expose_query_plan" && val == &Value::Bool(true)
Expand All @@ -740,6 +749,7 @@ mod tests {
.iter()
.any(|(name, val)| name == "apollo.include_subgraph_errors"
&& val == &json!({"all": true})));
cfg.validate().unwrap();

// Modify the file and try again
write_and_flush(&mut file, contents).await;
Expand All @@ -748,6 +758,7 @@ mod tests {
_ => panic!("the event from the stream must be UpdateConfiguration"),
};
assert!(cfg.supergraph.introspection);
assert!(!cfg.homepage.enabled);
assert!(cfg.sandbox.enabled);
assert!(cfg.plugins().iter().any(
|(name, val)| name == "experimental.expose_query_plan" && val == &Value::Bool(true)
Expand All @@ -757,12 +768,38 @@ mod tests {
.iter()
.any(|(name, val)| name == "apollo.include_subgraph_errors"
&& val == &json!({"all": true})));
cfg.validate().unwrap();

// This time write garbage, there should not be an update.
write_and_flush(&mut file, ":garbage").await;
assert!(stream.into_future().now_or_never().is_none());
}

#[tokio::test(flavor = "multi_thread")]
async fn config_dev_mode_without_file() {
let mut stream =
ConfigurationSource::from(Configuration::builder().dev(true).build().unwrap())
.into_stream()
.boxed();

let cfg = match stream.next().await.unwrap() {
UpdateConfiguration(configuration) => configuration,
_ => panic!("the event from the stream must be UpdateConfiguration"),
};
assert!(cfg.supergraph.introspection);
assert!(cfg.sandbox.enabled);
assert!(!cfg.homepage.enabled);
assert!(cfg.plugins().iter().any(
|(name, val)| name == "experimental.expose_query_plan" && val == &Value::Bool(true)
));
assert!(cfg
.plugins()
.iter()
.any(|(name, val)| name == "apollo.include_subgraph_errors"
&& val == &json!({"all": true})));
cfg.validate().unwrap();
}

#[tokio::test(flavor = "multi_thread")]
async fn config_by_file_invalid() {
let (path, mut file) = create_temp_file();
Expand Down
4 changes: 2 additions & 2 deletions apollo-router/src/router_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,8 @@ async fn create_plugins(
.iter()
.map(|(name, plugin)| (name, plugin.name()))
.collect::<Vec<(&String, &str)>>();
tracing::info!(
"enabled plugins: {:?}",
tracing::debug!(
"plugins list: {:?}",
plugin_details
.iter()
.map(|(name, _)| name)
Expand Down