-
Notifications
You must be signed in to change notification settings - Fork 43
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
Change Questions D-Bus API to fit new proposal #637
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I got it correctly, you are wrapping the questions objects (e.g., GenericQuestion
) into other structs (e.g., GenericQuestionObject
) which implements support for answering the questions, right?
About answering the questions, I see you decided to go with a trait. I am not sure how other kinds of questions would be answered yet (I guess you need to work more on that part).
In general, the approach looks good to me.
@imobachgs well, idea is to have object For answering idea is to have that trait |
I wanted to point out that if the D-Bus client needs to change, then we should change the number in the API, to |
More and more I am thinking about how to implement answer strategy from a file, the more I see problematic our approach with specific questions ( for now just Luks Activation ). With current approach it needs:
So what I propose is:
with this approach the need to know about question type is:
So how it will look on frontend side for LuksActivation:
note: number of attempts is also in data map. So to short it up, with current implementation polymorphism does not work at all, as every component that touch questions needs to know about all specialized types. @imobachgs @joseivanlopez how do you see it? |
Hi all, We have been discussing this problem and, although we did not come to a conclusion, we have some ideas. On the one hand, as @jreidinger noticed, the current API resembles a hierarchy: you have a I do not like that much passing hashes through D-Bus because:
Having said that, if it is our best bet, I will not oppose it. However, we wonder whether we could come up with a better (and more generic) model. Instead of having |
I understand (1) but not the subsequent points, please show examples
Not sure what you mean, but if you mean a hash over D-Bus, it would have to be one in parameter and another out parameter as D-Bus cannot do inout parameters or "modify argument objects". |
Regarding various kinds/classes of questions: Currently we have a specialized LuksQuestion that the frontend must handle specially. If we change that to have only one QuestionNG with a complex hash argument, the frontend will still have to handle it specially, right? It will be just obfuscated in the API. Unless we make the API so generic that it becomes a Dialog Interpreter, with Label, Hbox, Vbox, InputField, Button... 🤣 I maybe missing @jreidinger 's point, Josef please give examples. |
Ok, to clarify Question has now in this PR several properties including data map type and class string type.
As mention it is property. So only change is that property will be rw and not ro.
Well, frontend will have to handle it specially. That is true. But it will be simple interface and some other parts does not need to handle it specially like matching against answer or capturing question. Also now it needs specialized constructor in Questions object That is why we discuss on call about changing it to real "composition" that works much better with DBus ( at least to my limited knowledge ) that can be generic enough to allow easy extension in future and still kind of limit amount of questions that needs special handling. So that is what @imobachgs writes that we can have interfaces like
|
That's Questions::new_question
Not part of the PR yet, I don't see such |
yeap and it returns object path, so no need for any data as data will be filled as part of answer.
yeap, not yet part of PR, but I can easily add it. Question is what to do with that LuksQuestion. Maybe what I can do now to unblock this PR is to just rename luks question to WithPassword and it move creation of question to caller and not ruby. And then lets see if we need to change it in future. BTW to capture it also here. Agreement is to not bump version of API until we release some stable agama version as API is still kind of flexible and we are only user now. |
Oh just go ahead, I don't want to block anything
OK fine |
I would say that dbus API should be final now. Implementation is still something that can change. E.g. I do not like much how mixins are implemented and if more types or wilder combination of mixins can happen, then we will probably need something like
that allows variation of mixins and its combination and have methods on top of it that acts differently based on available mixins. |
@@ -16,6 +16,16 @@ impl GenericQuestionObject { | |||
self.0.id | |||
} | |||
|
|||
#[dbus_interface(property)] | |||
pub fn class(&self) -> &str { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update also doc/dbus/org.opensuse.Agama.Questions1.Generic.doc.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do, my plan is to update doc and test when everything works together as I am not sure if we not need to modify more.
@@ -58,22 +69,19 @@ impl LuksQuestionObject { | |||
pub fn set_password(&mut self, value: &str) -> () { | |||
self.0.password = value.to_string(); | |||
} | |||
|
|||
#[dbus_interface(property)] | |||
pub fn attempt(&self) -> u8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to discard attempt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeap, as I think it is not generic password specific stuff. So my plan now is to move it do data as ro data and if we need it more often, we can add WithAttempt mixin for questions that needs to capture it.
} | ||
|
||
trait AnswerStrategy { | ||
/// TODO: find way to be able to answer any type of question, not just generic | ||
/// Provides answer for generic question |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Provides answer for generic question | |
/// Provides answer for generic question. | |
/// Returns one of the question options. |
What does the None
return value mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None
means in both cases that Answering strategy does not provide it. As FileStrategy will be another implementation of this trait and if answer is not specified there, it should return None.
fn answer(&self, question: &GenericQuestion) -> Option<String>; | ||
/// Provides answer and password for base question with password | ||
fn answer_with_password(&self, question: &WithPassword) -> (Option<String>, Option<String>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again I don't understand why the returns are optional
Also, is their optionality independent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and yes. None means that strategy does not provide it and it is up to question to decide what it means. E.g. Default strategy for password should return something like (Some(self.default_answer), None)
which means use default answer and does not provide password.
@@ -224,6 +239,19 @@ impl Questions { | |||
} | |||
} | |||
} | |||
|
|||
/// tries to provide answer to question using answer strategies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe what happens when multiple strategies do (not) provide an answer. Is there a short circuit? What if answers conflict?
I shouldn't need to read the implementation to know this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will document it.
rust/agama-lib/src/questions.rs
Outdated
@@ -41,48 +41,20 @@ impl GenericQuestion { | |||
} | |||
} | |||
|
|||
/// Specialized question for Luks partition activation | |||
/// Composition for questions which include password. | |||
/// TODO: research a bit how ideally do mixins in rust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO has been resolved by this code already?
Or add pointers to ideas and examples to comments/commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will write it before comment in this PR. I will try to document the idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, it looks good to me. I only have a few questions that can be addressed in the following PR.
} | ||
} | ||
|
||
pub fn object_path(&self) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions are coupled with D-Bus, right? I wonder whether this struct should live in agama-lib
or directly in agama-dbus-server
. After all, any other component should use the D-Bus API to deal with questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imobachgs reason why it is is in agama-lib
and not in agama-dbus-server
is that object path is useful also for client side. So my general rule is if it is useful for server and client then have it in lib, so both CLI and dbus server can use it. If it is useful only for one of it, then it can live there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it is fine. I tend to move the stuff to agama-lib
only when I use it from other parts, but if you find it useful, it is OK.
rust/agama-lib/src/questions.rs
Outdated
|
||
impl WithPassword { | ||
pub fn new(base: GenericQuestion) -> Self { | ||
info!("Adding to question with password interface."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np: Most probably I would move the logging to the D-Bus interfaces objects. If you want to write some unit tests here, you will get a log message.
/// Confirmed answer. If empty then not answered yet. | ||
answer: String, | ||
} | ||
struct GenericQuestionObject(questions::GenericQuestion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of the newtype pattern. Actually, I wonder whether the object_path
function could be here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as written above I think it can be useful also for client implementation e.g. when you want to remove question, you should also provide object_path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and yeah, about newtype pattern, I hear about it multiple times from you and here I think it fits really well.
BaseWithPassword, | ||
} | ||
|
||
trait AnswerStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect a description of what this trait does and how is expected to be used. It can be in a different PR, as we agreed offline, but please do not forget about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, perhaps it deserves its own strategies
module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeap, own module can happen easily with adding FileStrategy to get it from file. I just do not want to over-complicate it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, no problem. We can move later.
.await?; | ||
self.questions.insert(id, QuestionType::Generic); | ||
self.questions.insert(id, QuestionType::Base); | ||
Ok(object_path) | ||
} | ||
|
||
/// creates new specialized luks activation question without answer and password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// creates new specialized luks activation question without answer and password | |
/// creates new question to ask for a password with no default answer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not right. There have to be always default strategy. I just want to mention here that created question has empty answer
and password
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then I did not understand the comment 😄
Rust zbus unfortunately outputs the provided interfaces in a random order in the introspection data, so we have to sort them before comparing
setup-service in particular
they are semantically the same, just with interfaces sorted by name
…/cleanup() had to manually re-add the interfaces missing on x86: <interface name="org.opensuse.Agama.Storage1.DASD.Manager"> <interface name="org.opensuse.Agama.Storage1.ZFCP.Manager">
The @Result output argument is unfortunately unnamed in zbus(Rust) introspection so we have to document it in plain text
af76190
to
f6429d7
Compare
} | ||
|
||
# "dot dot name" | ||
# "slash slash name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for explanation of variable
<!-- | ||
UseDefaultAnswer: | ||
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. | ||
--> | ||
<method name="UseDefaultAnswer" /> | ||
<method name="UseDefaultAnswer"> | ||
</method> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just curious why this change is needed? It is due to output of introspect method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, introspection has the whitespace text node. We know it can be ignored because we know the schema/DTD but the checker is too simple for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some typos. Otherwise, LGTM. Great job! Thanks!
/// and try to find the first strategy that provides answer. When | ||
/// answer is provided, it returns immediately. | ||
fn fill_answer(&self, question: &mut GenericQuestion) { | ||
for strategy in self.answer_strategies.iter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion for the future (not even for this PR), but I would say that a version using combinators would be easier to read. Something like (not tested):
let answer = self
.answer_strategies
.iter()
.find_map(|s| s.answer(question));
// Note: I did not use `and_then` because it expects the closure to return an Option, and that's not the case.
if let Some(value) = answer {
question.answer = value;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! LGTM now
Co-authored-by: Imobach González Sosa <[email protected]>
Problem
Questions does not allow yet automatic answering of question.
Solution
This is the first phase, which adapt API on backend and frontend side. Support for answers file is not part of this PR
Testing
TODO