github-actions[bot] commented on code in PR #4482:
URL: https://github.com/apache/iggy/pull/4482#discussion_r4237900693
##########
core/cli/src/main.rs:
##########
@@ -384,13 +382,17 @@ async fn main() -> Result<(), IggyCmdError> {
let iggy_args = merged_args.iggy;
let cli_options = merged_args.cli;
+ let server_address = iggy_args
+ .get_server_address()
+ .ok_or_else(||
ClientError::InvalidTransport(iggy_args.transport.clone()))?;
Review Comment:
nit: An invalid `--transport` prints `IggyClient(InvalidTransport("foo"))`,
which names no valid transport. Use the message in `validate_transport` for
this check and update
`core/integration/tests/cli/general/test_unknown_transport.rs` to match.
##########
core/common/src/types/args/mod.rs:
##########
@@ -353,19 +355,13 @@ pub struct Args {
pub websocket_tls_validate_certificate: bool,
}
-const QUIC_TRANSPORT: &str = "quic";
-const HTTP_TRANSPORT: &str = "http";
-const TCP_TRANSPORT: &str = "tcp";
-const WEBSOCKET_TRANSPORT: &str = "websocket";
-
impl Args {
pub fn get_server_address(&self) -> Option<String> {
- match self.transport.as_str() {
- QUIC_TRANSPORT => Some(self.quic_server_address.clone()),
- HTTP_TRANSPORT =>
Some(self.http_api_url.clone().replace("http://";, "")),
- TCP_TRANSPORT => Some(self.tcp_server_address.clone()),
- WEBSOCKET_TRANSPORT => Some(self.websocket_server_address.clone()),
- _ => None,
+ match TransportProtocol::from_str(&self.transport).ok()? {
Review Comment:
nit: `get_server_address` resolves only `tcp`, `quic`, `http` and `ws`, and
`None` means the value did not parse. Document that contract on the function.
##########
core/cli/src/credentials.rs:
##########
@@ -71,11 +71,10 @@ impl<'a> IggyCredentials<'a> {
}
let session_credentials = || -> Option<Credentials> {
- let server_address = iggy_args.get_server_address()?;
- let token =
ServerSession::new(server_address.clone()).get_token()?;
+ let token =
ServerSession::new(server_address.to_string()).get_token()?;
Review Comment:
nit: `IggyCredentials::new` calls the `session_credentials` closure twice
when no stored session exists, so the CLI reads the OS keyring twice. Add
`prefer_explicit_credentials` to the `else if` at line 149.
##########
core/integration/tests/cli/system/test_me_command.rs:
##########
@@ -43,7 +43,7 @@ impl TransportProtocolExt for TransportProtocol {
match self {
TransportProtocol::Tcp => vec!["--transport", "tcp"],
TransportProtocol::Quic => vec!["--transport", "quic"],
- TransportProtocol::WebSocket => vec!["--transport", "websocket"],
+ TransportProtocol::WebSocket => vec!["--transport", "ws"],
Review Comment:
simplification: `TransportProtocolExt` has one implementation and now only
repeats `TransportProtocol::as_str()`, while `TestMeCmd::new` already rejects
`Http`. Delete the trait and use `as_str()` in `get_command`.
##########
core/integration/tests/cli/system/test_me_command.rs:
##########
@@ -86,13 +86,14 @@ impl IggyCmdTestCase for TestMeCmd {
fn verify_command(&self, command_state: Assert) {
match &self.scenario {
Scenario::SuccessWithCredentials |
Scenario::SuccessWithoutCredentials => {
+ let transport = match self.protocol {
+ TransportProtocol::WebSocket => "WebSocket".to_string(),
Review Comment:
nit: The test hardcodes `WebSocket`, because `wire_conversions.rs` names
transport 4 `WebSocket` and `TransportProtocol` names it `ws`. Keep one display
name for the protocol and reuse it in both places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
