Ask cli #1457

jreidinger · 2024-07-12T08:39:25Z

Problem

There is no CLI command to ask new question. It is needed to ask questions from autoyast convert script.
Also there is a need to share functionality between various CLI calls that will use HTTP API as current one in network has limitation and is not designed as generic http agent that fits agama.

Solution

Implement needed functionality in questions http client.
Implement BaseHttpClient that shares code for doing http requests on agama http server.
Implement ask and list subcommand for questions to allow asking questions and also listing currently unanswered ones.

imobachgs

Apart from some minor naming issues (you know, naming is hard) and some missing docs, I think you are on the right track.

imobachgs · 2024-07-12T08:42:05Z

rust/agama-cli/src/main.rs

-async fn run_command(cli: Cli) -> anyhow::Result<()> {
-    match cli.command {
+async fn run_command(cli: Cli) -> Result<(), ServiceError> {
+    Ok(match cli.command {


I am unsure whether I would like to put the full match inside the Ok().

I can use let result = match and then Ok(result)

rust/agama-lib/src/questions/http_client.rs

imobachgs · 2024-07-12T08:57:09Z

rust/agama-lib/src/questions/http_client.rs

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


Finding a good name and importing it is a better (and more readable) approach IMHO.

imobachgs · 2024-07-12T08:58:07Z

rust/agama-lib/src/http_client.rs

+
+const API_URL: &str = "http://localhost/api";
+
+impl HTTPClient {


Please, document this client with some examples.

imobachgs · 2024-07-12T08:59:16Z

rust/agama-lib/src/http_client.rs

+
+        let mut headers = header::HeaderMap::new();
+        let value = header::HeaderValue::from_str(format!("Bearer {}", token).as_str())
+            .map_err(|e| ServiceError::NetworkClientError(e.to_string()))?;


NetworkClientError sounds like specific to the network scope only.

ah, my fault. It should be generic one. Thanks for catch ( c&p error )

imobachgs · 2024-07-12T09:00:14Z

rust/agama-lib/src/http_client.rs

+    }
+
+    const NO_TEXT: &'static str = "No text";
+    // Simple wrapper around Response to get target type.


What "target type" means?

it is basically to get object you expect from backend. Something like let questions : Vec<Question> = client.get_type("/questions")

imobachgs · 2024-07-12T09:00:59Z

rust/agama-lib/src/http_client.rs

+    // For more advanced usage use directly get method
+    pub async fn get_type<T: DeserializeOwned>(&self, path: &str) -> Result<T, ServiceError> {
+        let response = self.get(path).await?;
+        if !response.status().is_success() {


np: What about returning early if is_success()? I am saying just because the error handling seems to be longer.

BTW one more idea I had...I know that we use 400 for error and in body we use json for ServiceError...so do you think it make sense to have something like ErrorFromBackend that gets as argument ServiceError from response?

imobachgs · 2024-07-12T09:14:26Z

rust/agama-lib/src/http_client.rs

+            .map_err(|e| e.into())
+    }
+
+    fn target_path(&self, path: &str) -> String {


What about using request_url? (or just url)?

fine for me...url is probably better as it is internal only method..so shorter is better.

imobachgs · 2024-07-12T09:16:59Z

rust/agama-lib/src/http_client.rs

+    const NO_TEXT: &'static str = "No text";
+    // Simple wrapper around Response to get target type.
+    // For more advanced usage use directly get method
+    pub async fn get_type<T: DeserializeOwned>(&self, path: &str) -> Result<T, ServiceError> {


I like the idea but get_type is kind of confusing (am I getting the "type"?).

In any case, I would call this method get and rename the get method below to something like get_raw as I expect us to use this more often.

agreed. I am even thinking about naming this one as get_object but it is right, that plain get here and maybe get_response would be better

imobachgs · 2024-07-12T09:19:39Z

rust/agama-lib/src/questions/model.rs

+/// is that it contain both question and answer. It works for dbus
+/// API which has both as attributes, but web API separate
+/// question and its answer. So here it is split into GenericQuestion
+/// and GenericAnswer


Perhaps we should consider refactoring the GenericQuestion from lib to be a structure composed of question and answer structs from this module. This is just an idea.

I think whole questions DBus service needs a bit refactoring as this code does not make me proud to be author :)

imobachgs

In general it is OK. Just some comments about the documentation.

imobachgs · 2024-07-15T09:21:58Z

rust/agama-lib/src/base_http_client.rs

+        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))?;


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

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

imobachgs · 2024-07-15T09:23:18Z

rust/agama-cli/src/questions.rs

@@ -19,6 +19,8 @@ pub enum QuestionsCommands {
        /// Path to a file containing the answers in YAML format.
        path: String,
    },
+    /// prints list of questions that is waiting for answer in YAML format


I wonder whether we should use JSON for everything.

it is fine for me if agreed on it

reason why I use yaml for it is that in answers file we used for automatic answers we also use yaml.

Yes, I know, we use JSON for profiles and YAML for the answers file. I am not saying that we should fix it now, but let's have into account in the future.

rust/agama-lib/src/base_http_client.rs

imobachgs · 2024-07-15T09:26:05Z

rust/agama-lib/src/base_http_client.rs

+        })
+    }
+
+    const NO_TEXT: &'static str = "No text";


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

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

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

rust/agama-lib/src/base_http_client.rs

imobachgs · 2024-07-15T13:12:31Z

rust/agama-lib/src/base_http_client.rs

+            .map_err(|e| e.into())
+    }
+
+    pub async fn build_backend_error(&self, response: Response) -> ServiceError {


Perhaps you might want to implement something like From<Response> (or TryFrom).

Can it be asynchronous? As it is not simple conversion but it also ask for text.

Also it is a bit against documentation of traits which mention that it should be obvious to convert...which I am not sure about this case.

OK, async could be a problem. Let's keep it as it is.

About when to implement those traits, I am usually unsure 😅

rust/agama-lib/src/error.rs

rust/agama-lib/src/questions.rs

Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>

mvidner

Here reviewing just the BaseHTTPClient part

rust/agama-lib/src/base_http_client.rs

mvidner · 2024-07-16T12:02:34Z

rust/agama-lib/src/base_http_client.rs

+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")?;


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

mvidner · 2024-07-16T12:15:58Z

rust/agama-lib/src/base_http_client.rs

+        let text = response
+            .text()
+            .await
+            .unwrap_or_else(|_| Self::NO_TEXT.to_string());


"No text" -> "(failed to extract error text from HTTP response)" is perhaps too verbose but I would appreciate it when debugging it as an operator

I check quite deeply when call to text can fail and to be honest I do not find it either in documentation neither from code. So lets use your suggestion.

mvidner · 2024-07-16T12:16:36Z

rust/agama-lib/src/base_http_client.rs

+        })
+    }
+
+    const NO_TEXT: &'static str = "No text";


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

mvidner · 2024-07-16T12:23:24Z

rust/agama-lib/src/base_http_client.rs

+    }
+
+    /// post object to given path and report error if response is not success
+    pub async fn post(&self, path: &str, object: &impl Serialize) -> Result<(), ServiceError> {


Stupid question, is it normal that a successful HTTP POST returns no useful text? At least in Agama?

Well, response is optional in POST. I check what we have in agama-web-server openapi and it is like half to half if we return something or not...if we need it more often we can create variant that also return object...I am just not sure what will be good name :)

Yes, a POST call does not need to return anything. I am not convinced about using "no text" string. Why not just return an empty body? In any case, we can evolve this API when we use it.

rust/agama-lib/src/error.rs

rust/package/agama.changes

Co-authored-by: Martin Vidner <mvidner@suse.cz>

imobachgs

Just minor comments. Feel free to merge.

imobachgs · 2024-07-16T13:41:50Z

rust/agama-cli/src/questions.rs

@@ -19,6 +19,8 @@ pub enum QuestionsCommands {
        /// Path to a file containing the answers in YAML format.
        path: String,
    },
+    /// prints list of questions that is waiting for answer in YAML format


Yes, I know, we use JSON for profiles and YAML for the answers file. I am not saying that we should fix it now, but let's have into account in the future.

imobachgs · 2024-07-16T13:42:49Z

rust/agama-cli/src/questions.rs

+async fn list_questions() -> Result<(), ServiceError> {
+    let client = HTTPClient::new().await?;
+    let questions = client.list_questions().await?;
+    // FIXME: that conversion to anyhow error is nasty, but we do not expect issue


We could have a generic error for these cases. But let's keep it as it is now. At some point we should improve our error types.

rust/agama-lib/src/base_http_client.rs

imobachgs · 2024-07-16T13:46:14Z

rust/agama-lib/src/base_http_client.rs

+            .map_err(|e| e.into())
+    }
+
+    pub async fn build_backend_error(&self, response: Response) -> ServiceError {


OK, async could be a problem. Let's keep it as it is.

Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>

Prepare for releasing Agama 10· * #1263 * #1330 * #1407 * #1408 * #1410 * #1411 * #1412 * #1416 * #1417 * #1419 * #1420 * #1421 * #1422 * #1423 * #1424 * #1425 * #1428 * #1429 * #1430 * #1431 * #1432 * #1433 * #1436 * #1437 * #1438 * #1439 * #1440 * #1441 * #1443 * #1444 * #1445 * #1449 * #1450 * #1451 * #1452 * #1453 * #1454 * #1455 * #1456 * #1457 * #1459 * #1460 * #1462 * #1464 * #1465 * #1466 * #1467 * #1468 * #1469 * #1470 * #1471 * #1472 * #1473 * #1475 * #1476 * #1477 * #1478 * #1479 * #1480 * #1481 * #1482 * #1483 * #1484 * #1485 * #1486 * #1487 * #1488 * #1489 * #1491 * #1492 * #1493 * #1494 * #1496 * #1497 * #1498 * #1499 * #1500 * #1501 * #1502 * #1503 * #1504 * #1505 * #1506 * #1507 * #1508 * #1510 * #1511 * #1512 * #1513 * #1514 * #1515 * #1516 * #1517 * #1518 * #1519 * #1520 * #1522 * #1523 * #1524 * #1525 * #1526 * #1527 * #1528 * #1529 * #1530 * #1531 * #1532 * #1533 * #1534 * #1535 * #1536 * #1537 * #1540 * #1541 * #1543 * #1544 * #1545 * #1546 * #1547 * #1548 * #1549 * #1550 * #1552 * #1553 * #1554 * #1555 * #1556 * #1557 * #1558 * #1559 * #1560 * #1562 * #1563 * #1565 * #1566 * #1567 * #1568 * #1569 * #1570 * #1571 * #1572 * #1573 * #1574 * #1575 * #1576 * #1577 * #1578 * #1579 * #1580 * #1581 * #1583 * #1584 * #1585 * #1586 * #1587 * #1588 * #1589 * #1590 * #1591 * #1592 * #1593 * #1596 * #1597 * #1598 * #1600 * #1602 * #1605 * #1606 * #1607 * #1608 * #1610 * #1611 * #1612 * #1613 * #1614 * #1619 * #1620 * #1621

jreidinger added 2 commits July 11, 2024 17:23

questions and ask CLI WIP

d16b211

first working questions list command

fb6b97e

imobachgs reviewed Jul 12, 2024

View reviewed changes

jreidinger added 5 commits July 12, 2024 21:07

changes from review

177f5cc

add post method to base client and use it in creating question call

5ba3b2f

format rust

59faba9

implement getting answer

84120fb

implement deleting question

a552420

imobachgs reviewed Jul 15, 2024

View reviewed changes

jreidinger and others added 6 commits July 16, 2024 13:01

implement ask question

97fd52f

Apply suggestions from code review

491acec

Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>

changes from review

6e05a80

add cleaning of already answered questions:

1d67e2c

Merge remote-tracking branch 'origin/master' into ask_cli

4abaab0

changes

104adf2

jreidinger marked this pull request as ready for review July 16, 2024 11:59

mvidner reviewed Jul 16, 2024

View reviewed changes

rust/package/agama.changes Outdated Show resolved Hide resolved

jreidinger and others added 2 commits July 16, 2024 14:51

Apply suggestions from code review

4074fa6

Co-authored-by: Martin Vidner <mvidner@suse.cz>

changes from review

f35f94d

imobachgs approved these changes Jul 16, 2024

View reviewed changes

jreidinger and others added 2 commits July 16, 2024 15:53

Update rust/agama-lib/src/base_http_client.rs

f696f41

Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>

format rust

7eabf7e

jreidinger merged commit fce3d4c into master Jul 16, 2024
2 checks passed

jreidinger deleted the ask_cli branch July 16, 2024 19:06

imobachgs mentioned this pull request Sep 20, 2024

Update to release 10 #1622

Merged


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

		impl HTTPClient {

Ask cli #1457

Ask cli #1457

Conversation

jreidinger commented Jul 12, 2024 • edited Loading

Problem

Solution

imobachgs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imobachgs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvidner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imobachgs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreidinger commented Jul 12, 2024 •

edited

Loading