refactor: Reorganise c8y_mapper_ext::operations module

[Bravo555]

Proposed changes

No functional changes were made.

operations module was reorganised into submodules to better separate different concerns:

• plain conversions of operations-related messages
• topic filters
• spawning and joining of operation tasks
• handling different types of operations

This prepares the OperationHandler to more neatly be converted into an actor, as the lower-level handling of different operation types should remain unaffected.

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

• Simplify the operation handling code #2880

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]

Codecov Report

Attention: Patch coverage is 89.38172% with 79 lines in your changes missing coverage. Please review.

Project coverage is 78.1%. Comparing base (d74a8c8) to head (e419058).
Report is 53 commits behind head on main.

Additional details and impacted files

Files	Coverage Δ
crates/extensions/c8y_mapper_ext/src/lib.rs	88.8% <ø> (ø)
...per_ext/src/operations/handlers/config_snapshot.rs	91.6% <ø> (ø)
...apper_ext/src/operations/handlers/config_update.rs	92.6% <ø> (ø)
...per_ext/src/operations/handlers/firmware_update.rs	93.1% <ø> (ø)
...y_mapper_ext/src/operations/handlers/log_upload.rs	92.7% <ø> (ø)
.../c8y_mapper_ext/src/operations/handlers/restart.rs	80.0% <ø> (ø)
...apper_ext/src/operations/handlers/software_list.rs	80.9% <ø> (ø)
...per_ext/src/operations/handlers/software_update.rs	93.6% <ø> (ø)
...extensions/c8y_mapper_ext/src/operations/upload.rs	94.5% <ø> (ø)
crates/extensions/c8y_mapper_ext/src/tests.rs	92.7% <100.0%> (+<0.1%)	⬆️
... and 4 more

... and 46 files with indirect coverage changes

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
478	0	2	478	100	1h16m40.883193s

[rina23q]

The directory name implementation can be improved since all codes in repo are a kind of "implementation". Maybe contexts (as all files are inheriting OperationContext) or commands (as all files are specific operation type but the name operation is already taken).

[albinsuresh]

As @rina23q already raised, I'm not quite happy with the module named implementation either. How about calling that wrapper module handlers since those modules are extensions of the common handler?

[albinsuresh]

Not quite happy with this module with all the operation specific filters. I see that you wanted to separate the conversion aspects from the command handling aspects. In that case I get a feeling that we need two separate operation specific modules for each operation/command type: one with just the conversion aspects and other with the command processing aspects as done now.

[Bravo555]

Yeah, I wanted to separate these because reacting to operation status changes and MQTT topics are really different concerns, and I went on a bit of an autopilot separating modules, but looking at these functions, they're completely superfluous, the implementation is the same, just with different arguments.

I get a feeling that we need two separate operation specific modules for each operation/command type: one with just the conversion aspects and other with the command processing aspects as done now.

I think that wrt. the conversion aspect, there are really two things here: the topic filters (topic_filter module) and the conversions of incoming cumulocity messages that initiate thin-edge.io operations (convert module).

On the topic fiilters:

I'm not sure if we need a conversion module per operation type. Topic filters are necessary to create an MQTT subscription, and in this case I think the pattern is more like:

• OperationHandler handles some set of operations
• if CumulocityConverter gets an operation of type that can be handled by OperationHandler, it should pass the message to the handler
• so OperationHandler needs to expose the set of operations it supports, so that CumulocityConverter makes a proper MQTT subscription
• but it doesn't have to be per operation type. I think what's preferable is that OperationHandler produces only one topic filter, that covers all the operations that are enabled by the current configuration.

Also this will hopefully clear up more once we've made OperationHandler an actor: the topic filters will fit neatly into the message types of the new operations actor.

On the conversion functions in the convert module:

These are impl blocks on CumulocityConverter, which I've pulled under operations module because other than converting between Cumulocity messages and thin-edge.io messages, they perform some side effects like querying the entity store or calling into c8y proxy to obtain a URL where the operation has to upload a file.

This auxiliary communication with C8y when we receive an operation is very much symmetric to auxiliary communication with C8y when we terminate the operation (support or failure), so the long term plan is definitely to move these side-effects into OperationHandler, but right now it's very much coupled to CumulocityConverter which uses it primarily to convert between MQTT messages, so I think refactoring this will be best left as a follow-up.

Conclusion

• as for the topic filters, instead of having different topic filters for different operation types, there should be OperationHandler::topic_filter that just creates a filter for all operation types the handler is interested in, and CumulocityConverter should just call it and don't care about different operation types.
• as for the convert module functions, I think this should be left as a follow-up.

How does that sound?

[Bravo555]

implementation renamed in 80e6962, OperationHandler::topic_filter introduced in 8c484d0

[rina23q]

My comments are not related to the main code, so I would approve now.

[rina23q]

This one is equal to Capapilities::default().

[rina23q]

Nice simplification!

[albinsuresh]

LGTM