-
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: Build Rover without supergraph compose
for Alpine
#538
Conversation
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.
This was mostly not Rust, so I reviewed that more carefully. I did review the Rust, and didn't notice anything unexpected.
maybe what we do is we check |
I think that's a good approximation. Expanding on that I trust this implementation which uses that approach but expands on it a bit! |
1e0c56f
to
cd3433b
Compare
oo look what i found |
e27ee67
to
8bba2d9
Compare
8bba2d9
to
86063a8
Compare
@@ -57,7 +65,7 @@ strsim = "0.10" | |||
structopt = "0.3.21" | |||
toml = "0.5" | |||
tracing = "0.1.26" | |||
url = "2.2.2" |
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.
i think compiling with --no-default-features
made this break, so had to add it explicitly so our no-composition-js would compile
/// The relative path to the supergraph configuration file. | ||
#[structopt(long = "config")] | ||
#[serde(skip_serializing)] | ||
config_path: Option<Utf8PathBuf>, |
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.
made this an option so folks wouldn't see an error message requiring this flag that, if provided, wouldn't do anything anyway
let mut err = RoverError::new(anyhow!( | ||
"This version of Rover does not support this command." | ||
)); | ||
err.set_suggestion(Suggestion::CheckGnuVersion); |
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.
we should be doing this in our command code waaayyy more often. i forgot i made the api like this.
src/error/metadata/suggestion.rs
Outdated
@@ -127,8 +128,7 @@ impl Display for Suggestion { | |||
Suggestion::Adhoc(msg) => msg.to_string(), | |||
Suggestion::CheckServerConnection => "Make sure the endpoint is accepting connections and is spelled correctly".to_string(), | |||
Suggestion::ConvertGraphToSubgraph => "If you are sure you want to convert a non-federated graph to a subgraph, you can re-run the same command with a `--convert` flag.".to_string(), | |||
|
|||
|
|||
Suggestion::CheckGnuVersion => "If the command `ldd --version` prints a `glibc` version >= 2.18, you can try installing the Rover binary built for `x86_64-unknown-linux-gnu`.".to_string(), |
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.
don't love this messaging... lemme know if y'all have ideas
Alright! I think this is good to go, I've tested everything out on my fork and installs work great on both Alpine and Ubuntu. I made a few small small tweaks that I have kicked off a build for. By the time anybody sees this, hopefully the following command is good to go if you want to test everything yourself:
|
Co-authored-by: Irina Shestak <[email protected]>
01452ce
to
ca96069
Compare
This PR satisfies the first steps checklist outlined in #537.
composition-js
cargo feature to Rover (which is enabled by default)release.yml
we compile forx86_64-unknown-linux-musl
with the--no-default-features
flagtest.yml
we run an extracargo check --workspace --locked --no-default-features
to make sure it compiles OKIf a user installs the
musl
binary and attempts to runrover supergraph compose
, they will get the following error:I... couldn't find a nice way to check the glibc version right in Rust. I think that error message should be good enough in that case? It's not perfect.
A few other things:
Installing on Alpine will give you the following warning message:
I've added the
-l
flag to theexec $SHELL
command that we recommend since Alpine will only re-source.profile
on login shells. This flag also works onzsh
andbash
.