Skip to content

Software proposal#202

Merged
imobachgs merged 26 commits intomulti-processfrom
software-proposal
Jun 20, 2022
Merged

Software proposal#202
imobachgs merged 26 commits intomulti-processfrom
software-proposal

Conversation

@imobachgs
Copy link
Copy Markdown
Contributor

@imobachgs imobachgs commented Jun 17, 2022

Follow-up of #201 (see #168 for the original issue).

  • Add a D-Bus API to add/get/remove package proposals resolvables (/org/opensuse/DInstaller/Software/Proposal1).
  • Add replacement modules for Package, PackageProposal and SpaceCalculation.

Open questions

  • We need to add support for Package#Available and Package#Installed to D-Bus. Where? Should we merge all the calls in a single D-Bus object? Or should we add another one in addition to the Proposal1 object?

Screenshots

As the web UI needs to be adapted, what about a dinstallerctl screenshot? 😅

dinstallerctl

Tasks

  • Improve unit testing coverage.
  • Implement a Package#Available D-Bus method.

Comment thread service/lib/dinstaller/dbus/y2dir/manager/modules/PackagesProposal.rb Outdated
Base automatically changed from split-software-service to multi-process June 17, 2022 09:53
@imobachgs imobachgs force-pushed the software-proposal branch from 11c01a1 to ee12d97 Compare June 17, 2022 11:18
@imobachgs imobachgs force-pushed the software-proposal branch from 643ee84 to 9978963 Compare June 17, 2022 14:46
@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 2516233281

  • -178 of 190 (6.32%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+17.9%) to 93.466%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/lib/dinstaller/dbus/software_service.rb 0 2 0.0%
service/lib/dinstaller/dbus/software.rb 0 3 0.0%
service/lib/dinstaller/dbus/clients/software.rb 9 13 69.23%
service/lib/dinstaller/dbus/y2dir/software/modules/SpaceCalculation.rb 0 15 0.0%
service/lib/dinstaller/dbus/y2dir/manager/modules/Package.rb 0 16 0.0%
service/lib/dinstaller/dbus/y2dir/manager/modules/PackagesProposal.rb 0 30 0.0%
service/lib/dinstaller/dbus/software/proposal.rb 0 37 0.0%
service/lib/dinstaller/dbus/software/manager.rb 0 71 0.0%
Totals Coverage Status
Change from base Build 2514748462: 17.9%
Covered Lines: 329
Relevant Lines: 352

💛 - Coveralls

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 17, 2022

Pull Request Test Coverage Report for Build 2529204048

  • 16 of 198 (8.08%) changed or added relevant lines in 12 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-1.7%) to 73.892%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/lib/dinstaller/manager.rb 1 2 50.0%
service/lib/dinstaller/software.rb 1 2 50.0%
service/lib/dinstaller/dbus/software_service.rb 0 2 0.0%
service/lib/dinstaller/dbus/software.rb 0 3 0.0%
service/lib/dinstaller/dbus/y2dir/modules/Autologin.rb 0 3 0.0%
service/lib/dinstaller/dbus/clients/software.rb 12 17 70.59%
service/lib/dinstaller/dbus/y2dir/manager/modules/Package.rb 0 13 0.0%
service/lib/dinstaller/dbus/y2dir/software/modules/SpaceCalculation.rb 0 15 0.0%
service/lib/dinstaller/dbus/y2dir/manager/modules/PackagesProposal.rb 0 29 0.0%
service/lib/dinstaller/dbus/software/proposal.rb 0 41 0.0%
Files with Coverage Reduction New Missed Lines %
service/lib/dinstaller/dbus/y2dir/modules/Autologin.rb 1 0%
Totals Coverage Status
Change from base Build 2514748462: -1.7%
Covered Lines: 2020
Relevant Lines: 2730

💛 - Coveralls

Copy link
Copy Markdown
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

I see we still have a DBus::Clients::DInstaller providing #provision_selected? and #provision_selected? methods. I think we can drop that service.

module Software
# D-Bus object to manage software installation
class Manager < ::DBus::Object
PATH = "/org/opensuse/DInstaller/Software1"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be /org/opensuse/DInstaller/Software/Manager1? Similar to we did with storage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not see the need to have a manager path at all. For instance, we have /org/opensuse/DInstaller/Language1. I am not against it, though, but we should define what we prefer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Originally, the name of this file was main.rb. Would it be any better?

Copy link
Copy Markdown
Contributor

@joseivanlopez joseivanlopez Jun 20, 2022

Choose a reason for hiding this comment

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

I got confused, we don't have /org/opensuse/DInstaller/Storage/Manager1. For the future storage service, I would expect something like:

  • /org/opensuse/DInstaller/Storage1
  • /org/opensuse/DInstaller/Storage/Proposal1
  • /org/opensuse/DInstaller/Storage/Actions1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right. I will get rid of that client.

Comment thread service/lib/dinstaller/dbus/software/manager.rb Outdated
Comment thread service/lib/dinstaller/dbus/software/manager.rb
Comment thread service/lib/dinstaller/dbus/software/proposal.rb Outdated
Comment thread service/lib/dinstaller/dbus/y2dir/manager/modules/Package.rb Outdated
@imobachgs imobachgs marked this pull request as ready for review June 20, 2022 09:34
Comment thread service/lib/dinstaller/dbus/clients/software.rb
dbus_interface SOFTWARE_INTERFACE do
dbus_reader :available_base_products, "a(ssa{sv})"

dbus_watcher :available_base_products
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would say the watcher is not needed.

Copy link
Copy Markdown
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

@imobachgs imobachgs merged commit b058d19 into multi-process Jun 20, 2022
@imobachgs imobachgs deleted the software-proposal branch June 20, 2022 13:38
@imobachgs imobachgs restored the software-proposal branch November 23, 2022 09:03
@imobachgs imobachgs deleted the software-proposal branch November 23, 2022 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants