-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat(compose): when --output
is used, use --output
in supergraph bin
#2045
Changes from all commits
7b4d970
ce7ab03
7e152ea
24a5530
b2c4cab
774e260
8b7a33d
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 |
---|---|---|
|
@@ -9,7 +9,9 @@ use apollo_federation_types::{ | |
}; | ||
use camino::Utf8PathBuf; | ||
use clap::{Args, Parser}; | ||
use semver::Version; | ||
use serde::Serialize; | ||
use std::io::Read; | ||
|
||
use rover_client::shared::GraphRef; | ||
use rover_client::RoverClientError; | ||
|
@@ -112,6 +114,7 @@ impl Compose { | |
&self, | ||
override_install_path: Option<Utf8PathBuf>, | ||
client_config: StudioClientConfig, | ||
output_file: Option<Utf8PathBuf>, | ||
) -> RoverResult<RoverOutput> { | ||
let mut supergraph_config = get_supergraph_config( | ||
&self.opts.supergraph_config_source.graph_ref, | ||
|
@@ -125,18 +128,29 @@ impl Compose { | |
// WARNING: remove this unwrap | ||
.unwrap(); | ||
|
||
self.compose(override_install_path, client_config, &mut supergraph_config) | ||
.await | ||
self.compose( | ||
override_install_path, | ||
client_config, | ||
&mut supergraph_config, | ||
output_file, | ||
) | ||
.await | ||
} | ||
|
||
pub async fn compose( | ||
&self, | ||
override_install_path: Option<Utf8PathBuf>, | ||
client_config: StudioClientConfig, | ||
supergraph_config: &mut SupergraphConfig, | ||
output_file: Option<Utf8PathBuf>, | ||
) -> RoverResult<RoverOutput> { | ||
let output = self | ||
.exec(override_install_path, client_config, supergraph_config) | ||
.exec( | ||
override_install_path, | ||
client_config, | ||
supergraph_config, | ||
output_file, | ||
) | ||
.await?; | ||
Ok(RoverOutput::CompositionResult(output)) | ||
} | ||
|
@@ -146,10 +160,13 @@ impl Compose { | |
override_install_path: Option<Utf8PathBuf>, | ||
client_config: StudioClientConfig, | ||
supergraph_config: &mut SupergraphConfig, | ||
output_file: Option<Utf8PathBuf>, | ||
) -> RoverResult<CompositionOutput> { | ||
let mut output_file = output_file; | ||
// first, grab the _actual_ federation version from the config we just resolved | ||
// (this will always be `Some` as long as we have created with `resolve_supergraph_yaml` so it is safe to unwrap) | ||
let federation_version = supergraph_config.get_federation_version().unwrap(); | ||
|
||
let exe = self | ||
.maybe_install_supergraph( | ||
override_install_path, | ||
|
@@ -180,52 +197,116 @@ impl Compose { | |
f.sync_all()?; | ||
tracing::debug!("config file written to {}", &yaml_path); | ||
|
||
let federation_version = Self::extract_federation_version(&exe); | ||
let federation_version = Self::extract_federation_version(&exe)?; | ||
let exact_version = federation_version | ||
.get_exact() | ||
// This should be impossible to get to because we convert to a FederationVersion a few | ||
// lines above and so _should_ have an exact version | ||
.ok_or(RoverError::new(anyhow!( | ||
"failed to get exact Federation version" | ||
)))?; | ||
|
||
eprintln!( | ||
"composing supergraph with Federation {}", | ||
&federation_version | ||
&federation_version.get_tarball_version() | ||
); | ||
|
||
let output = Command::new(&exe) | ||
.args(["compose", yaml_path.as_ref()]) | ||
.output() | ||
.context("Failed to execute command")?; | ||
let stdout = str::from_utf8(&output.stdout) | ||
.with_context(|| format!("Could not parse output of `{} compose`", &exe))?; | ||
|
||
match serde_json::from_str::<BuildResult>(stdout) { | ||
Ok(build_result) => match build_result { | ||
Ok(build_output) => Ok(CompositionOutput { | ||
hints: build_output.hints, | ||
supergraph_sdl: build_output.supergraph_sdl, | ||
federation_version: Some(federation_version.to_string()), | ||
}), | ||
Err(build_errors) => Err(RoverError::from(RoverClientError::BuildErrors { | ||
source: build_errors, | ||
num_subgraphs, | ||
})), | ||
}, | ||
Err(bad_json) => Err(anyhow!("{}", bad_json)) | ||
.with_context(|| anyhow!("{} compose output: {}", &exe, stdout)) | ||
.with_context(|| anyhow!("Output from `{} compose` was malformed.", &exe)) | ||
.map_err(|e| { | ||
let mut error = RoverError::new(e); | ||
error.set_suggestion(RoverErrorSuggestion::SubmitIssue); | ||
error | ||
}), | ||
// When the `--output` flag is used, we need a supergraph binary version that is at least | ||
// v2.9.0. We ignore that flag for composition when we have anything less than that | ||
if output_file.is_some() && exact_version.major < 2 | ||
|| (exact_version.major == 2 && exact_version.minor < 9) | ||
{ | ||
eprintln!("ignoring `--output` because it is not supported in this version of the dependent binary, `supergraph`: {}. Upgrade to Federation 2.9.0 or greater to install a version of the binary that supports it.", federation_version); | ||
output_file = None; | ||
} | ||
|
||
// Whether we use stdout or a file dependson whether the the `--output` option was used | ||
let content = match output_file { | ||
// If it was, we use a file in the supergraph binary; this cuts down the overall time | ||
// it takes to do composition when we're working on really large compositions, but it | ||
// carries with it the assumption that stdout is superfluous | ||
Some(filepath) => { | ||
Command::new(&exe) | ||
.args(["compose", yaml_path.as_ref(), filepath.as_ref()]) | ||
.output() | ||
.context("Failed to execute command")?; | ||
Comment on lines
+225
to
+232
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 should check the version of composition and return a useful error if the user tries to use the new option with a too-old version of composition. Otherwise, they'll get a confusing message, since it's really not on them to know that we're passing through the option to another binary. In fact, it might be even better to support the new option for older compositions, and we just redirect the output ourselves instead of passing through the option? 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. ahh, smart! I added a check for supergraph binary version and am ignoring the flag when it's lower than 2.9.0 since this isn't a customer-requested feature, I'm going to punt on supporting older compositions by adding more logic--we can always add it later if/when folks want it |
||
|
||
let mut composition_file = std::fs::File::open(&filepath).unwrap(); | ||
let mut content: String = String::new(); | ||
composition_file.read_to_string(&mut content).unwrap(); | ||
content | ||
} | ||
// When we aren't using `--output`, we dump the composition directly to stdout | ||
None => { | ||
let output = Command::new(&exe) | ||
.args(["compose", yaml_path.as_ref()]) | ||
.output() | ||
.context("Failed to execute command")?; | ||
|
||
let content = str::from_utf8(&output.stdout) | ||
.with_context(|| format!("Could not parse output of `{} compose`", &exe))?; | ||
content.to_string() | ||
} | ||
}; | ||
|
||
// Make sure the composition is well-formed | ||
let composition = match serde_json::from_str::<BuildResult>(&content) { | ||
Ok(res) => res, | ||
Err(err) => { | ||
return Err(anyhow!("{}", err)) | ||
.with_context(|| anyhow!("{} compose output: {}", &exe, content)) | ||
.with_context(|| anyhow!("Output from `{} compose` was malformed.", &exe)) | ||
.map_err(|e| { | ||
let mut error = RoverError::new(e); | ||
error.set_suggestion(RoverErrorSuggestion::SubmitIssue); | ||
error | ||
}) | ||
} | ||
}; | ||
|
||
match composition { | ||
Ok(build_output) => Ok(CompositionOutput { | ||
hints: build_output.hints, | ||
supergraph_sdl: build_output.supergraph_sdl, | ||
federation_version: Some(format_version(federation_version.to_string())), | ||
}), | ||
Err(build_errors) => Err(RoverError::from(RoverClientError::BuildErrors { | ||
source: build_errors, | ||
num_subgraphs, | ||
})), | ||
} | ||
} | ||
|
||
fn extract_federation_version(exe: &Utf8PathBuf) -> &str { | ||
/// Extracts the Federation Version from the executable | ||
fn extract_federation_version(exe: &Utf8PathBuf) -> Result<FederationVersion, RoverError> { | ||
let file_name = exe.file_name().unwrap(); | ||
let without_exe = file_name.strip_suffix(".exe").unwrap_or(file_name); | ||
without_exe | ||
.strip_prefix("supergraph-") | ||
.unwrap_or(without_exe) | ||
let version = match Version::parse( | ||
without_exe | ||
.strip_prefix("supergraph-v") | ||
.unwrap_or(without_exe), | ||
) { | ||
Ok(version) => version, | ||
Err(err) => return Err(RoverError::new(err)), | ||
}; | ||
|
||
match version.major { | ||
0 | 1 => Ok(FederationVersion::ExactFedOne(version)), | ||
2 => Ok(FederationVersion::ExactFedTwo(version)), | ||
_ => Err(RoverError::new(anyhow!("unsupported Federation version"))), | ||
} | ||
} | ||
} | ||
|
||
/// Format the a Version string (coming from an exact version, which includes a `=` rather than a | ||
/// `v`) for readability | ||
fn format_version(version: String) -> String { | ||
let unformatted = &version[1..]; | ||
let mut formatted = unformatted.to_string(); | ||
formatted.insert(0, 'v'); | ||
formatted | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use rstest::rstest; | ||
|
@@ -247,7 +328,7 @@ mod tests { | |
fn it_can_extract_a_version_correctly(#[case] file_path: &str, #[case] expected_value: &str) { | ||
let mut fake_path = Utf8PathBuf::new(); | ||
fake_path.push(file_path); | ||
let result = Compose::extract_federation_version(&fake_path); | ||
assert_that(&result).is_equal_to(expected_value); | ||
let result = Compose::extract_federation_version(&fake_path).unwrap(); | ||
assert_that(&result).matches(|f| format_version(f.to_string()) == expected_value); | ||
} | ||
} |
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.
Looks like there's a precedence error here that can lead to the warning being printed when it shouldn't (but no other negative effects). Fixed in #2100