Merge `standalone-binaries` to the main CLI #259

Hennzau · 2024-11-23T11:54:51Z

Merge `standalon-binaries` inside `zfctl` CLI

This PR introduces improvements to the zfctl CLI for better usability.

Daemon

The zenoh-flow-standalone-daemon crate has been removed and replaced by the CLI command:

zfctl daemon start <NAME | CONFIGURATION>

Runtime

The zenoh-flow-standalone-runtime crate has been removed and replaced by the CLI command:

zfctl run-local <dataflow.yml> <optional configuration>

J-Loudet · 2024-11-27T13:16:10Z

rust-toolchain.toml

@@ -1,2 +1,2 @@
 [toolchain]
-channel = "1.72.1"
+channel = "1.75"


A small comment on this: the version of the Rust toolchain is linked to the version of Zenoh. The reason being that Zenoh-Flow can be compiled to a Zenoh plugin and, in order for Zenoh to load the plugin, it needs to have been compiled with the same version of Rust.

Hence, if you want to tackle this part, I'd suggest to make a separate PR that does both: bump Zenoh to version 1.0.x and update the toolchain to 1.75 😉

(I did not look closely at the rest of your PR as you marked it as a draft)

Oh thx! Yeah bumping Zenoh was also in my mind, I should definitely separate it in two PR.

J-Loudet · 2024-11-27T13:44:19Z

@Hennzau In order to accept your contribution, you need to create an account -- with the same e-mail address associated to your GitHub account (this is how Eclipse validates the contributions) -- on the Eclipse Foundation website and then sign the ECA. 😉
This page gives you all the information: https://www.eclipse.org/legal/eca/

J-Loudet · 2024-11-29T10:06:10Z

Thanks for the contribution @Hennzau!

I am currently traveling so I will be able to look at it thoroughly starting next week. There is however one aspect that caught my attention, you say:

Before this PR: Executing zfctl runtime list did not return any output.

Now: The command communicates with all active daemons and presents the running runtimes in a clear, organized format.

That is an odd behavior that I am unable to reproduce. I tried on my machine and that of a colleague and we both obtained the expected result.
Given we both use macOS, I'm wondering if it could be an OS specific issue. Could you explain what you tried and if you can reproduce it consistently?

Hennzau · 2024-11-29T10:12:06Z

Thanks for the contribution @Hennzau!

I am currently traveling so I will be able to look at it thoroughly starting next week. There is however one aspect that caught my attention, you say:

Before this PR: Executing zfctl runtime list did not return any output.

Now: The command communicates with all active daemons and presents the running runtimes in a clear, organized format.

That is an odd behavior that I am unable to reproduce. I tried on my machine and that of a colleague and we both obtained the expected result. Given we both use macOS, I'm wondering if it could be an OS specific issue. Could you explain what you tried and if you can reproduce it consistently?

Oh wait... I just realized I got the command wrong! I was actually referring to the zfctl instance list command. I will update my message

Hennzau · 2024-11-29T10:14:27Z

zfctl/src/instance_command.rs

+            InstancesQuery::List => {
+                let mut table = Table::new();
+                table.set_width(80);
+                table.set_header(row!("Instance Name", "Instance ID", "Instance State"));
+
+                while let Ok(response) = reply.recv_async().await {
+                    match response.sample {
+                        Ok(sample) => {
+                            match serde_json::from_slice::<
+                                HashMap<InstanceId, (Arc<str>, InstanceState)>,
+                            >(&sample.value.payload.contiguous())
+                            {
+                                Ok(list) => {
+                                    for (id, (name, state)) in list {
+                                        table.add_row(row!(name, id, state));
+                                    }
+                                }
+                                Err(e) => tracing::error!(
+                                    "Failed to parse 'list' reply from < {} >: {:?}",
+                                    response.replier_id,
+                                    e
+                                ),
+                            }
+                        }
+                        Err(err) => tracing::error!("{:?}", err),
+                    }
+                }
+
+                println!("{table}");
+            }


This is the code I added

J-Loudet

Again, thanks for your contribution @Hennzau 🙂

Your PR is, in this state, doing multiple things at the same time and it is not making it easy to follow (it's not unreadable at all, simply not as easy as it could be).

Hence, I would like you to reduce the scope to what the title of your PR says: simply merging the standalone binaries in zfctl.

I am not against changing the name and functionality of some of the commands but it would be better if it was done in a separate PR: that way we can discuss the changes.

For instance, I don't think the run in zfctl daemon run is the best verb. Something like spawn or start is more commonly used in similar contexts — e.g. you do systemctl start <service> to run a background daemon/service.
Similarly, maybe we could use zfctl run-local <dataflow.yaml> for the standalone runtime and zfctl instance deploy <dataflow.yaml> + zfctl instance start <uuid> for the distributed version.

The addition of the instance list is indeed welcomed!

Other than that I have no comment on the code itself. 😉

J-Loudet · 2024-12-04T11:39:08Z

examples/examples/topic-sender/src/main.rs

We already have an example that does exactly this in the Zenoh repository (here). Given that you are inclined to add this, maybe more specific instructions are required in the README?

Hennzau · 2024-12-04T12:19:43Z

Hi! Thank you very much, I understood everything! I’ll clarify all of this.

Hennzau · 2024-12-04T16:20:45Z

It should be good now! Thx for the previous review :)

J-Loudet

Sorry for the delay.

After taking some time to play with this new version of zfctl — having less command-line tools provides indeed a better experience — I don't think it makes sense to have zfctl runtime + zfctl daemon and I would suggest to fuse them in the daemon sub-command.

I understand this goes against my previous feedback of "just merging", so that one is on me. 😉
I can take care of it if you want.

Hennzau · 2024-12-09T16:20:49Z

Great! I can take care of it, but I won't have time until December 20th. So if you want this fusion done before that, you can do it; otherwise, I'll handle it :)

J-Loudet · 2024-12-09T17:04:39Z

I have merged it as is and I will do the changes later on. ;-)

J-Loudet reviewed Nov 27, 2024

View reviewed changes

Hennzau changed the title ~~[WIP] Merge standalone binaries to the main CLI~~ Merge standalone binaries to the main CLI Nov 28, 2024

Hennzau marked this pull request as ready for review November 28, 2024 17:32

Hennzau force-pushed the bump_and_cli branch from c1b4e43 to 29ca3ef Compare November 28, 2024 20:11

Hennzau commented Nov 29, 2024

View reviewed changes

J-Loudet requested changes Dec 4, 2024

View reviewed changes

J-Loudet reviewed Dec 4, 2024

View reviewed changes

Hennzau marked this pull request as draft December 4, 2024 12:44

Hennzau force-pushed the bump_and_cli branch from 29ca3ef to 959f8a0 Compare December 4, 2024 16:11

Hennzau changed the title ~~Merge standalone binaries to the main CLI~~ Merge standalone-binaries to the main CLI Dec 4, 2024

merge standalone binaries inside CLI

cbdad88

Hennzau force-pushed the bump_and_cli branch from 959f8a0 to cbdad88 Compare December 4, 2024 16:16

Hennzau marked this pull request as ready for review December 4, 2024 16:19

J-Loudet requested changes Dec 9, 2024

View reviewed changes

J-Loudet merged commit 23bae3b into eclipse-zenoh-flow:main Dec 9, 2024
5 checks passed

Hennzau deleted the bump_and_cli branch December 9, 2024 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge `standalone-binaries` to the main CLI #259

Merge `standalone-binaries` to the main CLI #259

Hennzau commented Nov 23, 2024 •

edited

Loading

J-Loudet Nov 27, 2024

Hennzau Nov 27, 2024

J-Loudet commented Nov 27, 2024 •

edited

Loading

J-Loudet commented Nov 29, 2024

Hennzau commented Nov 29, 2024

Hennzau Nov 29, 2024

J-Loudet left a comment

J-Loudet Dec 4, 2024

Hennzau commented Dec 4, 2024

Hennzau commented Dec 4, 2024

J-Loudet left a comment •

edited

Loading

Hennzau commented Dec 9, 2024

J-Loudet commented Dec 9, 2024

Merge standalone-binaries to the main CLI #259

Merge standalone-binaries to the main CLI #259

Conversation

Hennzau commented Nov 23, 2024 • edited Loading

Merge standalon-binaries inside zfctl CLI

Daemon

Runtime

J-Loudet Nov 27, 2024

Choose a reason for hiding this comment

Hennzau Nov 27, 2024

Choose a reason for hiding this comment

J-Loudet commented Nov 27, 2024 • edited Loading

J-Loudet commented Nov 29, 2024

Hennzau commented Nov 29, 2024

Hennzau Nov 29, 2024

Choose a reason for hiding this comment

J-Loudet left a comment

Choose a reason for hiding this comment

J-Loudet Dec 4, 2024

Choose a reason for hiding this comment

Hennzau commented Dec 4, 2024

Hennzau commented Dec 4, 2024

J-Loudet left a comment • edited Loading

Choose a reason for hiding this comment

Hennzau commented Dec 9, 2024

J-Loudet commented Dec 9, 2024

Merge `standalone-binaries` to the main CLI #259

Merge `standalone-binaries` to the main CLI #259

Hennzau commented Nov 23, 2024 •

edited

Loading

Merge `standalon-binaries` inside `zfctl` CLI

J-Loudet commented Nov 27, 2024 •

edited

Loading

J-Loudet left a comment •

edited

Loading