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

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

Merged
merged 19 commits into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 3 additions & 0 deletions doc/answers_example.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
answers:
- class: storage.luks_activation
answer: "skip"
9 changes: 6 additions & 3 deletions doc/dbus/bus/org.opensuse.Agama.Questions1.bus.xml
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,13 @@
<method name="Delete">
<arg name="question" type="o" direction="in"/>
</method>
<method name="AddAnswerFile">
<arg name="path" type="s" direction="in"/>
</method>
<!--
sets questions to be answered by default answer instead of asking user
property that defines if questions is interactive or automatically answered with
default answer
-->
<method name="UseDefaultAnswer">
</method>
<property name="Interactive" type="b" access="readwrite"/>
</interface>
</node>
26 changes: 19 additions & 7 deletions doc/dbus/org.opensuse.Agama.Questions1.doc.xml
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,30 @@ when the question is answered and the answer is successfully read.
<arg name="data" direction="in" type="a{ss}"/>
<arg direction="out" type="o"/>
</method>
<!--
Delete:
@question: object path of question that should be deleted.

Deletes question. Useful when question is answered and service that asks
already read answer. Usually called by the one that previously created it.
-->
<method name="Delete">
<arg name="question" direction="in" type="o"/>
<arg name="question" type="o" direction="in"/>
</method>

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


Switches questions to be automatically answered by default answer.
After this method call each follow up question is immediatelly answered.
Useful for doing non interactive installation.
Adds file with list of predefined answers that will be used to
automatically answer matching questions.
-->
<method name="UseDefaultAnswer">
<method name="AddAnswerFile">
<arg name="path" type="s" direction="in"/>
</method>
<!--
property that defines if questions is interactive or automatically answered with
default answer
-->
<property name="Interactive" type="b" access="readwrite"/>
</interface>
</node>
34 changes: 29 additions & 5 deletions rust/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 24 additions & 19 deletions rust/agama-cli/src/questions.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
use agama_lib::connection;
use agama_lib::proxies::Questions1Proxy;
use anyhow::{Context, Ok};
use anyhow::Context;
use clap::{Args, Subcommand, ValueEnum};

#[derive(Subcommand, Debug)]
pub enum QuestionsCommands {
/// Set mode for answering questions.
Mode(ModesArgs),
Answers {
path: String,
},
}

#[derive(Args, Debug)]
Expand All @@ -20,29 +23,31 @@ pub enum Modes {
Interactive,
NonInteractive,
}
// TODO when more commands is added, refactor and add it to agama-lib and share a bit of functionality
async fn set_mode(value: Modes) -> anyhow::Result<()> {
match value {
Modes::NonInteractive => {
let connection = connection().await?;
let proxy = Questions1Proxy::new(&connection)
.await
.context("Failed to connect to Questions service")?;

// TODO: how to print dbus error in that anyhow?
proxy
.use_default_answer()
.await
.context("Failed to set default answer")?;
}
Modes::Interactive => log::info!("not implemented"), //TODO do it
}
async fn set_mode(proxy: Questions1Proxy<'_>, value: Modes) -> anyhow::Result<()> {
// TODO: how to print dbus error in that anyhow?
proxy
.set_interactive(value == Modes::Interactive)
.await
.context("Failed to set mode for answering questions.")
}

Ok(())
async fn set_answers(proxy: Questions1Proxy<'_>, path: String) -> anyhow::Result<()> {
// TODO: how to print dbus error in that anyhow?
proxy
.add_answer_file(path.as_str())
.await
.context("Failed to set answers from answers file")
}

pub async fn run(subcommand: QuestionsCommands) -> anyhow::Result<()> {
let connection = connection().await?;
let proxy = Questions1Proxy::new(&connection)
.await
.context("Failed to connect to Questions service")?;

match subcommand {
QuestionsCommands::Mode(value) => set_mode(value.value).await,
QuestionsCommands::Mode(value) => set_mode(proxy, value.value).await,
QuestionsCommands::Answers { path } => set_answers(proxy, path).await,
}
}
2 changes: 2 additions & 0 deletions rust/agama-dbus-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@ async-std = { version = "1.12.0", features = ["attributes"]}
uuid = { version = "1.3.4", features = ["v4"] }
parking_lot = "0.12.1"
thiserror = "1.0.40"
serde = { version = "1.0.152", features = ["derive"] }
serde_yaml = "0.9.24"
2 changes: 1 addition & 1 deletion rust/agama-dbus-server/src/network/action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use agama_lib::network::types::DeviceType;

/// Networking actions, like adding, updating or removing connections.
///
/// These actions are meant to be processed by [crate::system::NetworkSystem], updating the model
/// These actions are meant to be processed by [crate::network::system::NetworkSystem], updating the model
/// and the D-Bus tree as needed.
#[derive(Debug)]
pub enum Action {
Expand Down
10 changes: 5 additions & 5 deletions rust/agama-dbus-server/src/network/dbus/interfaces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ impl Device {
///
/// Possible values: 0 = loopback, 1 = ethernet, 2 = wireless.
///
/// See [crate::model::DeviceType].
/// See [agama_lib::network::types::DeviceType].
#[dbus_interface(property, name = "Type")]
pub fn device_type(&self) -> u8 {
self.device.type_ as u8
Expand Down Expand Up @@ -124,7 +124,7 @@ impl Connections {
/// Adds a new network connection.
///
/// * `id`: connection name.
/// * `ty`: connection type (see [crate::model::DeviceType]).
/// * `ty`: connection type (see [agama_lib::network::types::DeviceType]).
pub async fn add_connection(&mut self, id: String, ty: u8) -> zbus::fdo::Result<()> {
let actions = self.actions.lock();
actions
Expand Down Expand Up @@ -274,7 +274,7 @@ impl Ipv4 {
///
/// Possible values: "disabled", "auto", "manual" or "link-local".
///
/// See [crate::model::IpMethod].
/// See [crate::network::model::IpMethod].
#[dbus_interface(property)]
pub fn method(&self) -> String {
let connection = self.get_connection();
Expand Down Expand Up @@ -401,7 +401,7 @@ impl Wireless {
///
/// Possible values: "unknown", "adhoc", "infrastructure", "ap" or "mesh".
///
/// See [crate::model::WirelessMode].
/// See [crate::network::model::WirelessMode].
#[dbus_interface(property)]
pub fn mode(&self) -> String {
let connection = self.get_wireless();
Expand Down Expand Up @@ -442,7 +442,7 @@ impl Wireless {
/// Possible values: "none", "owe", "ieee8021x", "wpa-psk", "sae", "wpa-eap",
/// "wpa-eap-suite-b192".
///
/// See [crate::model::SecurityProtocol].
/// See [crate::network::model::SecurityProtocol].
#[dbus_interface(property)]
pub fn security(&self) -> String {
let connection = self.get_wireless();
Expand Down
58 changes: 53 additions & 5 deletions rust/agama-dbus-server/src/questions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ use anyhow::Context;
use log;
use zbus::{dbus_interface, fdo::ObjectManager, zvariant::ObjectPath, Connection};

mod answers;

#[derive(Clone, Debug)]
struct GenericQuestionObject(questions::GenericQuestion);

Expand Down Expand Up @@ -81,7 +83,11 @@ enum QuestionType {
}

/// Trait for objects that can provide answers to all kind of Question.
///
/// If no strategy is selected or the answer is unknown, then ask to the user.
trait AnswerStrategy {
/// Id for quick runtime inspection of strategy type
fn id(&self) -> u8;
/// Provides answer for generic question
///
/// I gets as argument the question to answer. Returned value is `answer`
Expand All @@ -103,7 +109,17 @@ trait AnswerStrategy {
/// AnswerStrategy that provides as answer the default option.
struct DefaultAnswers;

impl DefaultAnswers {
pub fn id() -> u8 {
1
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}

impl AnswerStrategy for DefaultAnswers {
fn id(&self) -> u8 {
DefaultAnswers::id()
}

fn answer(&self, question: &GenericQuestion) -> Option<String> {
Some(question.default_option.clone())
}
Expand Down Expand Up @@ -227,11 +243,43 @@ impl Questions {
Ok(())
}

/// sets questions to be answered by default answer instead of asking user
async fn use_default_answer(&mut self) -> Result<(), Error> {
log::info!("Answer questions with default option");
self.answer_strategies.push(Box::new(DefaultAnswers {}));
Ok(())
/// property that defines if questions is interactive or automatically answered with
/// default answer
#[dbus_interface(property)]
fn interactive(&self) -> bool {
let last = self.answer_strategies.last();
if let Some(real_strategy) = last {
real_strategy.id() != DefaultAnswers::id()
Copy link
Contributor

@joseivanlopez joseivanlopez Jul 26, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ).

} else {
true
}
}

#[dbus_interface(property)]
fn set_interactive(&mut self, value: bool) {
if value != self.interactive() {
log::info!("interactive value unchanged - {}", value);
return;
}

log::info!("set interactive to {}", value);
if value {
jreidinger marked this conversation as resolved.
Show resolved Hide resolved
self.answer_strategies.pop();
} else {
self.answer_strategies.push(Box::new(DefaultAnswers {}));
}
}

fn add_answer_file(&mut self, path: String) -> Result<(), Error> {
log::info!("Adding answer file {}", path);
let answers = answers::Answers::new_from_file(path.as_str());
match answers {
Ok(answers) => {
self.answer_strategies.push(Box::new(answers));
Ok(())
}
Err(e) => Err(e.into()),
}
}
}

Expand Down
Loading
Loading