feat: add AutoYaST error reporting

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