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

Update to zbus 4 #1025

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update to zbus 4 #1025

wants to merge 1 commit into from

Conversation

nc7s
Copy link

@nc7s nc7s commented Jul 12, 2024

Mainly adapting to API changes.

Especially note the error messages, I'm certainly not knowledgeable enough to properly write them, plus there's one with TODO.

There is an apparent repeating pattern (below), which I feel suboptimal, but can't yet find a better alternative. Please enlighten me if you can.

if let Value::Variant(val) = raw {
    Some(val)
} else {
    None
}
.and_then(|val| val.try_clone().ok())
.ok_or_else(|| NetavarkError::msg("..."))?

Copy link
Contributor

openshift-ci bot commented Jul 12, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nc7s
Once this PR has been reviewed and has the lgtm label, please assign vrothberg for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nc7s nc7s changed the title fix(deps): update zbus to 4.3.1 Update zbus to 4.3.1 Jul 12, 2024
@nc7s nc7s changed the title Update zbus to 4.3.1 Update to zbus 4 Jul 12, 2024
@Luap99
Copy link
Member

Luap99 commented Jul 12, 2024

Note you have validate failures (make validate) that have to be fixed.

@mheon PTAL

@mheon
Copy link
Member

mheon commented Jul 12, 2024

I'm still thinking we replace zbus entirely with a simpler library at some point, but I have no objection to merging this in the interim.

@mheon
Copy link
Member

mheon commented Jul 12, 2024

Changes look fine once you can get CI to pass.

src/firewall/firewalld.rs Outdated Show resolved Hide resolved
@nc7s
Copy link
Author

nc7s commented Jul 12, 2024

centos9_build and ubuntu20_build failed because the zbus & zvariant 4 series require rustc 1.75; if you don't want to lift build requirements that's a hard no I guess.

@Luap99
Copy link
Member

Luap99 commented Jul 12, 2024

centos9_build and ubuntu20_build failed because the zbus & zvariant 4 series require rustc 1.75; if you don't want to lift build requirements that's a hard no I guess.

That should not a problem, they are ancient and mostly useless anyway as nobody takes care of these build images
ref: #775

We need to merge #1015 and then I guess we should just drop ubuntu job as well

@Luap99
Copy link
Member

Luap99 commented Jul 12, 2024

Also

diff --git a/src/firewall/firewalld.rs b/src/firewall/firewalld.rs
index ab2dcaf..501eb4f 100644
--- a/src/firewall/firewalld.rs
+++ b/src/firewall/firewalld.rs
@@ -129,7 +129,7 @@ impl firewall::FirewallDriver for FirewallD {
             &(PORTPOLICYNAME),
         )?;
         let body = policy_config_msg.body();
-        let policy_config: HashMap<&str, Value> = body.deserialize().map_err(|e| {
+        let mut policy_config: HashMap<&str, Value> = body.deserialize().map_err(|e| {
             NetavarkError::wrap(
                 format!("Error decoding DBus message for policy {PORTPOLICYNAME} configuration"),
                 e.into(),
@@ -137,17 +137,15 @@ impl firewall::FirewallDriver for FirewallD {
         })?;
 
         let mut port_forwarding_rules: Array;
-        match policy_config.get("forward_ports") {
+        match policy_config.remove("forward_ports") {
             Some(a) => {
                 port_forwarding_rules = if let Value::Array(arr) = a {
-                    Some(arr)
+                    arr
                 } else {
-                    None
-                }
-                .and_then(|arr| arr.try_clone().ok())
-                .ok_or_else(|| {
-                    NetavarkError::msg("forward-port in firewalld policy object has a bad type")
-                })?
+                    return Err(NetavarkError::msg(
+                        "forward-port in firewalld policy object has a bad type",
+                    ));
+                };
             }
             None => {
                 // No existing rules

You can do the same for the other policy_config.get() call

I am sure there are more tricks to avoid some more clones.

@nc7s
Copy link
Author

nc7s commented Jul 12, 2024

Updated with @Luap99's suggestions; I think I'll leave further refactoring out of this PR ;)

@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

baude added a commit to baude/netavark that referenced this pull request Oct 17, 2024
This PR was basically done in containers#1025 by a contributor Blair Noctis.  His
PR got stuck in a rebase problem and I have simply taken his diff and
worked out the rebasing issue.  Thanks so much to Blair for his work.

Signed-off-by: Brent Baude <[email protected]>
@baude baude mentioned this pull request Oct 17, 2024
baude added a commit to baude/netavark that referenced this pull request Oct 17, 2024
This PR was basically done in containers#1025 by a contributor Blair Noctis.  His
PR got stuck in a rebase problem and I have simply taken his diff and
worked out the rebasing issue.  Thanks so much to Blair for his work.

Signed-off-by: Blair Noctis <[email protected]>
Signed-off-by: Brent Baude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants