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

c8y-mapper sends software list to advanced software management endpoint #2771

Conversation

rina23q
Copy link
Member

@rina23q rina23q commented Mar 8, 2024

Proposed changes

Since c8y 10.14, advanced software management feature is supported together with the microservice "Advanced-software-mgmt". This PR enables users to use the feature. For more details about the feature, please read the c8y user doc.

Introduced two tedge configs:

  1. c8y.software_management.api
    To switch legacy software management (= sending list via HTTP) and advanced one (= sending list via SmartREST).
    By default, it is set to legacy. To enhance the detection, the ticket Cumulocity software api configuration options #2778 is created.
  2. c8y.software_management.with_types
    If mapper publishes c8y_SupportedSoftwareTypes to the inventory or not. If the type is set in its managed object, c8y stop using c8y_SoftwareList fragment to look up the installed software list. By default, it is set to false so that we can still use c8y_SoftwareList fragment for testing and so on.

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

#2654

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 Mar 8, 2024

Codecov Report

Attention: Patch coverage is 87.41259% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 77.3%. Comparing base (19773d6) to head (be43806).
Report is 10 commits behind head on main.

❗ Current head be43806 differs from pull request most recent head 667652a. Consider uploading reports for the commit 667652a to get more accurate results

Additional details and impacted files
Files Coverage Δ
...core/c8y_api/src/smartrest/smartrest_serializer.rs 89.6% <99.2%> (+5.9%) ⬆️
.../tedge_config/src/tedge_config_cli/tedge_config.rs 80.1% <80.0%> (-0.1%) ⬇️
crates/extensions/c8y_mapper_ext/src/inventory.rs 85.5% <81.8%> (-0.1%) ⬇️
crates/extensions/c8y_mapper_ext/src/config.rs 46.9% <50.0%> (-0.2%) ⬇️
crates/extensions/c8y_mapper_ext/src/tests.rs 92.7% <94.3%> (+<0.1%) ⬆️
...tedge_config_cli/models/c8y_software_management.rs 56.2% <56.2%> (ø)
crates/extensions/c8y_mapper_ext/src/converter.rs 83.4% <60.0%> (+0.7%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Mar 11, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
426 0 3 426 100 0s

@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from a764643 to 6ede9d3 Compare March 11, 2024 20:33
@rina23q rina23q marked this pull request as ready for review March 11, 2024 20:34
@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from 6ede9d3 to 7e81021 Compare March 11, 2024 20:54
@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from 7e81021 to ee8f32f Compare March 12, 2024 23:08
@rina23q rina23q temporarily deployed to Test Pull Request March 12, 2024 23:08 — with GitHub Actions Inactive
@rina23q rina23q temporarily deployed to Test Pull Request March 13, 2024 13:42 — with GitHub Actions Inactive
@rina23q rina23q requested a review from a team as a code owner March 13, 2024 14:29
@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from a70e8dc to 30741ee Compare March 13, 2024 14:35
@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from 30741ee to d2655fa Compare March 13, 2024 14:53
@rina23q rina23q temporarily deployed to Test Pull Request March 13, 2024 14:53 — with GitHub Actions Inactive
@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from d2655fa to c96de86 Compare March 13, 2024 16:37
@rina23q rina23q temporarily deployed to Test Pull Request March 13, 2024 16:37 — with GitHub Actions Inactive
@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from c96de86 to d2655fa Compare March 13, 2024 16:52
@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from fa391cb to cb75ce0 Compare March 13, 2024 17:15
@rina23q rina23q temporarily deployed to Test Pull Request March 13, 2024 17:15 — with GitHub Actions Inactive
Comment on lines +1461 to +1490
// Send c8y_SupportedSoftwareTypes, which is introduced in c8y >= 10.14
let data = SoftwareCommandMetadata::from_json(message.payload_str()?)?;
let payload = json!({"c8y_SupportedSoftwareTypes": data.types}).to_string();
let topic = self.get_inventory_update_topic(target)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR is not the right place to improve on this, but we will have to find a better pattern before being overwhelmed by this kind of artificial and error-prone complexity.

  • Artificial, because we only need to translate a tedge MQTT message into a c8y one. This is not obvious from these lines.
  • Error-prone, because the message must be partially parsed before calling this method (to extract the target and message topic from the message.topic).
  • I agree this is not so easy, as we need to access the inventory to translate the tedge entity id into a c8y external id.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we can work to improve, however, I'm not sure how much I should do in this PR...

By the way, since this action updates the device's c8y inventory, should the mapper publish to our twin topic, instead of directly publishing to the JSON over MQTT topic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how much I should do in this PR...

Definitely not in this PR as this would impact all the messages received by the c8y converter.

Copy link
Contributor

Choose a reason for hiding this comment

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

should the mapper publish to our twin topic, instead of directly publishing to the JSON over MQTT topic?

No, the twin topic is where child devices publish their metadata. The c8y mapper should react on these messages, not push data there.

@reubenmiller
Copy link
Contributor

reubenmiller commented Mar 20, 2024

@rina23q you'll have to rebase your PR to get the workflows to work again (since #2781 was merged)

@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from c594fc3 to be43806 Compare March 20, 2024 16:09
@rina23q rina23q temporarily deployed to Test Pull Request March 20, 2024 16:09 — with GitHub Actions Inactive
@rina23q rina23q temporarily deployed to Test Pull Request March 21, 2024 09:55 — with GitHub Actions Inactive
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved

Since Cumulocity 10.14, it is recommended to send software list to the
advanced software management endpoint. By default, the mapper publishes
its software list via SmartREST.

If `c8y.software_management.api` is set to 'true', the mapper also sends
c8y_SupportedSoftwareTypes to inventory via JSON over MQTT.

To keep backwards compatibility, if `c8y.software_management.api` is
set to "legacy", the mapper posts to c8y_SoftwareList fragment to
the inventory API via HTTP.

Signed-off-by: Rina Fujino <[email protected]>
@rina23q rina23q force-pushed the improve/2654/c8y-mapper-supports-advanced-software-type branch from 5546a35 to 667652a Compare March 21, 2024 14:00
@rina23q rina23q temporarily deployed to Test Pull Request March 21, 2024 14:00 — with GitHub Actions Inactive
@rina23q rina23q added this pull request to the merge queue Mar 21, 2024
Merged via the queue into thin-edge:main with commit 4109ef6 Mar 21, 2024
30 checks passed
@rina23q rina23q deleted the improve/2654/c8y-mapper-supports-advanced-software-type branch July 18, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants