Rover Dev Rewrite - Install Router Binary #2257
Conversation
✅ Docs Preview ReadyNo new or changed pages found. |
dotdat
force-pushed
the
dotdat/install-router-plugin
branch
from
351d824
to
6f8ea7f
Compare
dotdat
force-pushed
the
dotdat/install-router-plugin
branch
from
8ab5e55
to
9e3811f
Compare
dotdat
changed the title
| test-next = "test --all-features" | ||
| build-next = "build --features composition-js,dev-next" | ||
|
|
||
| test-next = "test --all-features -- --nocapture" |
There was a problem hiding this comment.
double-checking that you're wanting that --nocapture
There was a problem hiding this comment.
Yep. This was used as a binding in my editor and is to make sure we got traces logged right.
| impl RouterBinary { | ||
| pub fn new(exe: Utf8PathBuf, version: Version) -> RouterBinary { | ||
| RouterBinary { exe, version } | ||
| } | ||
| } |
There was a problem hiding this comment.
would we want to use Builder here instead? it seems like an emerging convention
There was a problem hiding this comment.
My litmus test has been that more than two arguments equates to wanting a builder. Otherwise, a constructor is usually good enough.
| #[allow(unused)] | ||
| pub fn new( | ||
| router_version: RouterVersion, | ||
| studio_client_config: StudioClientConfig, | ||
| ) -> InstallRouter { | ||
| InstallRouter { | ||
| router_version, | ||
| studio_client_config, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
same question about using Builder
| async fn install( | ||
| &self, | ||
| override_install_path: Option<Utf8PathBuf>, | ||
| elv2_license_accepter: LicenseAccepter, | ||
| skip_update: bool, | ||
| ) -> Result<Self::Binary, Self::Error> { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
That's my read as well.
| // 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()?; |
There was a problem hiding this comment.
do you know for sure that using fed_version.clone() above is going to get the right version from the supergraph binary's perspective?
There was a problem hiding this comment.
Yeah. The process is this:
- Get a
FederationVersion(which may be some kind of "latest") - Download the binary using the specified argument
- Extract the exact version from the supergraph binary name
- We now have an exact version, which is slightly different from a
FederationVersion, since it's a exact semantic version.
| SupergraphVersion(#[from] SupergraphVersionError), | ||
| } | ||
|
|
||
| /// The installer for the supergraph binary. It implements InstallSupergraphBinary and has an |
There was a problem hiding this comment.
does InstallSupergraphBinary exist anymore?
No description provided.