-
Notifications
You must be signed in to change notification settings - Fork 94
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
Bring dev
launch to parity with legacy
#2319
Changes from all commits
2e05b62
34739df
ce009e8
be295ba
40adce6
347daad
fdab439
8af0bdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
use std::fmt::{Display, Formatter}; | ||
use std::net::{IpAddr, Ipv4Addr, SocketAddr}; | ||
|
||
use buildstructor::buildstructor; | ||
|
@@ -6,12 +7,11 @@ use http::Uri; | |
use rover_std::{Fs, RoverStdError}; | ||
use thiserror::Error; | ||
|
||
use crate::utils::effect::read_file::ReadFile; | ||
|
||
use self::{ | ||
parser::{ParseRouterConfigError, RouterConfigParser}, | ||
state::{RunRouterConfigDefault, RunRouterConfigFinal, RunRouterConfigReadConfig}, | ||
}; | ||
use crate::utils::effect::read_file::ReadFile; | ||
|
||
mod parser; | ||
pub mod remote; | ||
|
@@ -54,6 +54,19 @@ impl RouterAddress { | |
} | ||
} | ||
|
||
impl Display for RouterAddress { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
let host = self | ||
.host | ||
.to_string() | ||
.replace("127.0.0.1", "localhost") | ||
.replace("0.0.0.0", "localhost") | ||
.replace("[::]", "localhost") | ||
.replace("[::1]", "localhost"); | ||
write!(f, "http://{}:{}", host, self.port) | ||
} | ||
} | ||
|
||
Comment on lines
+57
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic is directly copied over from the old version of the log line that says "your supergraph is ready" etc. |
||
impl Default for RouterAddress { | ||
fn default() -> Self { | ||
RouterAddress { | ||
|
@@ -178,6 +191,10 @@ impl RunRouterConfig<RunRouterConfigFinal> { | |
&self.state.health_check_endpoint | ||
} | ||
|
||
pub fn raw_config(&self) -> String { | ||
self.state.raw_config.clone() | ||
} | ||
|
||
#[allow(unused)] | ||
pub fn router_config(&self) -> RouterConfig { | ||
RouterConfig(self.state.raw_config.to_string()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,7 +84,7 @@ impl<'a> RouterConfigParser<'a> { | |
.and_then(|addr_and_port| Some(Uri::from_str(addr_and_port))) | ||
// See https://www.apollographql.com/docs/graphos/routing/self-hosted/health-checks for | ||
// defaults | ||
.unwrap_or(Uri::from_str("127.0.0.1:8088")) | ||
.unwrap_or(Uri::from_str("http://127.0.0.1:8088")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. URIs need a scheme in order to be correct, so added this in for the default case |
||
.map_err(|err| ParseRouterConfigError::ListenPath { | ||
path: "health_check.listen", | ||
source: err, | ||
|
@@ -161,8 +161,8 @@ health_check: | |
); | ||
let config_yaml = serde_yaml::from_str(&config_yaml_str)?; | ||
let router_config = RouterConfigParser { yaml: &config_yaml }; | ||
let health_check = router_config.health_check(); | ||
assert_that!(health_check).is_equal_to(is_health_check_enabled); | ||
let health_check = router_config.health_check_endpoint(); | ||
assert_that!(health_check.is_ok()).is_equal_to(is_health_check_enabled); | ||
Comment on lines
+164
to
+165
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tried to fix up these tests because they seemed to have rotted. If this wasn't the intent of either test, let me know and I'll get them fixed up |
||
Ok(()) | ||
} | ||
|
||
|
@@ -174,9 +174,8 @@ health_check: | |
}; | ||
let config_yaml = serde_yaml::from_str(&config_yaml_str)?; | ||
let router_config = RouterConfigParser { yaml: &config_yaml }; | ||
let health_check = router_config.health_check(); | ||
assert_that!(health_check).is_false(); | ||
Ok(()) | ||
let health_check = router_config.health_check_endpoint(); | ||
assert_that!(health_check).is_equal_to(Ok(Uri::from_str("http://127.0.0.1:8088/health")?)) | ||
} | ||
|
||
#[rstest] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,12 +11,19 @@ use rover_client::{ | |
operations::config::who_am_i::{RegistryIdentity, WhoAmIError, WhoAmIRequest}, | ||
shared::GraphRef, | ||
}; | ||
use rover_std::RoverStdError; | ||
use rover_std::{infoln, RoverStdError}; | ||
use tokio::{process::Child, time::sleep}; | ||
use tokio_stream::wrappers::UnboundedReceiverStream; | ||
use tower::{Service, ServiceExt}; | ||
use tracing::info; | ||
|
||
use super::{ | ||
binary::{RouterLog, RunRouterBinary, RunRouterBinaryError}, | ||
config::{remote::RemoteRouterConfig, ReadRouterConfigError, RouterAddress, RunRouterConfig}, | ||
hot_reload::{HotReloadEvent, HotReloadWatcher, RouterUpdateEvent}, | ||
install::{InstallRouter, InstallRouterError}, | ||
watchers::router_config::RouterConfigWatcher, | ||
}; | ||
use crate::{ | ||
command::dev::next::FileWatcher, | ||
composition::events::CompositionEvent, | ||
|
@@ -33,16 +40,8 @@ use crate::{ | |
}, | ||
}; | ||
|
||
use super::{ | ||
binary::{RouterLog, RunRouterBinary, RunRouterBinaryError}, | ||
config::{remote::RemoteRouterConfig, ReadRouterConfigError, RouterAddress, RunRouterConfig}, | ||
hot_reload::{HotReloadEvent, HotReloadWatcher, RouterUpdateEvent}, | ||
install::{InstallRouter, InstallRouterError}, | ||
watchers::router_config::RouterConfigWatcher, | ||
}; | ||
|
||
pub struct RunRouter<S> { | ||
state: S, | ||
pub(crate) state: S, | ||
} | ||
|
||
impl Default for RunRouter<state::Install> { | ||
|
@@ -86,6 +85,12 @@ impl RunRouter<state::LoadLocalConfig> { | |
.with_address(router_address) | ||
.with_config(read_file_impl, config_path.as_ref()) | ||
.await?; | ||
if let Some(config_path) = config_path.clone() { | ||
infoln!( | ||
"Watching {} for changes", | ||
config_path.as_std_path().display() | ||
); | ||
} | ||
Ok(RunRouter { | ||
state: state::LoadRemoteConfig { | ||
binary: self.state.binary, | ||
|
@@ -158,6 +163,7 @@ impl RunRouter<state::Run> { | |
.call( | ||
WriteFileRequest::builder() | ||
.path(hot_reload_config_path.clone()) | ||
.contents(Vec::from(self.state.config.raw_config())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it turns out we weren't actually passing anything through to the router config, so I had to fix that up to get rid of lots of telemetry errors etc. |
||
.build(), | ||
) | ||
.await | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,18 +6,6 @@ use rover_client::shared::GraphRef; | |
use tempfile::tempdir; | ||
use tower::MakeService; | ||
|
||
use crate::{ | ||
options::{LicenseAccepter, ProfileOpt}, | ||
utils::{ | ||
client::StudioClientConfig, | ||
effect::{ | ||
exec::ExecCommand, install::InstallBinary, introspect::IntrospectSubgraph, | ||
read_file::ReadFile, read_stdin::ReadStdin, write_file::WriteFile, | ||
}, | ||
parsers::FileDescriptorType, | ||
}, | ||
}; | ||
|
||
use super::{ | ||
runner::{CompositionRunner, Runner}, | ||
supergraph::{ | ||
|
@@ -32,6 +20,18 @@ use super::{ | |
}, | ||
CompositionError, CompositionSuccess, | ||
}; | ||
use crate::composition::pipeline::CompositionPipelineError::FederationOneWithFederationTwoSubgraphs; | ||
use crate::{ | ||
options::{LicenseAccepter, ProfileOpt}, | ||
utils::{ | ||
client::StudioClientConfig, | ||
effect::{ | ||
exec::ExecCommand, install::InstallBinary, introspect::IntrospectSubgraph, | ||
read_file::ReadFile, read_stdin::ReadStdin, write_file::WriteFile, | ||
}, | ||
parsers::FileDescriptorType, | ||
}, | ||
}; | ||
|
||
#[derive(thiserror::Error, Debug)] | ||
pub enum CompositionPipelineError { | ||
|
@@ -52,10 +52,12 @@ pub enum CompositionPipelineError { | |
}, | ||
#[error("Failed to install the supergraph binary.\n{}", .0)] | ||
InstallSupergraph(#[from] InstallSupergraphError), | ||
#[error("Federation 1 version specified, but supergraph schema includes Federation 2 subgraphs: {0:?}")] | ||
FederationOneWithFederationTwoSubgraphs(Vec<String>), | ||
} | ||
|
||
pub struct CompositionPipeline<State> { | ||
state: State, | ||
pub(crate) state: State, | ||
} | ||
|
||
impl Default for CompositionPipeline<state::Init> { | ||
|
@@ -98,10 +100,12 @@ impl CompositionPipeline<state::Init> { | |
} | ||
FileDescriptorType::Stdin => None, | ||
}); | ||
eprintln!("merging supergraph schema files"); | ||
let resolver = SupergraphConfigResolver::default() | ||
.load_remote_subgraphs(fetch_remote_subgraphs_factory, graph_ref.as_ref()) | ||
.await? | ||
.load_from_file_descriptor(read_stdin_impl, supergraph_yaml.as_ref())?; | ||
eprintln!("supergraph config loaded successfully"); | ||
Ok(CompositionPipeline { | ||
state: state::ResolveFederationVersion { | ||
resolver, | ||
|
@@ -134,11 +138,27 @@ impl CompositionPipeline<state::ResolveFederationVersion> { | |
&SubgraphPrompt::default(), | ||
) | ||
.await?; | ||
let federation_version = federation_version.unwrap_or_else(|| { | ||
let fed_two_subgraphs = fully_resolved_supergraph_config | ||
.subgraphs() | ||
.iter() | ||
.filter_map(|(name, subgraph)| { | ||
if subgraph.is_fed_two { | ||
Some(name.clone()) | ||
} else { | ||
None | ||
} | ||
}) | ||
.collect::<Vec<String>>(); | ||
let federation_version = if let Some(fed_version) = federation_version { | ||
if !fed_two_subgraphs.is_empty() && fed_version.is_fed_one() { | ||
return Err(FederationOneWithFederationTwoSubgraphs(fed_two_subgraphs)); | ||
} | ||
fed_version | ||
} else { | ||
Comment on lines
+141
to
+157
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We scan quite early for the mismatch in Fed Versions so we can bail out early on |
||
fully_resolved_supergraph_config | ||
.federation_version() | ||
.clone() | ||
}); | ||
}; | ||
Ok(CompositionPipeline { | ||
state: state::InstallSupergraph { | ||
resolver: self.state.resolver, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this here, as otherwise it seems to default to the version on the subgraphs and from the test plan that seems to not be what we want. Happy to change if that is the case though