feat: add AutoYaST error reporting (#1476)

## Problem

Autoyast conversion script can try to open ui for reporting errors or
ask questions.


## Solution

Monkey patch it to use agama question mechanism. This change tries to
cover the most common cases.
It also needs to implement generic Question with Password react
component.
And last but not least during manual testing few issues are revealed and
fixed. Also as part of change listing of all question was fixed to
really show only unanswered ones and optimize it to use ObjectManager
methods.

## Testing
For testing CLI for asking this command is used:

```sh
agama questions ask < data.json
```

For testing autoyast questions it uses encrypted autoyast profile that
ask for password and also report issue when password is wrong:

```sh
agama profile autoyast ftp://pong.suse.cz/autoinst_enc.xml
```

rust/agama-cli/src/questions.rs

@@ -3,6 +3,7 @@ use agama_lib::questions::http_client::HTTPClient;
 use agama_lib::{connection, error::ServiceError};
 use clap::{Args, Subcommand, ValueEnum};
 
+// TODO: use for answers also JSON to be consistent
 #[derive(Subcommand, Debug)]
 pub enum QuestionsCommands {
     /// Set the mode for answering questions.
@@ -19,9 +20,9 @@ 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
+    /// Prints the list of questions that are waiting for an answer in JSON format
     List,
-    /// Ask question from stdin in YAML format and print answer when it is answered.
+    /// Reads a question definition in JSON from stdin and prints the response when it is answered.
     Ask,
 }
 
@@ -56,12 +57,10 @@ async fn set_answers(proxy: Questions1Proxy<'_>, path: String) -> Result<(), Ser
 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
-    // when questions are already read from json
     // FIXME: if performance is bad, we can skip converting json from http to struct and then
     // serialize it, but it won't be pretty string
-    let questions_json =
-        serde_json::to_string_pretty(&questions).map_err(Into::<anyhow::Error>::into)?;
+    let questions_json = serde_json::to_string_pretty(&questions)
+        .map_err(|e| ServiceError::InternalError(e.to_string()))?;
     println!("{}", questions_json);
     Ok(())
 }
@@ -71,11 +70,17 @@ async fn ask_question() -> Result<(), ServiceError> {
     let question = serde_json::from_reader(std::io::stdin())?;
 
     let created_question = client.create_question(&question).await?;
-    let answer = client.get_answer(created_question.generic.id).await?;
-    let answer_json = serde_json::to_string_pretty(&answer).map_err(Into::<anyhow::Error>::into)?;
+    let Some(id) = created_question.generic.id else {
+        return Err(ServiceError::InternalError(
+            "Created question does not get id".to_string(),
+        ));
+    };
+    let answer = client.get_answer(id).await?;
+    let answer_json = serde_json::to_string_pretty(&answer)
+        .map_err(|e| ServiceError::InternalError(e.to_string()))?;
     println!("{}", answer_json);
 
-    client.delete_question(created_question.generic.id).await?;
+    client.delete_question(id).await?;
     Ok(())
 }
 

rust/agama-lib/src/error.rs

@@ -37,10 +37,15 @@ pub enum ServiceError {
     UnsuccessfulAction(String),
     #[error("Unknown installation phase: {0}")]
     UnknownInstallationPhase(u32),
+    #[error("Question with id {0} does not exist")]
+    QuestionNotExist(u32),
     #[error("Backend call failed with status {0} and text '{1}'")]
     BackendError(u16, String),
     #[error("You are not logged in. Please use: agama auth login")]
     NotAuthenticated,
+    // Specific error when something does not work as expected, but it is not user fault
+    #[error("Internal error. Please report a bug and attach logs. Details: {0}")]
+    InternalError(String),
 }
 
 #[derive(Error, Debug)]

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

@@ -63,7 +63,7 @@ impl HTTPClient {
     }
 
     pub async fn delete_question(&self, question_id: u32) -> Result<(), ServiceError> {
-        let path = format!("/questions/{}/answer", question_id);
+        let path = format!("/questions/{}", question_id);
         self.client.delete(path.as_str()).await
     }
 }

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

@@ -19,7 +19,8 @@ pub struct Question {
 #[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)]
 #[serde(rename_all = "camelCase")]
 pub struct GenericQuestion {
-    pub id: u32,
+    /// id is optional as newly created questions does not have it assigned
+    pub id: Option<u32>,
     pub class: String,
     pub text: String,
     pub options: Vec<String>,

rust/agama-server/src/questions/web.rs

@@ -7,6 +7,7 @@
 
 use crate::{error::Error, web::Event};
 use agama_lib::{
+    dbus::{extract_id_from_path, get_property},
     error::ServiceError,
     proxies::{GenericQuestionProxy, QuestionWithPasswordProxy, Questions1Proxy},
     questions::model::{Answer, GenericQuestion, PasswordAnswer, Question, QuestionWithPassword},
@@ -19,13 +20,12 @@ use axum::{
     routing::{delete, get},
     Json, Router,
 };
-use regex::Regex;
 use std::{collections::HashMap, pin::Pin};
 use tokio_stream::{Stream, StreamExt};
 use zbus::{
     fdo::ObjectManagerProxy,
     names::{InterfaceName, OwnedInterfaceName},
-    zvariant::{ObjectPath, OwnedObjectPath},
+    zvariant::{ObjectPath, OwnedObjectPath, OwnedValue},
 };
 
 // TODO: move to lib or maybe not and just have in lib client for http API?
@@ -34,6 +34,8 @@ struct QuestionsClient<'a> {
     connection: zbus::Connection,
     objects_proxy: ObjectManagerProxy<'a>,
     questions_proxy: Questions1Proxy<'a>,
+    generic_interface: OwnedInterfaceName,
+    with_password_interface: OwnedInterfaceName,
 }
 
 impl<'a> QuestionsClient<'a> {
@@ -48,6 +50,14 @@ impl<'a> QuestionsClient<'a> {
                 .destination("org.opensuse.Agama1")?
                 .build()
                 .await?,
+            generic_interface: InterfaceName::from_str_unchecked(
+                "org.opensuse.Agama1.Questions.Generic",
+            )
+            .into(),
+            with_password_interface: InterfaceName::from_str_unchecked(
+                "org.opensuse.Agama1.Questions.WithPassword",
+            )
+            .into(),
         })
     }
 
@@ -61,6 +71,7 @@ impl<'a> QuestionsClient<'a> {
             .map(|(k, v)| (k.as_str(), v.as_str()))
             .collect();
         let path = if question.with_password.is_some() {
+            tracing::info!("creating a question with password");
             self.questions_proxy
                 .new_with_password(
                     &generic.class,
@@ -71,6 +82,7 @@ impl<'a> QuestionsClient<'a> {
                 )
                 .await?
         } else {
+            tracing::info!("creating a generic question");
             self.questions_proxy
                 .new_question(
                     &generic.class,
@@ -82,14 +94,9 @@ impl<'a> QuestionsClient<'a> {
                 .await?
         };
         let mut res = question.clone();
-        // we are sure that regexp is correct, so use unwrap
-        let id_matcher = Regex::new(r"/(?<id>\d+)$").unwrap();
-        let Some(id_cap) = id_matcher.captures(path.as_str()) else {
-            let msg = format!("Failed to get ID for new question: {}", path.as_str()).to_string();
-            return Err(ServiceError::UnsuccessfulAction(msg));
-        }; // TODO: better error if path does not contain id
-        res.generic.id = id_cap["id"].parse::<u32>().unwrap();
-        Ok(question)
+        res.generic.id = Some(extract_id_from_path(&path)?);
+        tracing::info!("new question gets id {:?}", res.generic.id);
+        Ok(res)
     }
 
     pub async fn questions(&self) -> Result<Vec<Question>, ServiceError> {
@@ -99,54 +106,46 @@ impl<'a> QuestionsClient<'a> {
             .await
             .context("failed to get managed object with Object Manager")?;
         let mut result: Vec<Question> = Vec::with_capacity(objects.len());
-        let password_interface = OwnedInterfaceName::from(
-            InterfaceName::from_static_str("org.opensuse.Agama1.Questions.WithPassword")
-                .context("Failed to create interface name for question with password")?,
-        );
-        for (path, interfaces_hash) in objects.iter() {
-            if interfaces_hash.contains_key(&password_interface) {
-                result.push(self.build_question_with_password(path).await?)
-            } else {
-                result.push(self.build_generic_question(path).await?)
+
+        for (_path, interfaces_hash) in objects.iter() {
+            let generic_properties = interfaces_hash
+                .get(&self.generic_interface)
+                .context("Failed to create interface name for generic question")?;
+            // skip if question is already answered
+            let answer: String = get_property(generic_properties, "Answer")?;
+            if !answer.is_empty() {
+                continue;
+            }
+            let mut question = self.build_generic_question(generic_properties)?;
+
+            if interfaces_hash.contains_key(&self.with_password_interface) {
+                question.with_password = Some(QuestionWithPassword {});
             }
+
+            result.push(question);
         }
         Ok(result)
     }
 
-    async fn build_generic_question(
+    fn build_generic_question(
         &self,
-        path: &OwnedObjectPath,
+        properties: &HashMap<String, OwnedValue>,
     ) -> Result<Question, ServiceError> {
-        let dbus_question = GenericQuestionProxy::builder(&self.connection)
-            .path(path)?
-            .cache_properties(zbus::CacheProperties::No)
-            .build()
-            .await?;
         let result = Question {
             generic: GenericQuestion {
-                id: dbus_question.id().await?,
-                class: dbus_question.class().await?,
-                text: dbus_question.text().await?,
-                options: dbus_question.options().await?,
-                default_option: dbus_question.default_option().await?,
-                data: dbus_question.data().await?,
+                id: Some(get_property(properties, "Id")?),
+                class: get_property(properties, "Class")?,
+                text: get_property(properties, "Text")?,
+                options: get_property(properties, "Options")?,
+                default_option: get_property(properties, "DefaultOption")?,
+                data: get_property(properties, "Data")?,
             },
             with_password: None,
         };
 
         Ok(result)
     }
 
-    async fn build_question_with_password(
-        &self,
-        path: &OwnedObjectPath,
-    ) -> Result<Question, ServiceError> {
-        let mut result = self.build_generic_question(path).await?;
-        result.with_password = Some(QuestionWithPassword {});
-
-        Ok(result)
-    }
-
     pub async fn delete(&self, id: u32) -> Result<(), ServiceError> {
         let question_path = ObjectPath::from(
             ObjectPath::try_from(format!("/org/opensuse/Agama1/Questions/{}", id))
@@ -164,24 +163,29 @@ impl<'a> QuestionsClient<'a> {
             ObjectPath::try_from(format!("/org/opensuse/Agama1/Questions/{}", id))
                 .context("Failed to create dbus path")?,
         );
+        let objects = self.objects_proxy.get_managed_objects().await?;
+        let password_interface = OwnedInterfaceName::from(
+            InterfaceName::from_static_str("org.opensuse.Agama1.Questions.WithPassword")
+                .context("Failed to create interface name for question with password")?,
+        );
         let mut result = Answer::default();
-        let dbus_password_res = QuestionWithPasswordProxy::builder(&self.connection)
-            .path(&question_path)?
-            .cache_properties(zbus::CacheProperties::No)
-            .build()
-            .await;
-        if let Ok(dbus_password) = dbus_password_res {
+        let question = objects
+            .get(&question_path)
+            .ok_or(ServiceError::QuestionNotExist(id))?;
+
+        if let Some(password_iface) = question.get(&password_interface) {
             result.with_password = Some(PasswordAnswer {
-                password: dbus_password.password().await?,
+                password: get_property(password_iface, "Password")?,
             });
         }
-
-        let dbus_generic = GenericQuestionProxy::builder(&self.connection)
-            .path(&question_path)?
-            .cache_properties(zbus::CacheProperties::No)
-            .build()
-            .await?;
-        let answer = dbus_generic.answer().await?;
+        let generic_interface = OwnedInterfaceName::from(
+            InterfaceName::from_static_str("org.opensuse.Agama1.Questions.Generic")
+                .context("Failed to create interface name for generic question")?,
+        );
+        let generic_iface = question
+            .get(&generic_interface)
+            .context("Question does not have generic interface")?;
+        let answer: String = get_property(generic_iface, "Answer")?;
         if answer.is_empty() {
             Ok(None)
         } else {

rust/package/agama.changes

@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Mon Jul 22 15:27:44 UTC 2024 - Josef Reidinger <jreidinger@suse.com>
+
+- Fix `agama questions list` to list only unaswered questions and
+  improve its performance
+  (gh#openSUSE/agama#1476)
+
 -------------------------------------------------------------------
 Wed Jul 17 11:15:33 UTC 2024 - Jorik Cronenberg <jorik.cronenberg@suse.com>
 

service/README.md

@@ -0,0 +1,21 @@
+# Agama YaST
+
+According to [Agama's architecture](../doc/architecture.md) this project implements the following components:
+
+* The *Agama YaST*, the layer build on top of YaST functionality.
+
+## Testing Changes
+
+The easiest way to test changes done to Ruby code on Agama live media is to build
+the gem with modified sources with `gem build agama-yast`. Then copy the resulting file
+to Agama live image. Then run this sequence of commands:
+
+```sh
+# ensure that only modified sources are installed
+gem uninstall agama-yast
+# install modified sources including proper binary names
+gem install --no-doc --no-format-executable <path to gem>
+```
+
+If the changes modify the D-Bus part, then restart related D-Bus services.
+

service/lib/agama/autoyast/converter.rb

@@ -33,6 +33,8 @@
 require "fileutils"
 require "pathname"
 
+require "agama/autoyast/report_patching"
+
 # :nodoc:
 module Agama
   module AutoYaST