adapt dbus API to prepare for answers file and change use_defaults to… by jreidinger · Pull Request #669 · agama-project/agama

jreidinger · 2023-07-19T09:26:50Z

… interactive property

Problem

We want to have support for predefined answers and it is missing.

https://trello.com/c/9plYc08k/3373-8-agama-add-support-for-unattended-questions

Solution

Adapt dbus API to allow to add such file.

Testing

Tested manually
Unit tested

Follow-ups

document all types of questions we have
some tracking/reporting feature so user can see questions that was asked/answered.

… interactive property

coveralls · 2023-07-19T09:37:32Z

Changes unknown
when pulling a13759c on answers_support
into ** on master**.

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

mvidner · 2023-07-19T11:15:40Z

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

+        log::info!("set interactive to {}", value);
+        if value {
+            if !self.interactive() {
+                self.answer_strategies.pop();


This means that no strategies mean "no non-interactive" strategies, that is, the default strategy is interaction? Should be explicitly stated at Questions::answer_strategies declaration

yes, no strategies means that user need to answer it itself. Will document it.

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

mvidner · 2023-07-25T14:59:36Z

rust/agama-dbus-server/src/questions/answers.rs

+    }
+
+    #[test]
+    fn test_full_data_match() {


I think this test does the same thing as partial_data_match. What is the point?

well, difference here is that first test match only one answer. But here data2 match one answer and data1 another. So it test if wins correct answer.

I see, maybe I can express it slightly better in code or comments, will try

joseivanlopez · 2023-07-26T08:54:38Z

doc/dbus/org.opensuse.Agama.Questions1.doc.xml

    <!--
-      UseDefaultAnswer:
+      AddAnswerFile:
+      @path: Local fs path to answers file in yaml format.


NP: In Agama we are using both YaML and Json (jsonnet). It was already commented in some planning that we should converge to a single format if there is no reason to keep both. I would vote for using json everywhere: config file, autoinstall profile and questions answers.

luckily it is easy with abstraction library provides. It is just about different crate. So when it is unify it is fine for me to convert it.

joseivanlopez · 2023-07-26T08:58:03Z

rust/agama-cli/src/questions.rs

+    proxy
+        .set_interactive(value == Modes::Interactive)
+        .await
+        .context("Failed to set default answer")


I find the error a bit misleading. What about "Failed to set the mode for answering questions".

joseivanlopez · 2023-07-26T09:01:16Z

rust/agama-cli/src/questions.rs

+    proxy
+        .add_answer_file(path.as_str())
+        .await
+        .context("Failed to set default answer")


NP: The same here, maybe "Failed to set default answers from answers file".

joseivanlopez · 2023-07-26T09:10:15Z

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


+impl DefaultAnswers {
+    pub fn id() -> u8 {
+        1


NP: I think this can be a field of the struct.

reason why it is also class method is to allow detection of strategy like below that instance is DefaultAnswers

joseivanlopez · 2023-07-26T09:25:09Z

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

+    fn interactive(&self) -> bool {
+        let last = self.answer_strategies.last();
+        if let Some(real_strategy) = last {
+            real_strategy.id() != DefaultAnswers::id()


I am not sure if I understood the idea of having more than one strategy. Is it for priorities? First from file, if no found then default, if not found then ask to user? And the mode is only considered interactive if the last assigned strategy is not DefaultAnswers. But what happens if the last strategy is answers?

Reason for strategies is extensibility, which allows to add completely different source of answers from strategy like e.g. some remote server answering it.
So whole idea is to have list of strategies and the first one that can answer it, provide that answer.
Default strategy should be always able to answer, so it is reason why it is last as answer file has precedence. And also it means that it is non interactive.
If last strategy is answers and it does not provide answer to given question, it is asked user. It is general rule, if no strategy can answer, then ask user ( aka interactive mode ).

joseivanlopez · 2023-07-26T09:26:54Z

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


 /// Trait for objects that can provide answers to all kind of Question.
+///
+/// If not strategy is used or all defined strategies does not know answer,


NP: a bit of rewording: "If no strategy is selected or the answer is unknown, then ask to the user".

joseivanlopez · 2023-07-26T09:53:11Z

rust/agama-dbus-server/src/questions/answers.rs

+    pub data: Option<HashMap<String, String>>,
+    /// The answer text is the only mandatory part of an Answer
+    pub answer: String,
+    /// All possible mixins have to be here, so they can be specified in an Answer


Mixins to Base Question like WithPassword. So it means that all possible additional data entries from additional mixins to question have to be specified in answer.

adapt dbus API to prepare for answers file and change use_defaults to…

defdcdf

… interactive property

mvidner reviewed Jul 19, 2023

View reviewed changes

rust/agama-dbus-server/src/questions.rs Outdated Show resolved Hide resolved

mvidner reviewed Jul 19, 2023

View reviewed changes

fixes from review

3f8874d

mvidner reviewed Jul 19, 2023

View reviewed changes

rust/agama-dbus-server/src/questions.rs Show resolved Hide resolved

jreidinger added 8 commits July 19, 2023 13:58

fix formatting

d4cdb6f

simplify code

a13759c

implement answers support

c58c3e3

apply clippy suggestion

771143b

add tests for answers

8844c62

update dbus introspection and documentation for Questions

151bede

remove double Delete method from doc

98c4d42

use new dbus interface in CLI

0fb9164

jreidinger marked this pull request as ready for review July 20, 2023 20:32

jreidinger and others added 7 commits July 21, 2023 10:21

add example answers file and fix proxies to use correct defaults

fab42b2

one more fix for correct service name

44d2316

add test for exact yaml format and adapt yaml example file

be5e583

fix formatting

20a7e0a

doc: Fix broken links that 'cargo doc' complains about

b98242a

improve rustdoc for Answers

3827c84

Test an Answer matching everything; better test class names

d9e95c6

mvidner force-pushed the answers_support branch from 4e698ea to d9e95c6 Compare July 25, 2023 14:58

mvidner reviewed Jul 25, 2023

View reviewed changes

joseivanlopez reviewed Jul 26, 2023

View reviewed changes

jreidinger added 2 commits July 26, 2023 13:10

changes

d3095d2

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

266f3d0

joseivanlopez approved these changes Jul 26, 2023

View reviewed changes

jreidinger merged commit 81a5d9b into master Jul 26, 2023

jreidinger deleted the answers_support branch July 26, 2023 13:04

imobachgs mentioned this pull request Aug 2, 2023

Release version 3 #683

Merged

Conversation

jreidinger commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Testing

Follow-ups

Uh oh!

coveralls commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joseivanlopez Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jreidinger commented Jul 19, 2023 •

edited

Loading

coveralls commented Jul 19, 2023 •

edited

Loading

joseivanlopez Jul 26, 2023 •

edited

Loading