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

feat(rust): adapt to zbus 5 #1716

Merged
merged 26 commits into from
Nov 6, 2024
Merged

feat(rust): adapt to zbus 5 #1716

merged 26 commits into from
Nov 6, 2024

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Oct 30, 2024

The main goal

Although zbus 5 5 was released a few days ago, Agama is still using zbus 3. The goal of this PR is to adapt the code to use the latest version.

However, this task is not trivial: version 4 already introduced a good share of breaking changes in zbus API.

downcast_ref returns Result

The downcast_ref function now returns a Result instead of an Option with, IMHO, it is the right thing to do. This change has a big impact in our code to interact with NetworkManager, so I took the opportunity to:

  • Distinguish between a problem and a missing value (we always returned None when something went wrong).
  • Use get_property and get_optional_property to simplify our code a bit.

Other changes

  • Do some refactoring and organization of D-Bus proxies.
  • Improve the network::nm::dbus module. Let's use get_property and get_optional_property for better readability and error handling. Please, check this commit (WIP) if you are interested in further improvements.
  • Drop some unused code.

Tasks

  • Testing, testing and testing.
  • Update some outdated D-Bus definitions.
  • Improve the network::nm::dbus module.

@coveralls
Copy link

coveralls commented Oct 30, 2024

Pull Request Test Coverage Report for Build 11700249506

Details

  • 185 of 324 (57.1%) changed or added relevant lines in 23 files are covered.
  • 13 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.007%) to 71.486%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rust/agama-lib/src/questions/http_client.rs 0 1 0.0%
rust/agama-lib/src/scripts/model.rs 0 1 0.0%
rust/agama-lib/src/storage/model.rs 0 1 0.0%
rust/agama-server/src/storage/web/iscsi.rs 0 1 0.0%
rust/agama-server/src/web/common.rs 0 1 0.0%
rust/agama-lib/src/progress.rs 0 2 0.0%
rust/agama-server/src/storage/web/zfcp/stream.rs 0 2 0.0%
rust/agama-cli/src/questions.rs 0 3 0.0%
rust/agama-lib/src/network/client.rs 0 3 0.0%
rust/agama-lib/src/product/client.rs 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
rust/agama-lib/src/storage/client/iscsi.rs 1 0.0%
rust/agama-server/src/network/nm/builder.rs 1 0.0%
rust/agama-server/src/users/web.rs 1 0.0%
rust/agama-lib/src/storage/client/zfcp.rs 2 0.0%
rust/agama-server/src/network/nm/dbus.rs 3 76.16%
rust/agama-server/tests/common/mod.rs 5 74.19%
Totals Coverage Status
Change from base Build 11679986183: 0.007%
Covered Lines: 16890
Relevant Lines: 23627

💛 - Coveralls

@imobachgs imobachgs marked this pull request as ready for review November 5, 2024 13:03
Copy link
Contributor

@teclator teclator left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jcronenberg jcronenberg left a comment

Choose a reason for hiding this comment

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

I mostly looked at the NM dbus code, because that's what I know, but it looks like a nice improvement all around 🙂

rust/agama-server/src/network/nm/client.rs Outdated Show resolved Hide resolved
rust/agama-server/src/network/nm/dbus.rs Outdated Show resolved Hide resolved
rust/agama-server/src/network/nm/dbus.rs Outdated Show resolved Hide resolved
Ok(self.calculator_proxy.calculate(settings.into()).await?)
let map: HashMap<&str, zbus::zvariant::Value> = settings.into();
let options: HashMap<&str, &zbus::zvariant::Value> =
map.iter().map(|(id, value)| (*id, value)).collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

I have feeling that this be written better, but sadly do not have idea how....so lets keep it so far

Copy link
Contributor

@jreidinger jreidinger left a comment

Choose a reason for hiding this comment

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

nothing critical

@imobachgs imobachgs merged commit a71e9e0 into master Nov 6, 2024
7 checks passed
@imobachgs imobachgs deleted the zbus-5 branch November 6, 2024 11:35
@@ -1293,24 +1216,30 @@ mod test {
])];

let ipv4_section = HashMap::from([
("method".to_string(), Value::new("auto").to_owned()),
("method".to_string(), Value::new("auto").try_to_owned()?),
Copy link
Contributor

Choose a reason for hiding this comment

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

@imobachgs I know this is just test code, but it bothered me how the meaningful text was lost among the boilerplate, so I made a helper for readability:

diff --git a/rust/agama-server/src/network/nm/dbus.rs b/rust/agama-server/src/network/nm/dbus.rs
index 3e4ab1bf6..99e69ad2e 100644
--- a/rust/agama-server/src/network/nm/dbus.rs
+++ b/rust/agama-server/src/network/nm/dbus.rs
@@ -1195,6 +1195,14 @@ mod test {
     use uuid::Uuid;
     use zbus::zvariant::{self, Array, Dict, OwnedValue, Value};
 
+    // hash item
+    fn hi<'a, T>(key: &str, value: T) -> anyhow::Result<(String, OwnedValue)>
+    where
+        T: Into<Value<'a>> + zbus::zvariant::Type,
+    {
+        Ok((key.to_string(), Value::new(value).try_to_owned()?))
+    }
+
     #[test]
     fn test_connection_from_dbus() -> anyhow::Result<()> {
         let uuid = Uuid::new_v4().to_string();
@@ -1216,31 +1231,13 @@ mod test {
         ])];
 
         let ipv4_section = HashMap::from([
-            ("method".to_string(), Value::new("auto").try_to_owned()?),
-            (
-                "address-data".to_string(),
-                Value::new(address_v4_data).try_to_owned()?,
-            ),
-            (
-                "gateway".to_string(),
-                Value::new("192.168.0.1").try_to_owned()?,
-            ),
-            (
-                "dns-data".to_string(),
-                Value::new(vec!["192.168.0.2"]).try_to_owned()?,
-            ),
-            (
-                "dns-search".to_string(),
-                Value::new(vec!["suse.com", "example.com"]).try_to_owned()?,
-            ),
-            (
-                "ignore-auto-dns".to_string(),
-                Value::new(true).try_to_owned()?,
-            ),
-            (
-                "route-data".to_string(),
-                Value::new(route_v4_data).try_to_owned()?,
-            ),
+            hi("method", "auto")?,
+            hi("address-data", address_v4_data)?,
+            hi("gateway", "192.168.0.1")?,
+            hi("dns-data", vec!["192.168.0.2"])?,
+            hi("dns-search", vec!["suse.com", "example.com"])?,
+            hi("ignore-auto-dns", true)?,
+            hi("route-data", route_v4_data)?,
         ]);
 
         let address_v6_data = vec![HashMap::from([

mvidner added a commit that referenced this pull request Nov 7, 2024
## Problem

As I was reviewing #1716 I noticed it was touching some repetitive code
to set up test data

## Solution

Don't bury the actual test data among so much boilerplate.
Diff: +85 -236


## Testing

`cargo test` continues to work ✔️ 

## Screenshots

N/A
@imobachgs imobachgs mentioned this pull request Jan 10, 2025
imobachgs added a commit that referenced this pull request Jan 13, 2025
Update to release version 11.

* #1495
* #1564
* #1617
* #1618
* #1625
* #1626
* #1627
* #1628
* #1630
* #1631
* #1632
* #1633
* #1634
* #1635
* #1636
* #1639
* #1640
* #1641
* #1642
* #1643
* #1644
* #1645
* #1646
* #1647
* #1648
* #1649
* #1650
* #1651
* #1652
* #1654
* #1655
* #1656
* #1657
* #1660
* #1663
* #1666
* #1667
* #1668
* #1670
* #1671
* #1673
* #1674
* #1675
* #1676
* #1677
* #1681
* #1682
* #1683
* #1684
* #1687
* #1688
* #1689
* #1690
* #1691
* #1692
* #1693
* #1694
* #1695
* #1696
* #1698
* #1699
* #1702
* #1703
* #1704
* #1705
* #1707
* #1708
* #1709
* #1710
* #1711
* #1712
* #1713
* #1714
* #1715
* #1716
* #1717
* #1718
* #1720
* #1721
* #1722
* #1723
* #1727
* #1728
* #1729
* #1731
* #1732
* #1733
* #1734
* #1735
* #1736
* #1737
* #1740
* #1741
* #1743
* #1744
* #1745
* #1746
* #1751
* #1753
* #1754
* #1755
* #1757
* #1762
* #1763
* #1764
* #1765
* #1766
* #1767
* #1769
* #1771
* #1772
* #1773
* #1774
* #1777
* #1778
* #1785
* #1786
* #1787
* #1788
* #1789
* #1790
* #1791
* #1792
* #1793
* #1794
* #1795
* #1796
* #1797
* #1798
* #1799
* #1800
* #1802
* #1803
* #1804
* #1805
* #1807
* #1808
* #1809
* #1810
* #1811
* #1812
* #1814
* #1815
* #1821
* #1822
* #1823
* #1824
* #1825
* #1826
* #1827
* #1828
* #1830
* #1831
* #1832
* #1833
* #1834
* #1835
* #1836
* #1837
* #1838
* #1839
* #1840
* #1841
* #1842
* #1843
* #1844
* #1845
* #1847
* #1848
* #1849
* #1850
* #1851
* #1854
* #1855
* #1856
* #1857
* #1860
* #1861
* #1863
* #1864
* #1865
* #1866
* #1867
* #1871
* #1872
* #1873
* #1875
* #1876
* #1877
* #1878
* #1880
* #1881
* #1882
* #1883
* #1884
* #1885
* #1886
* #1888
* #1889
* #1890
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.

6 participants