feat: add an option to use c8y operation ID to update its status #3076

rina23q · 2024-08-21T14:40:51Z

Proposed changes

Cumulocity 10.18 version added 504-506 SmartREST static templates, which enables users to update operation status with its operation ID. This PR adds a new tedge config to switch either 501-503 or 504-506 for operation status updates.

One can change the setting by tedge config. true (504-506), false (501-503). Ideally making auto option, however, this is out of scope of this PR.

tedge config set c8y.smartrest.use_operation_id true

Note: for the naming decision, see this thread.

This change excludes the supports for below since they react to SmartREST operation messages, not JSON over MQTT. This means it's not possible to take its operation ID. Also, we don't generate thin-edge commands for these operations.

custom operation plugin
legacy c8y-firmware-plugin

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

#2616

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

codecov · 2024-08-21T14:55:15Z

Codecov Report

Attention: Patch coverage is 98.73950% with 12 lines in your changes missing coverage. Please review.

Files	Patch %	Lines
crates/extensions/c8y_mapper_ext/src/converter.rs	73.68%	5 Missing ⚠️
...ions/c8y_mapper_ext/src/operations/handlers/mod.rs	94.02%	4 Missing ⚠️
crates/extensions/c8y_mapper_ext/src/config.rs	50.00%	2 Missing ⚠️
...core/c8y_api/src/smartrest/smartrest_serializer.rs	98.46%	1 Missing ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2024-08-21T16:35:30Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
494	0	2	494	100	1h31m34.715098s

crates/common/tedge_config/src/tedge_config_cli/models/c8y.rs

didier-wenzek

I will be really happy to merge this PR. I just have one question.

didier-wenzek · 2024-08-27T07:31:07Z

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

+            /// Switch using 501-503 (without operation ID) or 504-506 (with operation ID) SmartREST messages for operation status update
+            #[tedge_config(example = "true", default(value = true))]
+            with_operation_id: bool,


What will be the behavior if operation ids are enabled on the device but not available on the c8y instance? Would things work? If not how the user will be notified? By error messages or stuck operations?

What will be the behavior if operation ids are enabled on the device but not available on the c8y instance? Would things work?

No, it won't work (that's we want to improve with auto option)

If not how the user will be notified? By error messages or stuck operations?

I can't really test it since all tenants available for us are already 10.18 or above. User will definitely receive an error message on c8y/s/e for unknown static template like below.

[c8y/s/us] 900 [c8y/s/e] 40,900,No static template for this message id

In this case, operation will be executed and successful/failed in thin-edge, but the status will stay PENDING in c8y.

What's the plan to implement auto detection? Can we probe for errors on c8y/s/e when sending 504-506 messages and then fallback to the previous API?

What's the plan to implement auto detection? Can we probe for errors on c8y/s/e when sending 504-506 messages and then fallback to the previous API?

Definitely it's one of the doable options. We have similar issue for Advanced Software Management (requirement: 10.14> and dedicated Microservice). I want to have a consistent solution for this and Advanced Software Management. Probably we need to discuss Device Management Team to choose the best solution.

tests/RobotFramework/tests/cumulocity/software_management/software.robot

albinsuresh

LGTM.

albinsuresh · 2024-08-27T06:54:21Z

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

+    fail_operation("505", op_id, reason)
+}
+
+fn fail_operation(number: &str, operation: &str, reason: &str) -> String {


Suggested change

fn fail_operation(number: &str, operation: &str, reason: &str) -> String {

fn fail_operation(template_id: &str, operation: &str, reason: &str) -> String {

Addressed all your renaming suggestion in this commit: 2e15950

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

albinsuresh · 2024-08-27T08:26:52Z

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

+
+            /// Switch using 501-503 (without operation ID) or 504-506 (with operation ID) SmartREST messages for operation status update
+            #[tedge_config(example = "true", default(value = true))]
+            with_operation_id: bool,


Suggested change

with_operation_id: bool,

direct_operation_update: bool,

Not fully happy about this suggestion either. Accept only if you're convinced.

The key will look c8y.smartrest.direct_operation_update. I don't think what "direct" means is so clear. At least, not for me. How about something in between my proposal and yours, c8y.smartrest.operation_id_update?

My bikeshed take would be something like c8y.smartrest.operation_explicit_id or operation_template_explicit_id

I see no reason to change anything here: with_operation_id is simple and clear.

Yeah, direct isn't very clear. I kinda liked operation_explicit_id as well. How about something even more direct: use_operation_id?

I vote +1 for use_operation_id

didier-wenzek

Approved

crates/core/tedge_api/src/mqtt_topics.rs

crates/extensions/c8y_firmware_manager/src/worker.rs

Cumulocity 10.18 version added 504-506 SmartREST static templates, which enables users to update operation status with its operation ID. Add a new tedge config to switch either 501-503 or 504-506 for operation status updates. Signed-off-by: Rina Fujino <[email protected]>

Signed-off-by: Rina Fujino <[email protected]>

rina23q temporarily deployed to Test Pull Request August 21, 2024 14:40 — with GitHub Actions Inactive

rina23q added the theme:c8y Theme: Cumulocity related topics label Aug 21, 2024

rina23q had a problem deploying to Test Auto August 21, 2024 16:18 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request August 21, 2024 19:34 — with GitHub Actions Failure

rina23q temporarily deployed to Test Pull Request August 21, 2024 20:02 — with GitHub Actions Inactive

rina23q had a problem deploying to Test Auto August 22, 2024 08:20 — with GitHub Actions Failure

didier-wenzek reviewed Aug 22, 2024

View reviewed changes

crates/common/tedge_config/src/tedge_config_cli/models/c8y.rs Outdated Show resolved Hide resolved

rina23q force-pushed the improve/2616/use-smartrest-504-505-506-for-operation-status-update branch from 4814b7a to e366d50 Compare August 23, 2024 20:32

rina23q had a problem deploying to Test Pull Request August 23, 2024 20:32 — with GitHub Actions Failure

rina23q temporarily deployed to Test Pull Request August 23, 2024 20:54 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto August 23, 2024 21:01 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Pull Request August 26, 2024 12:05 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto August 26, 2024 13:00 — with GitHub Actions Inactive

rina23q force-pushed the improve/2616/use-smartrest-504-505-506-for-operation-status-update branch from cd275db to 5e95aa0 Compare August 26, 2024 13:03

rina23q temporarily deployed to Test Pull Request August 26, 2024 13:03 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto August 26, 2024 13:32 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Pull Request August 26, 2024 14:49 — with GitHub Actions Inactive

rina23q marked this pull request as ready for review August 26, 2024 14:50

rina23q requested review from albinsuresh, jarhodes314, gligorisaev and a team as code owners August 26, 2024 14:50

rina23q requested a review from Bravo555 August 26, 2024 14:50

rina23q temporarily deployed to Test Auto August 26, 2024 14:54 — with GitHub Actions Inactive

didier-wenzek reviewed Aug 27, 2024

View reviewed changes

gligorisaev reviewed Aug 27, 2024

View reviewed changes

tests/RobotFramework/tests/cumulocity/software_management/software.robot Show resolved Hide resolved

gligorisaev reviewed Aug 27, 2024

View reviewed changes

tests/RobotFramework/tests/cumulocity/software_management/software.robot Show resolved Hide resolved

albinsuresh approved these changes Aug 27, 2024

View reviewed changes

didier-wenzek approved these changes Aug 27, 2024

View reviewed changes

rina23q had a problem deploying to Test Pull Request August 27, 2024 12:27 — with GitHub Actions Failure

Bravo555 self-assigned this Aug 27, 2024

Bravo555 requested changes Aug 27, 2024

View reviewed changes

crates/core/tedge_api/src/mqtt_topics.rs Outdated Show resolved Hide resolved

rina23q had a problem deploying to Test Pull Request August 27, 2024 12:53 — with GitHub Actions Failure

Bravo555 reviewed Aug 27, 2024

View reviewed changes

crates/extensions/c8y_firmware_manager/src/worker.rs Show resolved Hide resolved

rina23q temporarily deployed to Test Pull Request August 27, 2024 14:32 — with GitHub Actions Inactive

Bravo555 approved these changes Aug 27, 2024

View reviewed changes

rina23q temporarily deployed to Test Auto August 27, 2024 14:40 — with GitHub Actions Inactive

rina23q added 2 commits August 27, 2024 15:11

Add test to confirm operation status is updated by ID

e53921c

Signed-off-by: Rina Fujino <[email protected]>

rina23q force-pushed the improve/2616/use-smartrest-504-505-506-for-operation-status-update branch from 0136b9e to e53921c Compare August 27, 2024 15:11

rina23q temporarily deployed to Test Pull Request August 27, 2024 15:11 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto August 27, 2024 15:18 — with GitHub Actions Inactive

rina23q added this pull request to the merge queue Aug 27, 2024

Merged via the queue into thin-edge:main with commit c4d27ff Aug 27, 2024
33 checks passed

rina23q mentioned this pull request Aug 27, 2024

Use c8y operation ID for operation status update #2616

Closed

Bravo555 removed their assignment Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add an option to use c8y operation ID to update its status #3076

feat: add an option to use c8y operation ID to update its status #3076

rina23q commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

didier-wenzek left a comment

didier-wenzek Aug 27, 2024

rina23q Aug 27, 2024 •

edited

Loading

didier-wenzek Aug 27, 2024

rina23q Aug 27, 2024 •

edited

Loading

albinsuresh left a comment

albinsuresh Aug 27, 2024

rina23q Aug 27, 2024

albinsuresh Aug 27, 2024

rina23q Aug 27, 2024

Bravo555 Aug 27, 2024

didier-wenzek Aug 27, 2024

albinsuresh Aug 27, 2024

rina23q Aug 27, 2024

didier-wenzek left a comment

	fn fail_operation(number: &str, operation: &str, reason: &str) -> String {
	fn fail_operation(template_id: &str, operation: &str, reason: &str) -> String {

feat: add an option to use c8y operation ID to update its status #3076

feat: add an option to use c8y operation ID to update its status #3076

Conversation

rina23q commented Aug 21, 2024 • edited Loading

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Aug 21, 2024 • edited Loading

Codecov Report

github-actions bot commented Aug 21, 2024 • edited Loading

Robot Results

didier-wenzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rina23q Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rina23q Aug 27, 2024 • edited Loading

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

didier-wenzek left a comment

Choose a reason for hiding this comment

rina23q commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

rina23q Aug 27, 2024 •

edited

Loading

rina23q Aug 27, 2024 •

edited

Loading