Questions in a separate service/process by mvidner · Pull Request #231 · agama-project/agama

mvidner · 2022-07-22T15:02:53Z

Problem

The user-facing callbacks, aka Questions (dbus-api) have so far been limited to a single process/service. However, a major use case are the packager callbacks, and there is a separate o.o.DInstaller.Software service.

All the questions should live in one service. Originally we thought that it should be a separate process hosting only the questions, but so far that hasn't been necessary, the main service is able to host them.

At least a D-Bus call to add a new question is needed.

https://trello.com/c/JY14vDza

Solution

The main idea of the solution is: keep the existing CanAskQuestions mixin, and let it be used the same way, with the questions service being either in the same process or not.

This is achieved by providing def questions_manager { DInstaller::DBus::Clients::QuestionsManager.new } with the same API as DInstaller::QuestionsManager.

Testing

Added new unit tests
Tested manually

Screenshots

(No new UI)

coveralls · 2022-07-22T15:05:53Z

Pull Request Test Coverage Report for Build 2847033928

58 of 66 (87.88%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.3%) to 78.93%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
service/lib/dinstaller/dbus/questions.rb	5	13	38.46%

Totals
Change from base Build 2781581025:	-1.3%
Covered Lines:	596
Relevant Lines:	730

💛 - Coveralls

mvidner · 2022-07-29T08:27:19Z

One thing that is beyond me: when I put the Software service testing_question in the product selection, no UI showed to present it to the user. It only started working when I triggered it (with another testing D-Bus call) at the main installation summary.

Now that I think of it, probably the product selection react component needs to include the questions component? Haven't checked yet.

imobachgs · 2022-07-29T13:03:42Z

[..]

Now that I think of it, probably the product selection react component needs to include the questions component? Haven't checked yet.

Yes, we decided to not put the questions on the product selection page because we did not want other questions (like decrypting storage devices and so on) to come on top of the product selection. We consider product selection the very first step.

Of course, as everything related to D-Installer, we can discuss and decide something different. :-)

mvidner · 2022-08-09T14:19:28Z

DInstaller::QuestionsManager has instance methods add and delete which can fail.
delete obviously fails when its argument has not been added previously, that's fine
addfails when the added question is "duplicate" but there are 2 problems with this

CanAskQuestion#ask does not care about the failure
the criterion for duplication is Question#id but this id is unique for each Question instance, even with the same text+answers, or even if you happened to use the same add call site repeatedly

I don't understand what scenario add is trying to guard against.

Counterpoint: add should always succeed. (DInstaller::DBus::Clients::QuestionsManager#add already does)

@joseivanlopez as you designed this, what do you think?

still in the same old service busctl --system \ call org.opensuse.DInstaller \ /org/opensuse/DInstaller/Questions1 \ org.opensuse.DInstaller.Questions1 New sasas "should I stay or should I go" 2 yes no 1 yes

a later commit makes this run only when an env variable is set

To investigate: when I triggered it locally in product selection, the web frontend was not picking it up. To detect such cases, the front end could set an Asked(displayed) property and the backend could fail a question that was not asked after a timeout. Better than infinitely waiting for a question that the user did not see?

there are some ugly hacks with private members and with making a stub for the question proxy

the #wait implementation is still a hack

also fix some code formatting issues

like DInstaller::Question does. Which fixes a stupid bug where "nineveh" == :nineveh never matches

Both - DInstaller::QuestionsManager - DInstaller::DBus::Clients::QuestionsManager will #wait for the questions passed as argument. For the client case this is rather academic as the #ask interface supports only one question at a time. But in the main service we could be asking a question while another service is waiting for an answer to another, and that is not our business.

jreidinger · 2022-08-11T17:29:31Z

service/lib/dinstaller/dbus/clients/questions_manager.rb

+          # Stupid but simple way: poll the answer property, sleep, repeat
+          loop do
+            questions = questions.find_all { |q| !q.answered? }
+            break if questions.empty?


why not break if questions.all?(&:answered?)

Because our D-Bus clients do not cache properties and this way we don't make calls about questions that we already know are answered.

so please document it. or maybe use better variable name.

jreidinger · 2022-08-11T17:31:16Z

service/lib/dinstaller/dbus/clients/software.rb

          dbus_object.Finish
        end

+        def testing_question


leftover of testing method?

jreidinger · 2022-08-11T17:31:26Z

service/lib/dinstaller/dbus/software/manager.rb

          dbus_method(:Install) { install }
          dbus_method(:Finish) { finish }
+
+          dbus_method(:TestingQuestion) { backend.testing_question }


testing method leftover?

jreidinger · 2022-08-11T17:32:32Z

service/lib/dinstaller/dbus/questions.rb

+          dbus_q = @service.get_node(question_path)&.object
+          raise ArgumentError, "Object path #{question_path} not found" unless dbus_q
+          raise ArgumentError, "Object #{question_path} is not a Question" unless
+            dbus_q.is_a? DInstaller::DBus::Question


please do not use trailing if/unless when it is not single line

jreidinger · 2022-08-11T17:33:29Z

service/lib/dinstaller/manager.rb

+      if ENV["DINSTALLER_TEST_QUESTIONS"] == "1"
+        testing_question
+        software.testing_question
+      end


testing leftover?

jreidinger · 2022-08-11T17:36:14Z

service/lib/dinstaller/questions_manager.rb

      loop do
        on_wait_callbacks.each(&:call)
+        questions = questions.find_all { |q| !q.answered? }
+        break if questions.empty?


same here. I would use single line more readable break if questions.all?(&:answered?)

jreidinger · 2022-08-11T17:36:44Z

service/lib/dinstaller/software.rb


+      # TODO: if a question is asked here, there is no web UI to handle it.
+      # Is it a problem?
+      # testing_question if ENV["DINSTALLER_TEST_QUESTIONS"] == "1"


testing code leftover?

Yes, but a good opportunity to ask about the TODO:

What should happen when some code asks a Question and the web UI has no component to display it?
Currently the code deadlocks waiting for an answer that will not come.
Arguably it will only happen during development.
We could add a displayed? property to Questions and raise an error if a Question is not displayed within 20 seconds.

jreidinger · 2022-08-11T17:39:30Z

service/test/dinstaller/manager_test.rb

  describe "#config_phase" do
+    before do
+      allow(subject).to receive(:testing_question)
+      allow(software).to receive(:testing_question)


Well, now you get me. I am no longer sure if that testing question is testing leftover or not. But in general I do not like it :)

mvidner · 2022-08-12T13:21:32Z

With commit 75d9464 I am removing the testing questions. When we add some actual question, that code can serve as usage example of the API.

compared to DInstaller::QuestionsManager - it does not need callbacks like #on_add - #add will not return an error. CanAskQuestion#ask ignores them anyway.

joseivanlopez · 2022-08-17T08:14:03Z

DInstaller::QuestionsManager has instance methods add and delete which can fail. delete obviously fails when its argument has not been added previously, that's fine addfails when the added question is "duplicate" but there are 2 problems with this

CanAskQuestion#ask does not care about the failure

Yes, it doesn't care. If #add fails, then the question already exists.

the criterion for duplication is Question#id but this id is unique for each Question instance, even with the same text+answers, or even if you happened to use the same add call site repeatedly

The idea is to avoid adding the very same question:

q1 = Question.new("Question 1")
q2 = q1.dup

questions_manager.add(q1) #=> ok
questions_manager.add(q2) #=> fails

Note that the text, options, etc of a question cannot be edited.

I don't understand what scenario add is trying to guard against.

The only important thing is to avoid duplicate questions. Failing "helps" to identify whether the question was already added. But, as you suggested, #add could simply success when the question already exists (without adding the question again).

Counterpoint: add should always succeed. (DInstaller::DBus::Clients::QuestionsManager#add already does)

@joseivanlopez as you designed this, what do you think?

mvidner force-pushed the questions-service branch from 6c776b8 to 9661d86 Compare July 27, 2022 14:40

mvidner force-pushed the questions-service branch 2 times, most recently from bbe9ed6 to 86441c1 Compare August 11, 2022 14:30

mvidner added 18 commits August 11, 2022 16:45

[service] Add a new Question via D-Bus

4d41555

still in the same old service busctl --system \ call org.opensuse.DInstaller \ /org/opensuse/DInstaller/Questions1 \ org.opensuse.DInstaller.Questions1 New sasas "should I stay or should I go" 2 yes no 1 yes

[service] a simple testing question in the same service

2831430

a later commit makes this run only when an env variable is set

[service] Help with consistent indentation in VS Code

78572e6

[service] Change CanAskQuestion protocol to #delete the result of #add

2a2611a

[service] WIP: try asking a remote question

314c70f

[service] continue implementing QuestionsManager+CanAskQuestion

3811e97

there are some ugly hacks with private members and with making a stub for the question proxy

[service] Clean up CanAskQuestion+QuestionsManager

b30be9c

the #wait implementation is still a hack

[service] Make DInstaller::DBus::Question#backend public

3cdafec

[service] Use DInstaller::DBus::Clients::Question

4065680

[service] Fix Manager tests

69ed210

[service] Adapt test to changed #add #delete API truthy/falsy

527d8f1

[service] Ask testing questions only if env DINSTALLER_TEST_QUESTIONS=1

8e08749

also fix some code formatting issues

[service] Fix DInsaller::DBus::Client::Question API to use symbols

db9ad52

like DInstaller::Question does. Which fixes a stupid bug where "nineveh" == :nineveh never matches

[service] Test DInstaller::DBus::Clients::QuestionsManager,Question

1e699e0

[service] argument checking for DInstaller::DBus::Questions#Delete

f3d1a2a

[service] Test CanAskQuestion

7120bf4

mvidner force-pushed the questions-service branch from 86441c1 to 7120bf4 Compare August 11, 2022 14:57

[cli] Test coverage

a4f4c73

mvidner changed the title ~~WIP: Questions in a separate service/process~~ Questions in a separate service/process Aug 11, 2022

mvidner marked this pull request as ready for review August 11, 2022 15:14

jreidinger reviewed Aug 11, 2022

View reviewed changes

[service] Test CanAskQuestion without relying on testing_question

7819fc2

mvidner added 3 commits August 12, 2022 15:26

[service] Remove all the testing questions

75d9464

[service] Simplify QuestionsManager#wait

70a279b

[service] Resolve fixmes for DInstaller::DBus::Clients::QuestionsManager

ea77d16

compared to DInstaller::QuestionsManager - it does not need callbacks like #on_add - #add will not return an error. CanAskQuestion#ask ignores them anyway.

mvidner force-pushed the questions-service branch from 4d6018e to ea77d16 Compare August 12, 2022 13:26

jreidinger approved these changes Aug 12, 2022

View reviewed changes

mvidner merged commit 2b49c34 into master Aug 15, 2022

mvidner deleted the questions-service branch August 15, 2022 08:41

imobachgs mentioned this pull request Aug 29, 2022

Add a D-Bus API for registering questions #169

Closed

mvidner mentioned this pull request Sep 6, 2022

LUKS activation over D-Bus #250

Merged

Conversation

mvidner commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Testing

Screenshots

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 2847033928

💛 - Coveralls

Uh oh!

mvidner commented Jul 29, 2022

Uh oh!

imobachgs commented Jul 29, 2022

Uh oh!

mvidner commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jreidinger Aug 11, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvidner commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseivanlopez commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mvidner commented Jul 22, 2022 •

edited

Loading

coveralls commented Jul 22, 2022 •

edited

Loading

mvidner commented Aug 9, 2022 •

edited

Loading

jreidinger Aug 11, 2022 •

edited

Loading

mvidner commented Aug 12, 2022 •

edited

Loading

joseivanlopez commented Aug 17, 2022 •

edited

Loading