refactor: Create a module with SmartREST message id consts #3282

Bravo555 · 2024-12-04T22:16:27Z

Proposed changes

Create a single module that contains ids for Smartrest messages and static templates.

user code is easier to read
can easily jump to definition
easier to see the dependency and find usages by grepping for the usage of the module

The next steps would be for thin-edge not to use these ids directly, but to provide a higher-level layer of abstraction for interacting with Cumulocity, much like go-c8y-cli.

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

codecov · 2024-12-04T22:28:02Z

Codecov Report

Attention: Patch coverage is 89.61039% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/tedge/src/cli/connect/command.rs	0.00%	3 Missing ⚠️
...rates/extensions/c8y_firmware_manager/src/actor.rs	80.00%	2 Missing ⚠️
...re/c8y_api/src/smartrest/smartrest_deserializer.rs	66.66%	1 Missing ⚠️
crates/core/c8y_api/src/utils.rs	0.00%	1 Missing ⚠️
...ore/tedge/src/cli/connect/c8y_direct_connection.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2024-12-05T00:58:54Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
529	0	2	529	100	1h30m10.50289s

didier-wenzek

This is a very nice initiative and I would be really happy to see that merged. I would like to have opinions from the team about the code as a suffix being useful or not.

crates/core/c8y_api/src/smartrest/message_ids.rs

jarhodes314

I like the changes this PR makes, although I think the pattern matching is currently broken

crates/core/tedge/src/cli/connect/command.rs

crates/extensions/c8y_firmware_manager/src/actor.rs

didier-wenzek

Approved

albinsuresh

I see that you've inherited the existing names from the template titles in the C8y documentation. I've given some renaming suggestions, especially around the operation templates to make them sound like verbs rather than nouns. Feel free to accept/reject the ones that you feel relevant.

albinsuresh · 2024-12-10T14:32:11Z

crates/core/c8y_api/src/smartrest/message_ids.rs

@@ -0,0 +1,107 @@
+//! Definitions of Smartrest MQTT message ids.


Module name suggestion: static_templates.rs(https://cumulocity.com/docs/smartrest/mqtt-static-templates)

It was called that previously, but also wanted to incorporate ids like 41 and 71, which were used in the codebase but aren't really templates.

albinsuresh · 2024-12-10T14:43:25Z

crates/core/c8y_api/src/smartrest/message_ids.rs

+// Operation templates (5xx)
+
+pub const RESTART: usize = 510;
+pub const COMMAND: usize = 511;


Suggested change

pub const COMMAND: usize = 511;

pub const RUN_COMMAND: usize = 511;

albinsuresh · 2024-12-10T14:43:35Z

crates/core/c8y_api/src/smartrest/message_ids.rs

+
+pub const RESTART: usize = 510;
+pub const COMMAND: usize = 511;
+pub const CONFIGURATION: usize = 513;


Suggested change

pub const CONFIGURATION: usize = 513;

pub const SET_CONFIGURATION: usize = 513;

albinsuresh · 2024-12-10T14:43:58Z

crates/core/c8y_api/src/smartrest/message_ids.rs

+pub const RESTART: usize = 510;
+pub const COMMAND: usize = 511;
+pub const CONFIGURATION: usize = 513;
+pub const FIRMWARE: usize = 515;


Suggested change

pub const FIRMWARE: usize = 515;

pub const INSTALL_FIRMWARE: usize = 515;

albinsuresh · 2024-12-10T14:45:20Z

crates/core/c8y_api/src/smartrest/message_ids.rs

+pub const COMMAND: usize = 511;
+pub const CONFIGURATION: usize = 513;
+pub const FIRMWARE: usize = 515;
+pub const SOFTWARE_LIST: usize = 516;


Suggested change

pub const SOFTWARE_LIST: usize = 516;

pub const INSTALL_SOFTWARE_LIST: usize = 516;

albinsuresh · 2024-12-10T14:47:58Z

crates/core/c8y_api/src/smartrest/message_ids.rs

+pub const SOFTWARE_LIST: usize = 516;
+pub const MEASUREMENT_REQUEST_OPERATION: usize = 517;
+pub const RELAY: usize = 518;
+pub const RELAY_ARRAY: usize = 519;


Suggested change

pub const RELAY_ARRAY: usize = 519;

pub const SWITCH_RELAY_ARRAY: usize = 519;

albinsuresh · 2024-12-10T14:48:29Z

crates/core/c8y_api/src/smartrest/message_ids.rs

+pub const RELAY_ARRAY: usize = 519;
+pub const UPLOAD_CONFIGURATION_FILE: usize = 520;
+pub const DOWNLOAD_CONFIGURATION_FILE: usize = 521;
+pub const LOGFILE_REQUEST: usize = 522;


Suggested change

pub const LOGFILE_REQUEST: usize = 522;

pub const LOG_FILE_REQUEST: usize = 522;

albinsuresh · 2024-12-10T14:49:06Z

crates/core/c8y_api/src/smartrest/message_ids.rs

+pub const UPLOAD_CONFIGURATION_FILE: usize = 520;
+pub const DOWNLOAD_CONFIGURATION_FILE: usize = 521;
+pub const LOGFILE_REQUEST: usize = 522;
+pub const COMMUNICATION_MODE: usize = 523;


Suggested change

pub const COMMUNICATION_MODE: usize = 523;

pub const CHANGE_COMMUNICATION_MODE: usize = 523;

albinsuresh · 2024-12-10T14:49:47Z

crates/core/c8y_api/src/smartrest/message_ids.rs

+pub const LOGFILE_REQUEST: usize = 522;
+pub const COMMUNICATION_MODE: usize = 523;
+pub const DOWNLOAD_CONFIGURATION_FILE_WITH_TYPE: usize = 524;
+pub const FIRMWARE_FROM_PATCH: usize = 525;


Suggested change

pub const FIRMWARE_FROM_PATCH: usize = 525;

pub const INSTALL_FIRMWARE_FROM_PATCH: usize = 525;

albinsuresh · 2024-12-10T14:50:38Z

crates/core/c8y_api/src/smartrest/message_ids.rs

+pub const DOWNLOAD_CONFIGURATION_FILE_WITH_TYPE: usize = 524;
+pub const FIRMWARE_FROM_PATCH: usize = 525;
+pub const UPLOAD_CONFIGURATION_FILE_WITH_TYPE: usize = 526;
+pub const SET_DEVICE_PROFILES: usize = 527;


Suggested change

pub const SET_DEVICE_PROFILES: usize = 527;

pub const SET_DEVICE_PROFILE: usize = 527;

Usage of the magic numbers was replaced with using the consts where it could be done trivially, i.e. when these magic numbers were not parts of string literals where replacing them would add a lot of code, which mainly happens to be in the unit tests. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Bravo555 · 2024-12-10T15:12:22Z

I see that you've inherited the existing names from the template titles in the C8y documentation. I've given some renaming suggestions, especially around the operation templates to make them sound like verbs rather than nouns. Feel free to accept/reject the ones that you feel relevant.

I wanted to keep the names somewhat consistent with c8y documentation, and don't really want to spend more time revising the names, so I will merge them as they are now. We can always adjust them later.

didier-wenzek · 2024-12-10T16:02:31Z

I wanted to keep the names somewhat consistent with c8y documentation, and don't really want to spend more time revising the names, so I will merge them as they are now. We can always adjust them later.

Agree. Keeping the names consistent with c8y documentation is important even if some might be unclear.

Create a module with smartrest message ids consts

a761790

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Bravo555 added the refactoring Developer value label Dec 4, 2024

Bravo555 requested review from didier-wenzek, albinsuresh, jarhodes314 and rina23q as code owners December 4, 2024 22:16

Bravo555 had a problem deploying to Test Pull Request December 4, 2024 22:16 — with GitHub Actions Failure

Bravo555 force-pushed the improve/smartrest-types branch from 107963d to 82f9bae Compare December 4, 2024 23:34

Bravo555 temporarily deployed to Test Pull Request December 4, 2024 23:34 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto December 5, 2024 00:42 — with GitHub Actions Inactive

didier-wenzek reviewed Dec 5, 2024

View reviewed changes

crates/core/c8y_api/src/smartrest/message_ids.rs Show resolved Hide resolved

jarhodes314 requested changes Dec 5, 2024

View reviewed changes

crates/core/tedge/src/cli/connect/command.rs Outdated Show resolved Hide resolved

crates/core/tedge/src/cli/connect/command.rs Outdated Show resolved Hide resolved

crates/extensions/c8y_firmware_manager/src/actor.rs Outdated Show resolved Hide resolved

Bravo555 temporarily deployed to Test Pull Request December 10, 2024 13:28 — with GitHub Actions Inactive

Bravo555 requested a review from jarhodes314 December 10, 2024 13:28

Bravo555 temporarily deployed to Test Auto December 10, 2024 13:34 — with GitHub Actions Inactive

jarhodes314 approved these changes Dec 10, 2024

View reviewed changes

didier-wenzek approved these changes Dec 10, 2024

View reviewed changes

albinsuresh approved these changes Dec 10, 2024

View reviewed changes

Bravo555 force-pushed the improve/smartrest-types branch from bd580c2 to f0ff4f7 Compare December 10, 2024 15:10

Bravo555 temporarily deployed to Test Pull Request December 10, 2024 15:10 — with GitHub Actions Inactive

Bravo555 had a problem deploying to Test Auto December 10, 2024 15:16 — with GitHub Actions Failure

Bravo555 temporarily deployed to Test Auto December 10, 2024 16:39 — with GitHub Actions Inactive

Bravo555 added this pull request to the merge queue Dec 10, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 10, 2024

Bravo555 added this pull request to the merge queue Dec 11, 2024

Merged via the queue into thin-edge:main with commit 82cde40 Dec 11, 2024
33 checks passed

Bravo555 deleted the improve/smartrest-types branch December 12, 2024 08:49

reubenmiller changed the title ~~refactor: Create a module with smartrest message id consts~~ refactor: Create a module with SmartREST message id consts Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Create a module with SmartREST message id consts #3282

refactor: Create a module with SmartREST message id consts #3282

Bravo555 commented Dec 4, 2024

codecov bot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 5, 2024 •

edited

Loading

didier-wenzek left a comment

jarhodes314 left a comment

didier-wenzek left a comment

albinsuresh left a comment

albinsuresh Dec 10, 2024

Bravo555 Dec 10, 2024

albinsuresh Dec 10, 2024

albinsuresh Dec 10, 2024

albinsuresh Dec 10, 2024

albinsuresh Dec 10, 2024

albinsuresh Dec 10, 2024

albinsuresh Dec 10, 2024

albinsuresh Dec 10, 2024

albinsuresh Dec 10, 2024

albinsuresh Dec 10, 2024

Bravo555 commented Dec 10, 2024

didier-wenzek commented Dec 10, 2024

		@@ -0,0 +1,107 @@
		//! Definitions of Smartrest MQTT message ids.

	pub const COMMAND: usize = 511;
	pub const RUN_COMMAND: usize = 511;

	pub const CONFIGURATION: usize = 513;
	pub const SET_CONFIGURATION: usize = 513;

	pub const FIRMWARE: usize = 515;
	pub const INSTALL_FIRMWARE: usize = 515;

	pub const SOFTWARE_LIST: usize = 516;
	pub const INSTALL_SOFTWARE_LIST: usize = 516;

	pub const RELAY_ARRAY: usize = 519;
	pub const SWITCH_RELAY_ARRAY: usize = 519;

	pub const LOGFILE_REQUEST: usize = 522;
	pub const LOG_FILE_REQUEST: usize = 522;

	pub const COMMUNICATION_MODE: usize = 523;
	pub const CHANGE_COMMUNICATION_MODE: usize = 523;

	pub const FIRMWARE_FROM_PATCH: usize = 525;
	pub const INSTALL_FIRMWARE_FROM_PATCH: usize = 525;

	pub const SET_DEVICE_PROFILES: usize = 527;
	pub const SET_DEVICE_PROFILE: usize = 527;

refactor: Create a module with SmartREST message id consts #3282

refactor: Create a module with SmartREST message id consts #3282

Conversation

Bravo555 commented Dec 4, 2024

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

github-actions bot commented Dec 5, 2024 • edited Loading

Robot Results

didier-wenzek left a comment

Choose a reason for hiding this comment

jarhodes314 left a comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

albinsuresh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bravo555 commented Dec 10, 2024

didier-wenzek commented Dec 10, 2024

codecov bot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 5, 2024 •

edited

Loading