refactor: Extract operation implementation details from CumulocityConverter#3260
Conversation
Codecov ReportAttention: Patch coverage is Additional details and impacted files📢 Thoughts on this report? Let us know! |
BTreeMap for Operations storageCumulocityConverter
Using it multiple times will create operations with the same names, but its impossible to have multiple operation implementations for the same operation name Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
51774d9 to
7efcbfc
Compare
Robot Results
|
7efcbfc to
2f0c534
Compare
| .expect("entity should've been checked before that it's not a service"); | ||
|
|
||
| let prev_operation = current_operations.remove_operation(&operation.name); | ||
| let prev_operation = current_operations.insert_operation(operation); |
There was a problem hiding this comment.
This change seems weird at first ... but is correct!
Previously any operation with the same name was removed before adding a new version of the same operation.
| /// Required because when given an external id of the main device when creating an operation, we want to create it | ||
| /// in a main directory, instead of a subdirectory. |
There was a problem hiding this comment.
Okay. It makes sense for this PR focus on code refactoring without any user-visible changes, but this asymmetry between the main device and the child devices makes things more complicated than they should.
There was a problem hiding this comment.
I don't fully get what you mean. Do you mean this PR specifically, or the codebase more generally?
One maybe unnecessary change that I did was instead of having "operations for the main device" in one field and "operations for child devices" in a hashmap, i put it all in a single hashmap, so that CumulocityConverter only passes along the external id and doesn't have to care if a device is a child device or a main device. In retrospect the functions can still take an external id but the operations for the main device can be made a normal field instead of a hashmap entry, since "operations for the main device" is something that always exists and putting it in a hashmap where it can be removed introduces an extra failure mode.
There was a problem hiding this comment.
My point is in no case to disagree with this PR which is a real improvement reducing the accidental complexity of the c8y mapper.
My comment is coming from the fact that you (correctly) feel the need to add a comment on why we need the main device external id here (while we don't for the child devices).
| EntityType::MainDevice => Ok(self.operations.filter_by_topic(topic, prefix)), | ||
| EntityType::ChildDevice => { | ||
| if let Some(operations) = self.children.get(device_xid) { | ||
| Ok(operations.filter_by_topic(topic, prefix)) | ||
| } else { | ||
| Ok(Vec::new()) | ||
| } | ||
| } | ||
| EntityType::MainDevice | EntityType::ChildDevice => Ok(self | ||
| .supported_operations | ||
| .get_operation_handlers(device_xid, topic, prefix)), |
There was a problem hiding this comment.
This is one of the benefit of this PR, pushing away the artificial difference that has been introduced between the main and the child devices (see also: #3260 (comment))
didier-wenzek
left a comment
There was a problem hiding this comment.
Approved. This is nice clarification of the code base.
refactor: Extract operation implementation details from `CumulocityConverter`
| //! This module should encompass loading and saving supported operations from and to the filesystem. | ||
| //! | ||
| //! This module works with a c8y operations directory, which by default is `/etc/tedge/operations/c8y`. This directory | ||
| //! contains operation files for the main device and directories that contain operation files for child devices. |
There was a problem hiding this comment.
| //! contains operation files for the main device and directories that contain operation files for child devices. | |
| //! contains operation files for the main device, directories that contain operation files for child devices and operation templates for all devices. |
Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
2f0c534 to
e5eb707
Compare
TODO
Proposed changes
This PR introduces an
SupportedOperationsstruct that is used byCumulocityConverterto load and save operations.It achieves following things:
CumulocityConverterinto new operations module: The mainCumulocityConverterdoesn't create any files or symlinks, or doesn't need to create paths, the logic of mapping supported operations to the filesystem is contained in its own moduleCumulocityConverter: instead of having separateoperations: Operationsandchildren: HashMap<Child, Operations>,SupportedOperationhandles this distinction, the converter just passes an external idBTreeMapfor to have only one operation under a given name and to ensure consistent orderingTypes of changes
Paste Link to the issue
Operationsstruct refactor #3256Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments