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

Rover Dev Rewrite - Install Router Binary #2257

Merged
merged 6 commits into from
Nov 20, 2024
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
5 changes: 2 additions & 3 deletions .cargo/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,5 @@
xtask = "run --package xtask --"
rover = "run --package rover --"
install-rover = "run --release --package rover -- install --force"
test-next = "test --all-features"
build-next = "build --features composition-js,dev-next"

test-next = "test --all-features -- --nocapture"
Copy link
Contributor

Choose a reason for hiding this comment

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

double-checking that you're wanting that --nocapture

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. This was used as a binding in my editor and is to make sure we got traces logged right.

build-next = "build --features composition-js,dev-next"
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -244,3 +244,4 @@ rstest = { workspace = true }
serial_test = { workspace = true }
speculoos = { workspace = true }
tracing-test = "0.2.5"
temp-env = { version = "0.3.6", features = ["async_closure"] }
17 changes: 17 additions & 0 deletions src/command/dev/next/router/binary.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
use camino::Utf8PathBuf;
use semver::Version;

#[derive(Clone, Debug)]
#[cfg_attr(test, derive(derive_getters::Getters))]
pub struct RouterBinary {
#[allow(unused)]
exe: Utf8PathBuf,
#[allow(unused)]
version: Version,
}

impl RouterBinary {
pub fn new(exe: Utf8PathBuf, version: Version) -> RouterBinary {
RouterBinary { exe, version }
}
}
Comment on lines +13 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

would we want to use Builder here instead? it seems like an emerging convention

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My litmus test has been that more than two arguments equates to wanting a builder. Otherwise, a constructor is usually good enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

sg, seems sensible

251 changes: 251 additions & 0 deletions src/command/dev/next/router/install.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,251 @@
use apollo_federation_types::config::RouterVersion;
use async_trait::async_trait;
use camino::{Utf8Path, Utf8PathBuf};
use semver::Version;

use crate::{
command::{install::Plugin, Install},
options::LicenseAccepter,
utils::{client::StudioClientConfig, effect::install::InstallBinary},
};

use super::binary::RouterBinary;

#[derive(thiserror::Error, Debug)]
#[error("Failed to install the router")]
pub enum InstallRouterError {
#[error("unable to find dependency: \"{err}\"")]
MissingDependency {
/// The error while attempting to find the dependency
err: String,
},
#[error("Missing filename for path: {path}")]
MissingFilename { path: Utf8PathBuf },
#[error("Invalid semver version: \"{input}\"")]
Semver {
input: String,
source: semver::Error,
},
}

pub struct InstallRouter {
studio_client_config: StudioClientConfig,
router_version: RouterVersion,
}

impl InstallRouter {
#[allow(unused)]
pub fn new(
router_version: RouterVersion,
studio_client_config: StudioClientConfig,
) -> InstallRouter {
InstallRouter {
router_version,
studio_client_config,
}
}
}
Comment on lines +37 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

same question about using Builder


#[async_trait]
impl InstallBinary for InstallRouter {
type Binary = RouterBinary;
type Error = InstallRouterError;
async fn install(
&self,
override_install_path: Option<Utf8PathBuf>,
elv2_license_accepter: LicenseAccepter,
skip_update: bool,
) -> Result<Self::Binary, Self::Error> {
Comment on lines +53 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

doublechecking: override_install_path and skip_update are the only cli options we need to support? that's my read of the help for dev

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's my read as well.

let plugin = Plugin::Router(self.router_version.clone());
let install_command = Install {
force: false,
plugin: Some(plugin),
elv2_license_accepter,
};
let exe = install_command
.get_versioned_plugin(
override_install_path,
self.studio_client_config.clone(),
skip_update,
)
.await
.map_err(|err| InstallRouterError::MissingDependency {
err: err.to_string(),
})?;
let version = version_from_path(&exe)?;
let binary = RouterBinary::new(exe, version);
Ok(binary)
}
}

fn version_from_path(path: &Utf8Path) -> Result<Version, InstallRouterError> {
let file_name = path
.file_name()
.ok_or_else(|| InstallRouterError::MissingFilename {
path: path.to_path_buf(),
})?;
let without_exe = file_name.strip_suffix(".exe").unwrap_or(file_name);
let without_prefix = without_exe.strip_prefix("router-v").unwrap_or(without_exe);
let version = Version::parse(without_prefix).map_err(|err| InstallRouterError::Semver {
input: without_prefix.to_string(),
source: err,
})?;
Ok(version)
}

#[cfg(test)]
mod tests {
use std::{str::FromStr, time::Duration};

use anyhow::Result;
use apollo_federation_types::config::RouterVersion;
use assert_fs::{NamedTempFile, TempDir};
use camino::Utf8PathBuf;
use flate2::{write::GzEncoder, Compression};
use houston::Config;
use http::Method;
use httpmock::MockServer;
use rstest::{fixture, rstest};
use semver::Version;
use speculoos::prelude::*;
use tracing_test::traced_test;

use crate::{
options::LicenseAccepter,
utils::{
client::{ClientBuilder, StudioClientConfig},
effect::install::InstallBinary,
},
};

use super::InstallRouter;

#[fixture]
#[once]
fn http_server() -> MockServer {
MockServer::start()
}

#[fixture]
#[once]
fn mock_server_endpoint(http_server: &MockServer) -> String {
let address = http_server.address();
let endpoint = format!("http://{}", address);
endpoint
}

#[fixture]
#[once]
fn home() -> TempDir {
TempDir::new().unwrap()
}

#[fixture]
fn router_version() -> RouterVersion {
RouterVersion::Latest
}

#[fixture]
#[once]
fn api_key() -> String {
"api-key".to_string()
}

#[fixture]
fn config(api_key: &str, home: &TempDir) -> Config {
let home = Utf8PathBuf::from_path_buf(home.to_path_buf()).unwrap();
Config {
home,
override_api_key: Some(api_key.to_string()),
}
}

#[fixture]
fn studio_client_config(mock_server_endpoint: &str, config: Config) -> StudioClientConfig {
StudioClientConfig::new(
Some(mock_server_endpoint.to_string()),
config,
false,
ClientBuilder::default(),
None,
)
}

#[traced_test]
#[tokio::test]
#[rstest]
#[timeout(Duration::from_secs(15))]
async fn test_install(
router_version: RouterVersion,
studio_client_config: StudioClientConfig,
http_server: &MockServer,
mock_server_endpoint: &str,
) -> Result<()> {
let license_accepter = LicenseAccepter {
elv2_license_accepted: Some(true),
};
let override_install_path = NamedTempFile::new("override_path")?;
let install_router = InstallRouter::new(router_version, studio_client_config);
http_server.mock(|when, then| {
when.matches(|request| {
request.method == Method::HEAD.to_string()
&& request.path.starts_with("/tar/router")
});
then.status(302).header("X-Version", "v1.57.1");
});
http_server.mock(|when, then| {
when.matches(|request| {
request.method == Method::GET.to_string()
&& request.path.starts_with("/tar/router/")
});
then.status(302)
.header("Location", format!("{}/router/", mock_server_endpoint));
});

let enc = GzEncoder::new(Vec::new(), Compression::default());
let mut archive = tar::Builder::new(enc);
let contents = b"router";
let mut header = tar::Header::new_gnu();
header.set_path("dist/router")?;
header.set_size(contents.len().try_into().unwrap());
header.set_cksum();
archive.append(&header, &contents[..]).unwrap();

let finished_archive = archive.into_inner()?;
let finished_archive_bytes = finished_archive.finish()?;

http_server.mock(|when, then| {
when.matches(|request| {
request.method == Method::GET.to_string() && request.path.starts_with("/router")
});
then.status(200)
.header("Content-Type", "application/octet-stream")
.body(&finished_archive_bytes);
});
let binary = temp_env::async_with_vars(
[("APOLLO_ROVER_DOWNLOAD_HOST", Some(mock_server_endpoint))],
async {
install_router
.install(
Utf8PathBuf::from_path_buf(override_install_path.to_path_buf()).ok(),
license_accepter,
false,
)
.await
},
)
.await;
let subject = assert_that!(binary).is_ok().subject;
assert_that!(subject.version()).is_equal_to(&Version::from_str("1.57.1")?);

let installed_binary_path = override_install_path
.path()
.join(".rover/bin/router-v1.57.1");
assert_that!(subject.exe())
.is_equal_to(&Utf8PathBuf::from_path_buf(installed_binary_path.clone()).unwrap());
assert_that!(installed_binary_path.exists()).is_equal_to(true);
let installed_binary_contents = std::fs::read(installed_binary_path)?;
assert_that!(installed_binary_contents).is_equal_to(b"router".to_vec());
Ok(())
}
}
2 changes: 2 additions & 0 deletions src/command/dev/next/router/mod.rs
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
pub mod binary;
pub mod config;
pub mod install;
40 changes: 17 additions & 23 deletions src/command/supergraph/compose/do_compose.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,15 @@ use crate::{
events::CompositionEvent,
runner::Runner,
supergraph::{
binary::{InstallSupergraph, InstallSupergraphBinary, OutputTarget, SupergraphBinary},
binary::{OutputTarget, SupergraphBinary},
config::{
resolve::{
subgraph::FullyResolvedSubgraph, FullyResolvedSubgraphs,
FullyResolvedSupergraphConfig, UnresolvedSupergraphConfig,
},
SupergraphConfigResolver,
},
install::InstallSupergraph,
version::SupergraphVersion,
},
},
Expand All @@ -57,6 +58,7 @@ use crate::{
effect::{
exec::TokioCommand,
fetch_remote_subgraph::RemoteSubgraph,
install::InstallBinary,
read_file::FsReadFile,
write_file::{FsWriteFile, WriteFile},
},
Expand Down Expand Up @@ -260,31 +262,23 @@ impl Compose {
}

// Build the supergraph binary, paying special attention to the CLI options
let supergraph_binary = InstallSupergraph::builder()
.federation_version(fed_version.clone())
.elv2_license_accepter(self.opts.plugin_opts.elv2_license_accepter)
.studio_client_config(client_config.clone())
.and_override_install_path(override_install_path)
.skip_update(self.opts.plugin_opts.skip_update)
.build()
.install()
let supergraph_binary = InstallSupergraph::new(fed_version.clone(), client_config.clone())
.install(
override_install_path,
self.opts.plugin_opts.elv2_license_accepter,
self.opts.plugin_opts.skip_update,
)
.await?;

// Federation versions and supergraph versions are synonymous, but have different types
// representing them depending on their use (eg, we only ever have an exact
// SupergraphVersion because we can only ever use an exact version of the binary, where the
// FederationVersion can just point to latest to get the latest version--these are similar,
// but represent different ways of using the underlying version)
let supergraph_version: SupergraphVersion = fed_version.clone().try_into()?;
Comment on lines -273 to -278
Copy link
Contributor

Choose a reason for hiding this comment

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

do you know for sure that using fed_version.clone() above is going to get the right version from the supergraph binary's perspective?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. The process is this:

  1. Get a FederationVersion (which may be some kind of "latest")
  2. Download the binary using the specified argument
  3. Extract the exact version from the supergraph binary name
  4. We now have an exact version, which is slightly different from a FederationVersion, since it's a exact semantic version.


let supergraph_binary = SupergraphBinary::builder()
.exe(supergraph_binary)
.output_target(output_file)
.version(supergraph_version)
.build();

let result = supergraph_binary
.compose(&exec_command, &read_file, supergraph_config_filepath)
.compose(
&exec_command,
&read_file,
&output_file
.map(|path| OutputTarget::File(path))
.unwrap_or_else(|| OutputTarget::Stdout),
supergraph_config_filepath,
)
.await?;

Ok(RoverOutput::CompositionResult(result.into()))
Expand Down
Loading
Loading