[#432] Implement CLI iox2-config #468

brosier01 · 2024-10-14T21:53:07Z

Notes for Reviewer

Create a new CLI for iceoryx2 : iox2-config
iox2-config can show the configuration currently in use and generate a new configuration file at the default location iceoryx2 is looking for.
Remove the print_system_configuration() function in iceoryx2-bb/posix/src/system_configuration.rs file and move it into the CLI iox2-config

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
~~Unit tests have been written for new behavior~~
Public API is documented
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Relates to #432

elfenpiff · 2024-10-14T22:35:17Z

@brosier01 don't close it - it looked good!

orecham

In general looks pretty good, just one real blocker re: config location and also you'll need to address the static analysis CI findings.

Thanks for getting involved and welcome!

doc/release-notes/iceoryx2-unreleased.md

iceoryx2-cli/iox2-config/src/commands.rs

orecham

I tested the CLI on the different platforms. A few more things to resolve before it can be merged.

iceoryx2-cli/iox2-config/src/commands.rs

orecham

The generated configs are now being used and there is a confirmation message for overwriting a pre-existing generated config - nice.

Some more things left to address, see unresolved threads.

iceoryx2/src/config.rs

iceoryx2-cli/iox2-config/src/commands.rs

iceoryx2/src/config.rs

brosier01 · 2024-11-19T21:13:34Z

Hi @orecham @elfenpiff, is there any way to run CI locally?

elfenpiff · 2024-11-20T06:27:27Z

@brosier01 Yes there is. You can use the ./internal/scripts/iceoryx2_env.sh to enter the right docker container, depending on what target fails. The syntax is ./internal/scripts/iceoryx2_env.sh enter ubuntu:22.04 to start the ubuntu docker container for instance.

When you entered it you can run the usual cargo commands. Under .github/workflows/build-test.yml you find the test definitions. It is a bit hands on but we did not yet have time to add individual build & test scripts to ./internal/scripts/.

brosier01 · 2024-11-26T18:57:36Z

Hi @orecham, I think I've implemented all the elements described in the original problem. Do you see any other features I need to work on ?

orecham · 2024-11-27T12:36:22Z

Hi @orecham, I think I've implemented all the elements described in the original problem. Do you see any other features I need to work on ?

@brosier01 I will be able to review sometime today or tomorrow, apologies for the delay.

orecham

Sorry for taking so long to get back to you on this - had too much on my plate the last few weeks. I should be more responsive from now though.

PR is almost done! Can be merged after:

Rebasing on main
Addressing the remaining open conversations (I have resolved all not relevant for this PR)

iceoryx2/src/config.rs

iceoryx2-cal/src/config_path/mod.rs

iceoryx2-cli/iox2-config/src/commands.rs

doc/release-notes/iceoryx2-unreleased.md

orecham · 2024-12-22T01:21:28Z

@brosier01 Back to you.

orecham · 2024-12-23T01:03:59Z

@brosier01

Some conflicts need resolving:

Also, these duplicate commits should be squashed into single commits.

Co-authored-by: orecham <[email protected]>

…file

…rm coloring + implementation of user confirmation required before deleting current configuration file

codecov · 2024-12-24T23:43:55Z

Codecov Report

Attention: Patch coverage is 66.66667% with 9 lines in your changes missing coverage. Please review.

Project coverage is 79.49%. Comparing base (f4931fd) to head (836b7c4).

Files with missing lines	Patch %	Lines
iceoryx2/src/config.rs	50.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
+ Coverage   79.25%   79.49%   +0.23%     
==========================================
  Files         203      205       +2     
  Lines       25230    25179      -51     
==========================================
+ Hits        19995    20015      +20     
+ Misses       5235     5164      -71

Files with missing lines	Coverage Δ
iceoryx2-bb/posix/src/system_configuration.rs	`11.34% <ø> (+4.59%)`	⬆️
iceoryx2-cal/src/config_path/dirs.rs	`100.00% <100.00%> (ø)`
iceoryx2-cal/src/config_path/mod.rs	`100.00% <100.00%> (ø)`
iceoryx2/src/config.rs	`77.33% <50.00%> (-0.45%)`	⬇️

... and 3 files with indirect coverage changes

elfenpiff

I need to review the last 3 files but I left some comments. The review of the next 3 files will follow soon.

elfenpiff · 2024-12-25T02:06:20Z

doc/release-notes/iceoryx2-unreleased.md

-    NOTE: Add new entries sorted by issue number to minimize the possibility of
-    conflicts when merging.
-->
+Create a new CLI for iceoryx2 `iox2-config`


Please summarize everything into one sentence and put everything in one bullet point as you did in line 15. If someone is interested in the details, they can follow the provided issue link.

elfenpiff · 2024-12-25T02:07:12Z

doc/release-notes/iceoryx2-unreleased.md

-->
-
-* Example text [#1](https://github.com/eclipse-iceoryx/iceoryx2/issues/1)
+Remove the `print_system_configuration()` function in


Please put everything into one bullet point.

elfenpiff · 2024-12-25T02:08:08Z

doc/release-notes/iceoryx2-unreleased.md


-1. Example
+iox2 config show system


Can you put every command into an

```sh ```

environment and explain with one sentence what it does.

elfenpiff · 2024-12-25T02:17:07Z

iceoryx2-cal/Cargo.toml

@@ -26,6 +26,7 @@ iceoryx2-bb-memory = { workspace = true }
 iceoryx2-bb-lock-free = { workspace = true }
 iceoryx2-pal-concurrency-sync = { workspace = true }

+dirs = { workspace = true }


Please remove this and the config_path component from iceoryx2_cal. This is not a iceoryx2 concept. The mistake is on our side since we did not yet had the time to document the iceoryx2 concept pattern in detail, what it is, what problem it solves and how it shall be implemented.

A concept is an abstract mechanism:

That has multiple implementations.

Can be constructed and used via the traits of the concepts

Comes with a generic test suite to verify the same behavior of all concept implementations.

One example is the communication channel.

it has a receiver and a sender

it comes with a builder trait

the sender sends data, the receiver receives data

it can be implemented by a UNIX domain socket, message queue, named pipe or a shared memory queue

it comes with a test suite that verifies that the sent data can be retrieved with the corresponding receiver.

I agree that you require a way of acquiring the config directory and your approach seems valid but this should be part of the iceoryx2/src/config.rs

elfenpiff · 2024-12-25T02:17:20Z

iceoryx2-cal/src/config_path/dirs.rs

@@ -0,0 +1,29 @@
+// Copyright (c) 2024 Contributors to the Eclipse Foundation


Please remove this

elfenpiff · 2024-12-25T02:17:25Z

iceoryx2-cal/src/config_path/mod.rs

@@ -0,0 +1,22 @@
+// Copyright (c) 2024 Contributors to the Eclipse Foundation


Please remove this

… subcommand to display the contents of the configuration file + added configuration file standard location

elfenpiff · 2025-01-08T18:17:11Z

iceoryx2/src/config.rs

-                Err(ConfigCreationError::FailedToReadConfigFileContents) => {
-                    warn!(from "Config::global_config()", "Default config file found but unable to read content, populate config with default values.");
-                    ICEORYX2_CONFIG.set_value(Config::default());
+            match config_dir() {


I took a deep dive into the dirs crate and the config_dir function and I realized that it depends on dirs-rs which requires a $HOME env variable that is available on linux and some unix systems - not Windows. But we must have a solution that is extendable to other platforms and works out of the box on Linux, Windows, Mac OS and FreeBSD.

So the best solution would be to add this functionality to the iceoryx2_pal layer. I will take care of this in the next days and make it available to you.
We have to be careful with introducing new dependencies to iceoryx2. They must work on all our target platforms and we also need to have a strategy on how to handle software certification later.

But please do not let this discourage you! You did a great job with the CLI so far and the crate-dependency thing was also nowhere documented (so its on me). I'll try to add this to the platform and then we can merge this hopefully latest next week.

brosier01 closed this Oct 14, 2024

brosier01 deleted the iox2-432-CLI-displaying-full-system-configuration branch October 14, 2024 22:27

brosier01 restored the iox2-432-CLI-displaying-full-system-configuration branch October 14, 2024 22:28

brosier01 reopened this Oct 14, 2024

orecham reviewed Oct 14, 2024

View reviewed changes

orecham assigned brosier01 Oct 14, 2024

orecham reviewed Oct 14, 2024

View reviewed changes

iceoryx2-cli/iox2-config/src/commands.rs Show resolved Hide resolved

orecham changed the title ~~[eclipse-iceoryx#432] Implement CLI iox2-config~~ [#432] Implement CLI iox2-config Oct 15, 2024

brosier01 force-pushed the iox2-432-CLI-displaying-full-system-configuration branch from ff8654f to 973c574 Compare October 15, 2024 22:28

orecham reviewed Oct 16, 2024

View reviewed changes

brosier01 requested a review from orecham October 23, 2024 17:26

orecham reviewed Oct 24, 2024

View reviewed changes

brosier01 requested review from orecham and elfenpiff November 11, 2024 19:00

brosier01 requested a review from elBoberido November 19, 2024 20:40

brosier01 force-pushed the iox2-432-CLI-displaying-full-system-configuration branch from e7798a3 to 037781e Compare November 19, 2024 21:07

brosier01 force-pushed the iox2-432-CLI-displaying-full-system-configuration branch from 037781e to 82f5387 Compare November 19, 2024 21:27

brosier01 force-pushed the iox2-432-CLI-displaying-full-system-configuration branch 2 times, most recently from 08a4135 to 41d20b8 Compare November 26, 2024 18:48

orecham reviewed Dec 22, 2024

View reviewed changes

orecham mentioned this pull request Dec 22, 2024

CLI output of global config on PowerShell is not formatted properly #562

Open

brosier01 force-pushed the iox2-432-CLI-displaying-full-system-configuration branch from 41d20b8 to bd3e3d2 Compare December 22, 2024 22:19

brosier01 requested a review from orecham December 22, 2024 22:19

brosier01 and others added 4 commits December 24, 2024 22:43

[eclipse-iceoryx#432] Implement CLI iox2-config

45085e8

[eclipse-iceoryx#432] Update doc/release-notes/iceoryx2-unreleased.md

98759bc

Co-authored-by: orecham <[email protected]>

[eclipse-iceoryx#432] Set default location for default configuration …

5ada5fc

…file

[eclipse-iceoryx#432] Fix print_system_configuration for cross-platfo…

2d59ec1

…rm coloring + implementation of user confirmation required before deleting current configuration file

brosier01 force-pushed the iox2-432-CLI-displaying-full-system-configuration branch 4 times, most recently from d28c6fb to 613a2e0 Compare December 24, 2024 23:21

brosier01 force-pushed the iox2-432-CLI-displaying-full-system-configuration branch from 613a2e0 to 836b7c4 Compare December 24, 2024 23:49

elfenpiff reviewed Dec 25, 2024

View reviewed changes

brosier01 force-pushed the iox2-432-CLI-displaying-full-system-configuration branch from 836b7c4 to 92982fb Compare December 26, 2024 07:13

[eclipse-iceoryx#432] Added error handling for config_dir() + added a…

c66b7d9

… subcommand to display the contents of the configuration file + added configuration file standard location

brosier01 force-pushed the iox2-432-CLI-displaying-full-system-configuration branch from 92982fb to c66b7d9 Compare December 26, 2024 07:31

brosier01 requested a review from elfenpiff December 31, 2024 14:20

elfenpiff reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#432] Implement CLI iox2-config #468

[#432] Implement CLI iox2-config #468

brosier01 commented Oct 14, 2024 •

edited

Loading

elfenpiff commented Oct 14, 2024

orecham left a comment •

edited

Loading

orecham left a comment

orecham left a comment

brosier01 commented Nov 19, 2024

elfenpiff commented Nov 20, 2024

brosier01 commented Nov 26, 2024

orecham commented Nov 27, 2024

orecham left a comment

orecham commented Dec 22, 2024

orecham commented Dec 23, 2024

codecov bot commented Dec 24, 2024 •

edited

Loading

elfenpiff left a comment

elfenpiff Dec 25, 2024

elfenpiff Dec 25, 2024

elfenpiff Dec 25, 2024

elfenpiff Dec 25, 2024

elfenpiff Dec 25, 2024

elfenpiff Dec 25, 2024

elfenpiff Jan 8, 2025

		@@ -0,0 +1,29 @@
		// Copyright (c) 2024 Contributors to the Eclipse Foundation

		@@ -0,0 +1,22 @@
		// Copyright (c) 2024 Contributors to the Eclipse Foundation

[#432] Implement CLI iox2-config #468

Are you sure you want to change the base?

[#432] Implement CLI iox2-config #468

Conversation

brosier01 commented Oct 14, 2024 • edited Loading

Notes for Reviewer

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

elfenpiff commented Oct 14, 2024

orecham left a comment • edited Loading

Choose a reason for hiding this comment

orecham left a comment

Choose a reason for hiding this comment

orecham left a comment

Choose a reason for hiding this comment

brosier01 commented Nov 19, 2024

elfenpiff commented Nov 20, 2024

brosier01 commented Nov 26, 2024

orecham commented Nov 27, 2024

orecham left a comment

Choose a reason for hiding this comment

orecham commented Dec 22, 2024

orecham commented Dec 23, 2024

codecov bot commented Dec 24, 2024 • edited Loading

Codecov Report

elfenpiff left a comment

Choose a reason for hiding this comment

elfenpiff Dec 25, 2024

Choose a reason for hiding this comment

elfenpiff Dec 25, 2024

Choose a reason for hiding this comment

elfenpiff Dec 25, 2024

Choose a reason for hiding this comment

elfenpiff Dec 25, 2024

Choose a reason for hiding this comment

elfenpiff Dec 25, 2024

Choose a reason for hiding this comment

elfenpiff Dec 25, 2024

Choose a reason for hiding this comment

elfenpiff Jan 8, 2025

Choose a reason for hiding this comment

brosier01 commented Oct 14, 2024 •

edited

Loading

orecham left a comment •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading