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

feat: add operation mapping for device profile #3015

Merged
merged 8 commits into from
Aug 7, 2024

Conversation

Ruadhri17
Copy link
Contributor

@Ruadhri17 Ruadhri17 commented Jul 25, 2024

Proposed changes

TODO:

  • test for main device mapping
  • test for child device mapping
  • doc test for c8y deserializer
  • test deserilaizer

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

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 95.41640% with 71 lines in your changes missing coverage. Please review.

Project coverage is 78.5%. Comparing base (dc7c336) to head (5358ddc).
Report is 3 commits behind head on main.

Files Patch % Lines
...pper_ext/src/operations/handlers/device_profile.rs 96.6% 4 Missing and 38 partials ⚠️
...xtensions/c8y_mapper_ext/src/operations/convert.rs 83.6% 6 Missing and 2 partials ⚠️
crates/core/tedge_api/src/device_profile.rs 84.6% 6 Missing ⚠️
crates/extensions/c8y_mapper_ext/src/converter.rs 57.1% 5 Missing and 1 partial ⚠️
crates/core/c8y_api/src/json_c8y_deserializer.rs 98.3% 1 Missing and 2 partials ⚠️
crates/core/tedge_api/src/commands.rs 33.3% 2 Missing ⚠️
.../tedge_config/src/tedge_config_cli/tedge_config.rs 75.0% 0 Missing and 1 partial ⚠️
crates/extensions/c8y_mapper_ext/src/config.rs 0.0% 1 Missing ⚠️
...xtensions/c8y_mapper_ext/src/operations/handler.rs 83.3% 0 Missing and 1 partial ⚠️
...ions/c8y_mapper_ext/src/operations/handlers/mod.rs 66.6% 1 Missing ⚠️
Additional details and impacted files
Files Coverage Δ
crates/core/c8y_api/src/smartrest/inventory.rs 70.2% <100.0%> (+0.9%) ⬆️
...core/c8y_api/src/smartrest/smartrest_serializer.rs 90.4% <100.0%> (+<0.1%) ⬆️
crates/core/tedge_api/src/lib.rs 100.0% <ø> (ø)
crates/core/tedge_api/src/mqtt_topics.rs 88.5% <100.0%> (+1.2%) ⬆️
crates/extensions/c8y_mapper_ext/src/lib.rs 90.0% <100.0%> (+1.1%) ⬆️
crates/extensions/c8y_mapper_ext/src/tests.rs 92.7% <100.0%> (-0.1%) ⬇️
.../tedge_config/src/tedge_config_cli/tedge_config.rs 73.6% <75.0%> (+<0.1%) ⬆️
crates/extensions/c8y_mapper_ext/src/config.rs 51.5% <0.0%> (-0.3%) ⬇️
...xtensions/c8y_mapper_ext/src/operations/handler.rs 95.1% <83.3%> (-0.2%) ⬇️
...ions/c8y_mapper_ext/src/operations/handlers/mod.rs 77.0% <66.6%> (-0.1%) ⬇️
... and 6 more

... and 7 files with indirect coverage changes

@Ruadhri17 Ruadhri17 force-pushed the device-profile-mapping branch from 7c3536a to 5e02a3b Compare July 25, 2024 18:35
@Ruadhri17 Ruadhri17 force-pushed the device-profile-mapping branch from 5e02a3b to 717e594 Compare July 25, 2024 19:09
@Ruadhri17 Ruadhri17 force-pushed the device-profile-mapping branch from 717e594 to c46a879 Compare July 26, 2024 12:09
@Ruadhri17 Ruadhri17 force-pushed the device-profile-mapping branch from c46a879 to 403d198 Compare July 26, 2024 12:10
@rina23q rina23q added theme:c8y Theme: Cumulocity related topics theme:profile Device Profile labels Jul 26, 2024
Copy link
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions

crates/core/c8y_api/src/json_c8y_deserializer.rs Outdated Show resolved Hide resolved
crates/core/c8y_api/src/json_c8y_deserializer.rs Outdated Show resolved Hide resolved
crates/core/c8y_api/src/json_c8y_deserializer.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mapping logic looks fine in general. But spotted a few corner cases.

The device profile payload may or may not contain all of software, firmware and configuration values at all times. If firmware is not included, it won't even be there in the cloud payload. But the current logic expects that key at all times, failing to convert otherwise. software and configuration can be empty arrays as well but not absent. But, I'd be defensive and code against their absence as well. Also, it'd also be nice if those entries are excluded from the mapped tedge command payload when those arrays are empty as well, just to reduce noise. But that'd be a secondary improvement.

Also, improve the test coverage for softwareType conversions as well, by using type suffixes in versions.

crates/core/c8y_api/src/json_c8y_deserializer.rs Outdated Show resolved Hide resolved
crates/core/tedge_api/src/device_profile_command.rs Outdated Show resolved Hide resolved
@Ruadhri17 Ruadhri17 marked this pull request as ready for review July 30, 2024 18:07
@Ruadhri17 Ruadhri17 force-pushed the device-profile-mapping branch from b696494 to 0d00a20 Compare July 30, 2024 18:18
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request July 30, 2024 18:26 — with GitHub Actions Inactive
Copy link
Contributor

github-actions bot commented Jul 30, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
491 0 2 491 100 1h23m39.204247999s

Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only major missing piece is the status update mapping.

Although the test coverage has improved, a few more cases can be added as highlighted. Also, note that the codecov report isn't favourable either. You'll have to make it happy as well.

@Ruadhri17 Ruadhri17 force-pushed the device-profile-mapping branch from ce7f2ab to 41a8f23 Compare July 31, 2024 16:39
@rina23q
Copy link
Member

rina23q commented Aug 6, 2024

While I am testing end-to-end, I found that handling for the device_profile command metadata is missing. This means, c8y_DeviceProfile is never declared in 114 message...

This match needs to be expanded. The implementation should be very similar to the case OperationType::Restart.

Channel::CommandMetadata { operation } => {
self.validate_operation_supported(operation, &source)?;
match operation {
OperationType::Restart => self.register_restart_operation(&source).await,
OperationType::SoftwareList => {
self.register_software_list_operation(&source, message)
.await
}
OperationType::SoftwareUpdate => {
self.register_software_update_operation(&source).await
}
OperationType::LogUpload => self.convert_log_metadata(&source, message),
OperationType::ConfigSnapshot => {
self.convert_config_snapshot_metadata(&source, message)
}
OperationType::ConfigUpdate => {
self.convert_config_update_metadata(&source, message)
}
OperationType::FirmwareUpdate => {
self.register_firmware_update_operation(&source)
}
OperationType::Custom(c8y_op_name) => {
self.register_custom_operation(&source, c8y_op_name)
}
_ => Ok(vec![]),
}
}

I'll let you decide if you address this in this PR or a follow-up PR.

@rina23q
Copy link
Member

rina23q commented Aug 6, 2024

Another problem: c8y-mapper doesn't subscribe to ../cmd/device_profile topic and ../cmd/device_profile/+ topic. You have to add something like below

 if capabilities.device_profile { 
     topics.add_all(crate::operations::device_profile::topic_filter(&mqtt_schema)); 
 } 

to this file

if capabilities.log_upload {
topics.add_all(crate::operations::log_upload::log_upload_topic_filter(
&mqtt_schema,
));
}
if capabilities.config_snapshot {
topics.add_all(crate::operations::config_snapshot::topic_filter(
&mqtt_schema,
));
}
if capabilities.config_update {
topics.add_all(crate::operations::config_update::topic_filter(&mqtt_schema));
}
if capabilities.firmware_update {
topics.add_all(
crate::operations::firmware_update::firmware_update_topic_filter(&mqtt_schema),
);
}

Copy link
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed both issue were fixed. Thank you.

Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature is working as expected. But, the codecov report is not at all favourable. Something needs to be done to get it in shape.

@Ruadhri17 Ruadhri17 force-pushed the device-profile-mapping branch from a92c04d to 1e39e2d Compare August 7, 2024 14:58
@Ruadhri17 Ruadhri17 force-pushed the device-profile-mapping branch from 1e39e2d to 5358ddc Compare August 7, 2024 15:44
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request August 7, 2024 15:44 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 added this pull request to the merge queue Aug 7, 2024
Merged via the queue into thin-edge:main with commit 6e9c25a Aug 7, 2024
33 checks passed
@gligorisaev
Copy link
Contributor

The feature is working as expected, QA checked the functionality according description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:c8y Theme: Cumulocity related topics theme:profile Device Profile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants