feat: unify progress handling (#1356)

Trello: https://trello.com/c/Miuvai6N/3706-5-better-progress-report-during-installation The main goal of this PR is to implement a better mechanism for displaying progress in the web UI. It takes the product selection progress as inspiration but eliminates hardcoded values. ## Progress API problem When we designed the initial progress API, Agama was quite different. While working on this PR, we have identified a relevant problem: there is no way to have a subprogress in the same service. The manager service defines the main steps (e.g., "Probing Software") and the details are just the progress from software and storage services (e.g., "Refreshing repositories metadata"). As a consequence, we cannot have a detailed "Configuring system" step at the end of the installation with its details (e.g., "Writing users"). You can only have either 1) a "Configuring system" with no details or several separate steps, which is too much. In general, we are telling people that they can use the WS to track the progress, so we should decide on a stable API as soon as possible.
agama-project · Jun 20, 2024 · 7b4db54 · 7b4db54
2 parents 06ab34f + 82265b5
commit 7b4db54
Show file tree

Hide file tree

Showing 24 changed files with 436 additions and 332 deletions.
diff --git a/rust/agama-lib/src/progress.rs b/rust/agama-lib/src/progress.rs
@@ -54,6 +54,7 @@ use zbus::Connection;
 
 /// Represents the progress for an Agama service.
 #[derive(Clone, Default, Debug, Serialize)]
+#[serde(rename_all = "camelCase")]
 pub struct Progress {
     /// Current step
     pub current_step: u32,

diff --git a/rust/agama-lib/src/proxies.rs b/rust/agama-lib/src/proxies.rs
@@ -23,6 +23,10 @@ trait Progress {
     /// TotalSteps property
     #[dbus_proxy(property)]
     fn total_steps(&self) -> zbus::Result<u32>;
+
+    /// Steps property
+    #[dbus_proxy(property)]
+    fn steps(&self) -> zbus::Result<Vec<String>>;
 }
 
 #[dbus_proxy(

diff --git a/rust/agama-server/src/web/common.rs b/rust/agama-server/src/web/common.rs
@@ -165,10 +165,22 @@ struct ProgressState<'a> {
     proxy: ProgressProxy<'a>,
 }
 
-async fn progress(State(state): State<ProgressState<'_>>) -> Result<Json<Progress>, Error> {
+/// Information about the current progress sequence.
+#[derive(Clone, Default, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub struct ProgressSequence {
+    /// Sequence steps if known in advance
+    steps: Vec<String>,
+    #[serde(flatten)]
+    progress: Progress,
+}
+
+async fn progress(State(state): State<ProgressState<'_>>) -> Result<Json<ProgressSequence>, Error> {
     let proxy = state.proxy;
     let progress = Progress::from_proxy(&proxy).await?;
-    Ok(Json(progress))
+    let steps = proxy.steps().await?;
+    let sequence = ProgressSequence { steps, progress };
+    Ok(Json(sequence))
 }
 
 #[pin_project]

diff --git a/rust/package/agama.changes b/rust/package/agama.changes
@@ -1,3 +1,11 @@
+-------------------------------------------------------------------
+Thu Jun 20 05:32:42 UTC 2024 - Imobach Gonzalez Sosa <[email protected]>
+
+- Add support for progress sequences with pre-defined descriptions
+  (gh#openSUSE/agama#1356).
+- Fix the "Progress" signal to use camelCase
+  (gh#openSUSE/agama#1356).
+
 -------------------------------------------------------------------
 Fri Jun 14 06:17:52 UTC 2024 - Imobach Gonzalez Sosa <[email protected]>
 

diff --git a/service/lib/agama/dbus/interfaces/progress.rb b/service/lib/agama/dbus/interfaces/progress.rb
@@ -78,6 +78,15 @@ def progress_finished
           backend.progress.finished?
         end
 
+        # Returns the known step descriptions
+        #
+        # @return [Array<String>]
+        def progress_steps
+          return [] unless backend.progress
+
+          backend.progress.descriptions
+        end
+
         # D-Bus properties of the Progress interface
         #
         # @return [Hash]
@@ -104,6 +113,7 @@ def self.included(base)
               dbus_reader :progress_total_steps, "u", dbus_name: "TotalSteps"
               dbus_reader :progress_current_step, "(us)", dbus_name: "CurrentStep"
               dbus_reader :progress_finished, "b", dbus_name: "Finished"
+              dbus_reader :progress_steps, "as", dbus_name: "Steps"
             end
           end
         end

diff --git a/service/lib/agama/manager.rb b/service/lib/agama/manager.rb
@@ -84,9 +84,11 @@ def config_phase
       service_status.busy
       installation_phase.config
 
-      start_progress(2)
-      progress.step(_("Probing Storage")) { storage.probe }
-      progress.step(_("Probing Software")) { software.probe }
+      start_progress_with_descriptions(
+        _("Analyze disks"), _("Configure software")
+      )
+      progress.step { storage.probe }
+      progress.step { software.probe }
 
       logger.info("Config phase done")
     rescue StandardError => e
@@ -102,26 +104,31 @@ def config_phase
     def install_phase
       service_status.busy
       installation_phase.install
-      start_progress(7)
+      start_progress_with_descriptions(
+        _("Prepare disks"),
+        _("Install software"),
+        _("Configure the system")
+      )
 
       Yast::Installation.destdir = "/mnt"
 
-      progress.step(_("Partitioning")) do
+      progress.step do
         storage.install
         proxy.propose
         # propose software after /mnt is already separated, so it uses proper
         # target
         software.propose
       end
 
-      progress.step(_("Installing Software")) { software.install }
-
-      on_target do
-        progress.step(_("Writing Users")) { users.write }
-        progress.step(_("Writing Network Configuration")) { network.install }
-        progress.step(_("Saving Language Settings")) { language.finish }
-        progress.step(_("Writing repositories information")) { software.finish }
-        progress.step(_("Finishing storage configuration")) { storage.finish }
+      progress.step { software.install }
+      progress.step do
+        on_target do
+          users.write
+          network.install
+          language.finish
+          software.finish
+          storage.finish
+        end
       end
 
       logger.info("Install phase done")

diff --git a/service/lib/agama/progress.rb b/service/lib/agama/progress.rb
@@ -24,9 +24,15 @@ module Agama
   #
   # It allows to configure callbacks to be called on each step and also when the progress finishes.
   #
+  # In most cases all steps are known in advance (e.g., "probing software", "probing storage", etc.)
+  # but, in some situations, only the number of steps is known (e.g., "Installing package X").
+  #
+  # Use the Progress.with_descriptions to initialize a progress with known step descriptions and
+  # Progress.with_size when only the number of steps is known
+  #
   # @example
   #
-  #   progress = Progress.new(3)                    # 3 steps
+  #   progress = Progress.with_size(3)              # 3 steps
   #   progress.on_change { puts progress.message }  # configures callbacks
   #   progress.on_finish { puts "finished"  }       # configures callbacks
   #
@@ -45,6 +51,14 @@ module Agama
   #
   #   progress.finished?                            #=> true
   #   progress.current_step                         #=> nil
+  #
+  # @example Progress with known step descriptions
+  #
+  #   progress = Progress.with_descriptions(["Partitioning", "Installing", "Finishing"])
+  #   progress.step { partitioning }                      # next step
+  #   progress.current_step.description                   #=> "Partitioning"
+  #   progress.step("Installing packages") { installing } # overwrite the description
+  #   progress.current_step.description                   # "Installing packages"
   class Progress
     # Step of the progress
     class Step
@@ -73,11 +87,30 @@ def initialize(id, description)
     # @return [Integer]
     attr_reader :total_steps
 
+    # Step descriptions in case they are known
+    #
+    # @return [Array<String>]
+    attr_reader :descriptions
+
+    class << self
+      def with_size(size)
+        new(size: size)
+      end
+
+      def with_descriptions(descriptions)
+        new(descriptions: descriptions)
+      end
+    end
+
     # Constructor
     #
-    # @param total_steps [Integer] total number of steps
-    def initialize(total_steps)
-      @total_steps = total_steps
+    # @param descriptions [Array<String>] Steps of the progress sequence. This argument
+    #   has precedence over the `size`
+    # @param size [Integer] total number of steps of the progress sequence
+    def initialize(descriptions: [], size: nil)
+      @descriptions = descriptions || []
+      @total_steps = descriptions.size unless descriptions.empty?
+      @total_steps ||= size
       @current_step = nil
       @counter = 0
       @finished = false
@@ -99,15 +132,16 @@ def current_step
     # It calls the `on_change` callbacks and then runs the given block, if any. It also calls
     # `on_finish` callbacks after the last step.
     #
-    # @param description [String] description of the step
+    # @param description [String, nil] description of the step
     # @param block [Proc]
     #
     # @return [Object, nil] result of the given block or nil if no block is given
-    def step(description, &block)
+    def step(description = nil, &block)
       return if finished?
 
       @counter += 1
-      @current_step = Step.new(@counter, description)
+      step_description = description || description_for(@counter)
+      @current_step = Step.new(@counter, step_description)
       @on_change_callbacks.each(&:call)
 
       result = block_given? ? block.call : nil
@@ -154,5 +188,11 @@ def to_s
 
       "#{current_step.description} (#{@counter}/#{total_steps})"
     end
+
+  private
+
+    def description_for(step)
+      @descriptions[step - 1] || ""
+    end
   end
 end
diff --git a/service/lib/agama/software/callbacks/progress.rb b/service/lib/agama/software/callbacks/progress.rb
@@ -41,8 +41,8 @@ def initialize(pkg_count, progress)
         end
 
         def setup
-          Yast::Pkg.CallbackDonePackage(
-            fun_ref(method(:package_installed), "string (integer, string)")
+          Yast::Pkg.CallbackStartPackage(
+            fun_ref(method(:start_package), "void (string, string, string, integer, boolean)")
           )
         end
 
@@ -55,12 +55,8 @@ def fun_ref(method, signature)
           Yast::FunRef.new(method, signature)
         end
 
-        # TODO: error handling
-        def package_installed(_error, _reason)
-          @installed += 1
-          progress.step(msg)
-
-          ""
+        def start_package(package, _file, _summary, _size, _other)
+          progress.step("Installing #{package}")
         end
 
         def msg

diff --git a/service/lib/agama/software/manager.rb b/service/lib/agama/software/manager.rb
@@ -132,11 +132,11 @@ def probe
         logger.info "Probing software"
 
         if repositories.empty?
-          start_progress(3)
+          start_progress_with_size(3)
           Yast::PackageCallbacks.InitPackageCallbacks(logger)
           progress.step(_("Initializing sources")) { add_base_repos }
         else
-          start_progress(2)
+          start_progress_with_size(2)
         end
 
         progress.step(_("Refreshing repositories metadata")) { repositories.load }
@@ -178,7 +178,7 @@ def install
         Yast::Pkg.TargetLoad
 
         steps = proposal.packages_count
-        start_progress(steps)
+        start_progress_with_size(steps)
         Callbacks::Progress.setup(steps, progress)
 
         # TODO: error handling
@@ -197,14 +197,11 @@ def install
 
       # Writes the repositories information to the installed system
       def finish
-        start_progress(1)
-        progress.step(_("Writing repositories to the target system")) do
-          Yast::Pkg.SourceSaveAll
-          Yast::Pkg.TargetFinish
-          # copy the libzypp caches to the target
-          copy_zypp_to_target
-          registration.finish
-        end
+        Yast::Pkg.SourceSaveAll
+        Yast::Pkg.TargetFinish
+        # copy the libzypp caches to the target
+        copy_zypp_to_target
+        registration.finish
       end
 
       # Determine whether the given tag is provided by the selected packages

diff --git a/service/lib/agama/storage/finisher.rb b/service/lib/agama/storage/finisher.rb
@@ -50,7 +50,7 @@ def initialize(logger, config, security)
       # Execute the final storage actions, reporting the progress
       def run
         steps = possible_steps.select(&:run?)
-        start_progress(steps.size)
+        start_progress_with_size(steps.size)
 
         on_target do
           steps.each do |step|

diff --git a/service/lib/agama/storage/manager.rb b/service/lib/agama/storage/manager.rb
@@ -107,7 +107,7 @@ def on_probe(&block)
 
       # Probes storage devices and performs an initial proposal
       def probe
-        start_progress(4)
+        start_progress_with_size(4)
         config.pick_product(software.selected_product)
         check_multipath
         progress.step(_("Activating storage devices")) { activate_devices }
@@ -120,7 +120,7 @@ def probe
 
       # Prepares the partitioning to install the system
       def install
-        start_progress(4)
+        start_progress_with_size(4)
         progress.step(_("Preparing bootloader proposal")) do
           # first make bootloader proposal to be sure that required packages are installed
           proposal = ::Bootloader::ProposalClient.new.make_proposal({})

diff --git a/service/lib/agama/with_progress.rb b/service/lib/agama/with_progress.rb
@@ -30,22 +30,37 @@ class NotFinishedProgress < StandardError; end
     # @return [Progress, nil]
     attr_reader :progress
 
+    # Creates a new progress with a given number of steps
+    #
+    # @param size [Integer] Number of steps
+    def start_progress_with_size(size)
+      start_progress(size: size)
+    end
+
+    # Creates a new progress with a given set of steps
+    #
+    # @param descriptions [Array<String>] Steps descriptions
+    def start_progress_with_descriptions(*descriptions)
+      start_progress(descriptions: descriptions)
+    end
+
     # Creates a new progress
     #
     # @raise [RuntimeError] if there is an unfinished progress.
     #
-    # @param total_steps [Integer] total number of the steps for the progress.
-    def start_progress(total_steps)
+    # @param args [*Hash] Progress constructor arguments.
+    def start_progress(args)
       raise NotFinishedProgress if progress && !progress.finished?
 
       on_change_callbacks = @on_progress_change_callbacks || []
       on_finish_callbacks = @on_progress_finish_callbacks || []
 
-      @progress = Progress.new(total_steps).tap do |progress|
+      @progress = Progress.new(**args).tap do |progress|
         progress.on_change { on_change_callbacks.each(&:call) }
         progress.on_finish { on_finish_callbacks.each(&:call) }
       end
     end
+    private :start_progress
 
     # Finishes the current progress
     def finish_progress

diff --git a/service/package/rubygem-agama-yast.changes b/service/package/rubygem-agama-yast.changes
@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Thu Jun 20 05:25:49 UTC 2024 - Imobach Gonzalez Sosa <[email protected]>
+
+- Add support for progress sequences with pre-defined descriptions
+  (gh#openSUSE/agama#1356).
+
 -------------------------------------------------------------------
 Wed Jun 19 06:04:46 UTC 2024 - Ladislav Slezák <[email protected]>