fix: do not overwrite latest twin data from inventory.json values on mapper restart#3742
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
| map.entry("name").or_insert(json!(self.device_name)); | ||
| map.entry("type").or_insert(json!(self.device_type)); |
There was a problem hiding this comment.
The mapper publishing the device name and type from the tedge config as twin data on startup was already wrong, as this would have lead to one mapper overwriting the name overridden by another mapper in the config (since different cloud mappers can have their own device ids). The agent shouldn't be doing this either, as the impact would be the same and hence this logic was simply dropped and not ported to the agent either.
| if self | ||
| .entity_store | ||
| .get_twin_fragment(&main_device, &key) | ||
| .is_none() |
There was a problem hiding this comment.
The side effect of this guard is that the user can not use the inventory.json file to update any twin values after the initial startup. They must use the twin/ topics for any updates. But, new values can be added to the file, which will be processed on subsequent restarts.
Robot Results
|
3c250aa to
0ff2887
Compare
didier-wenzek
left a comment
There was a problem hiding this comment.
Approved (from a Rust perspective). I let @reubenmiller judge if the simplified behavior is okay as some device/twon metadata are no pushed to C8y on startup.
| pub struct C8yMapperConfig { | ||
| pub device_id: String, | ||
| pub device_topic_id: EntityTopicId, | ||
| pub device_type: String, |
There was a problem hiding this comment.
Nice to see this property gone. This has been a source of troubles from the start.
0ff2887 to
95b791b
Compare
rina23q
left a comment
There was a problem hiding this comment.
The change looks very nice. Typo needs to be fixed before merge :)
| "c8y_Agent" : { | ||
| "name": "thin-edge.io", | ||
| "url": "https://thin-edge.io", | ||
| "version": version | ||
| } |
There was a problem hiding this comment.
I am not asking to fix, but I found it interesting. In the c8y docs, c8y_Agent.version is marked mandatory. Though I know we can put anything via REST API or JSON over MQTT.
95b791b to
799e662
Compare
tests/RobotFramework/tests/cumulocity/inventory/inventory_update_bootstrap.robot
Show resolved
Hide resolved
There was a problem hiding this comment.
Since the reading of the inventory.json is now done by the tedge-agent service, we'll have to updated the docs to match as it used to guide users to restart the tedge-mapper-c8y service
To see the changes you need to restart the tedge-mapper.
It could also be worth mentioning that when removing values from the inventory.json file, the user will also have to clear older values in the entity store (like below), otherwise the values can be re-published again to the cloud when the tedge-mapper-c8y service is restarted.
tedge http delete /te/v1/entities/device/main///twin/type
799e662 to
c3203d8
Compare
c3203d8 to
a8a48b2
Compare
Proposed changes
tedge-mapper-c8ytotedge-agentinventory.jsonfile totwintopics only after all the existing retained twin messages are processed by the agent, so that already publishedtwinkeys are skipped (to avoid overwriting them with old values in the file).Types of changes
Paste Link to the issue
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments