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

Fix storage config #1854

Merged
merged 2 commits into from
Dec 23, 2024
Merged

Fix storage config #1854

merged 2 commits into from
Dec 23, 2024

Conversation

jreidinger
Copy link
Contributor

Problem

There is collision between #1848 and #1840 where bootloader adds another interface with Get/SetConfig on Storage service. This result that generic access no longer works and needs access via specific interface.

reported on slack https://suse.slack.com/archives/C02TLF25571/p1734924853266999

Solution

Change code to avoid ambitious code.

Testing

  • Tested manually

@coveralls
Copy link

Pull Request Test Coverage Report for Build 12471458359

Details

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage remained the same at 70.87%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/lib/agama/dbus/clients/storage.rb 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
service/lib/agama/dbus/clients/storage.rb 1 87.88%
service/service/lib/agama/dbus/clients/storage.rb 2 87.88%
Totals Coverage Status
Change from base Build 12433973856: 0.0%
Covered Lines: 17132
Relevant Lines: 24174

💛 - Coveralls

Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

Although DBus stuff are not my thing, rationale and code looks good to me. If you tested it and it works, I guess you can go ahead.

Nevertheless, I have left a comment to be discussed in a near future, if needed.

@@ -67,7 +67,8 @@ def finish
#
# @return [Hash]
def config
serialized_config = dbus_object.GetConfig
# Use storage iface to avoid collision with bootloader iface
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I wonder if it worth to always use the corresponding iface to avoid more errors like the one this commit fixes and get rid of specific comment.

Something to be discussed, not a blocker for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it is zbus way to always use combination of service, interface and object. On other hand ruby-dbus use different way and allow access throught interface, but for simplicity if method in unique, it is added directly to object. My original fix change dbus_object to always return interface, but then mixins start failing as it uses some methods that is not in interface. (during test e.g. this mixins failing https://github.com/agama-project/agama/blob/master/service/lib/agama/dbus/clients/with_service_status.rb#L43 ).
Maybe it will be better to have beside dbus_object also dbus_interface method on which we should call all needed storage methods ( that is idea that I get after thinking a bit more about it after I have working solution and not enough time to test different one ).

@dgdavid
Copy link
Contributor

dgdavid commented Dec 23, 2024

Merging it as requested in the internal team-yast Slack channel.

@dgdavid dgdavid merged commit c3d85bb into master Dec 23, 2024
10 checks passed
@dgdavid dgdavid deleted the fix_storage_config branch December 23, 2024 19:51
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.

3 participants