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

Ask cli #1457

Merged
merged 17 commits into from
Jul 16, 2024
Merged

Ask cli #1457

Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 4 additions & 2 deletions rust/agama-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ async fn build_manager<'a>() -> anyhow::Result<ManagerClient<'a>> {
}

async fn run_command(cli: Cli) -> Result<(), ServiceError> {
Ok(match cli.command {
match cli.command {
Commands::Config(subcommand) => {
let manager = build_manager().await?;
wait_for_services(&manager).await?;
Expand All @@ -139,7 +139,9 @@ async fn run_command(cli: Cli) -> Result<(), ServiceError> {
Commands::Logs(subcommand) => run_logs_cmd(subcommand).await?,
Commands::Auth(subcommand) => run_auth_cmd(subcommand).await?,
Commands::Download { url } => crate::profile::download(&url, std::io::stdout())?,
})
};

Ok(())
}

/// Represents the result of execution.
Expand Down
81 changes: 81 additions & 0 deletions rust/agama-lib/src/base_http_client.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
use anyhow::Context;
use reqwest::{header, Client, Response};
use serde::de::DeserializeOwned;

use crate::{auth::AuthToken, error::ServiceError};

/// Base that all http clients should use.
jreidinger marked this conversation as resolved.
Show resolved Hide resolved
///
/// It provides several features including automatic base url switching,
jreidinger marked this conversation as resolved.
Show resolved Hide resolved
/// websocket events listening or object constructions.
///
/// Usage should be just thin layer in domain specific client.
///
/// ```no_run
/// use agama_lib::questions::model::Question;
/// use agama_lib::base_http_client::BaseHTTPClient;
/// use agama_lib::error::ServiceError;
///
/// async fn get_questions() -> Result<Vec<Question>, ServiceError> {
/// let client = BaseHTTPClient::new()?;
/// client.get("/questions").await
/// }
/// ```
pub struct BaseHTTPClient {
client: Client,
pub base_url: String,
}

const API_URL: &str = "http://localhost/api";

impl BaseHTTPClient {
// if there is need for client without authorization, create new constructor for it
pub fn new() -> Result<Self, ServiceError> {
let token = AuthToken::find().context("You are not logged in")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message is correct (I hope) but even better would be a hint how to fix the situation: "... Try: agama auth login"?


let mut headers = header::HeaderMap::new();
// just use generic anyhow error here as Bearer format is constructed by us, so failures can come only from token
let value = header::HeaderValue::from_str(format!("Bearer {}", token).as_str())
.map_err(|e| anyhow::Error::new(e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still no convinced of using anyhow, especially in a library. But we can postpone that until we refactor the ServiceError stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

me neither...I basically use it for trash error which should not happen instead of unwrap


headers.insert(header::AUTHORIZATION, value);

let client = Client::builder().default_headers(headers).build()?;

Ok(Self {
client,
base_url: API_URL.to_string(), // TODO: add support for remote server
})
}

const NO_TEXT: &'static str = "No text";
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about the "No text" value. Why not an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To see that error from backend does not provide any text. For me less confusing to write explicitelly that no text received from backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the const definition closer to the method that uses it. That also makes it more apparent what it should say

/// Simple wrapper around Response to get object from response.
jreidinger marked this conversation as resolved.
Show resolved Hide resolved
/// If complete Response is needed use get_response method.
jreidinger marked this conversation as resolved.
Show resolved Hide resolved
pub async fn get<T: DeserializeOwned>(&self, path: &str) -> Result<T, ServiceError> {
let response = self.get_response(path).await?;
if response.status().is_success() {
response.json::<T>().await.map_err(|e| e.into())
} else {
let code = response.status().as_u16();
let text = response
.text()
.await
.unwrap_or_else(|_| Self::NO_TEXT.to_string());
Err(ServiceError::BackendError(code, text))
}
}

/// Calls GET method on given path and return Response that can be further
jreidinger marked this conversation as resolved.
Show resolved Hide resolved
/// processed. If only simple object from json is required, use method get.
jreidinger marked this conversation as resolved.
Show resolved Hide resolved
pub async fn get_response(&self, path: &str) -> Result<Response, ServiceError> {
self.client
.get(self.url(path))
.send()
.await
.map_err(|e| e.into())
}

fn url(&self, path: &str) -> String {
self.base_url.clone() + path
}
}
61 changes: 0 additions & 61 deletions rust/agama-lib/src/http_client.rs

This file was deleted.

2 changes: 1 addition & 1 deletion rust/agama-lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

pub mod auth;
pub mod error;
mod http_client;
pub mod base_http_client;
pub mod install_settings;
pub mod localization;
pub mod manager;
Expand Down
9 changes: 4 additions & 5 deletions rust/agama-lib/src/questions/http_client.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,19 @@
use crate::error::ServiceError;
use crate::{base_http_client::BaseHTTPClient, error::ServiceError};

use super::model;

pub struct HTTPClient {
client: crate::http_client::HTTPClient,
jreidinger marked this conversation as resolved.
Show resolved Hide resolved
client: BaseHTTPClient,
}

impl HTTPClient {
pub async fn new() -> Result<Self, ServiceError> {
Ok(Self {
client: crate::http_client::HTTPClient::new().await?,
client: BaseHTTPClient::new()?,
})
}

pub async fn list_questions(&self) -> Result<Vec<model::Question>, ServiceError> {
let questions = self.client.get_type("/questions").await?;
Ok(questions)
self.client.get("/questions").await
}
}
Loading