feat: support user topics when handling custom operation by Ruadhri17 · Pull Request #3189 · thin-edge/thin-edge.io

Ruadhri17 · 2024-10-15T23:18:33Z

Proposed changes

This PR adds support for user topics when handling custom operation. Custom topic can be provided in custom operation in following way (templates supported):

[exec]
topic = "${.bridge.topic_prefix}/custom/topic/one"
on_fragment = "com_cumulocity_model_WebCamDevice"
command = "/usr/bin/do_something.sh ${.payload.com_cumulocity_model_WebCamDevice.parameters.duration}"

Mapper replace template with mapper prefix (if needed) and subscribe to the topic. If template is different than ${.bridge.topic_prefix} then this topic will be skipped.

TODO:

Robot framework test

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

Use workflows for custom operation handlers #3095

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

github-actions · 2024-10-15T23:40:26Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
525	0	2	525	100	1h31m19.722918s

codecov · 2024-10-16T00:28:14Z

Codecov Report

Attention: Patch coverage is 83.62069% with 38 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/extensions/c8y_mapper_ext/src/converter.rs	84.26%	4 Missing and 10 partials ⚠️
crates/extensions/c8y_mapper_ext/src/config.rs	55.17%	13 Missing ⚠️
crates/core/c8y_api/src/smartrest/operations.rs	82.60%	5 Missing and 3 partials ⚠️
crates/extensions/c8y_mapper_ext/src/tests.rs	96.72%	0 Missing and 2 partials ⚠️
crates/common/mqtt_channel/src/topics.rs	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

gligorisaev · 2024-10-17T09:29:49Z

...rk/tests/cumulocity/custom_operation/custom_operation_command/custom_operation_command.robot

The added test step is doing what it was intended for

gligorisaev

Checked the whole test, The added test step is doing what it was intended for

reubenmiller · 2024-10-17T13:04:28Z

tests/RobotFramework/tests/cumulocity/custom_operation/custom_operation_command/c8y_Command_3

@@ -0,0 +1,4 @@
+[exec]
+topic = "${.topic.root_prefix}/custom/topic/one"


What does ${.topic.root_prefix} refer to?

It would be more common to be able to expand the mapper prefix (e.g. c8y or whether the cloud prefix etc.), and to avoid confusion with the topic.root_topic value (which is currently the local thin-edge.io topic prefix, e.g. te).

Sorry I just read the other code, and could answer the questions.

We should rename the variable as follows:

${.topic.root_prefix} => ${.bridge.topic_prefix}

The .bridge.topic_prefix then aligns with the tedge.toml value c8y.bridge.topic_prefix, where the leading c8y. prefix is not required as the mapper already knows what context it is (as the mapper is started with tedge-mapper c8y)

resolved by c1311aa

${.bridge.topic_prefix} sounds really ad-hoc and not extensible in the future.

I would be explicit with ${.config.c8y.bridge.topic_prefix} and more general with the ability to inject any config setting. Surely more useful for the script rather than the topic on which commands are expected, I agree. However, making the context explicit avoid consistency and extensibility issues.

Injecting any config value might be a bit too involved for this PR. Hence, it's okay to start with a simple string replacement (as currently implemented) but with an explicit config context (i.e. ${.config.c8y.bridge.topic_prefix}).

Though the ${.bridge.topic_prefix} value is actually the prefix is used by the cloud "profile" used by the mapper. And since the custom operation templates are under the context of the mapper's cloud already, the value does not need to "c8y" context helper.

the value does not need to "c8y" context helper.

Yes. My concern is that this looks like as a path templating system while it is not, just variable substitution using ${.bridge.topic_prefix} as a variable name.

Though doesn't the the property name topic suggest otherwise?

@didier-wenzek Sorry I misunderstood your point. I've opened a new discussion about the code in question. https://github.com/thin-edge/thin-edge.io/pull/3189/files#r1813004355

@reubenmiller And I misunderstood yours ;-)

After an offline discussion, I agree that it makes sense to use the bridge config and not the tedge config as root of the substitution in ${.bridge.topic_prefix}.

For the implementation, what is done is working and can be taken as fist step - even if not extensible.

rina23q

Still need to work and investigate the test failure, why 504&506 are sent twice.

rina23q · 2024-10-18T13:23:41Z

crates/core/c8y_api/src/smartrest/operations.rs

+    pub fn filter_by_topics(
+        &self,
+        topics: Vec<String>,
+        state: &GenericCommandState,


As per this comment, this function doesn't have to do anything with workflow's GenericCommandState for the topic. Instead, you need to parse prefix here.

resolved by c1311aa

rina23q · 2024-10-18T13:28:24Z

crates/extensions/c8y_mapper_ext/src/config.rs

+            for topic in topics {
+                // if topic contains a template for the root prefix, replace that template with mapper prefix.
+                // else if topic contains a template not for the root prefix, reject that topic when appending the topic filter
+                // else if no template is present, simply add that topic to the topic filter (assuming that provided pattern is valid)
+                let topic = topic
+                    .split_inclusive("}")
+                    .flat_map(|s| match s.find("${") {
+                        None => vec![s],
+                        Some(i) => {
+                            let (prefix, template) = s.split_at(i);
+                            vec![prefix, template]
+                        }
+                    })
+                    .map(|s| {
+                        if let Some(template) = GenericCommandState::extract_path(s) {
+                            if template.eq(".topic.root_prefix") {
+                                prefix.as_str()
+                            } else {
+                                warn!("The custom topic '{topic}' is invalid and ignored.");
+                                s
+                            }
+                        } else {
+                            s
+                        }
+                    })
+                    .collect::<String>();
+


The logic here can be much simpler. You just need to replace ${.bridge.topic_prefix} by C8yMapperConfig::c8y_prefix.

resolved by c1311aa

didier-wenzek · 2024-10-29T15:16:23Z

crates/core/c8y_api/src/smartrest/operations.rs

                (None, Some(on_fragment)) => vec.push((on_fragment, op.clone())),
-                (Some(topic), Some(on_fragment)) if topic == topic_name => {
+                (Some(ref topic), Some(on_fragment))
+                    if record.inject_values_into_template(topic) == topic_name =>


It would be better to inject any ${.bridge...} config when the operations are loaded. The bridge config will not change after the mapper starts.

Suggested change

if record.inject_values_into_template(topic) == topic_name =>

if topic == topic_name =>

didier-wenzek · 2024-10-29T15:56:13Z

crates/core/c8y_api/src/smartrest/operations.rs

+    struct TestBridgeConfig {
+        c8y_prefix: TopicPrefix,
+    }


Why another struct TestBridgeConfig that is exactly a copy of BridgeConfig?

BridgeConfig is part of C8yMapperConfig so cannot provide it to operations but one is needed to run tests

didier-wenzek

Approved.

Signed-off-by: Krzysztof Piotrowski <K.R.Piotrowski17@gmail.com>

Ruadhri17 temporarily deployed to Test Pull Request October 15, 2024 23:18 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Auto October 15, 2024 23:24 — with GitHub Actions Inactive

Ruadhri17 force-pushed the custom-operation-topic branch from 8ad970a to 1fcdbbd Compare October 16, 2024 00:16

Ruadhri17 had a problem deploying to Test Pull Request October 16, 2024 00:16 — with GitHub Actions Failure

Ruadhri17 marked this pull request as ready for review October 16, 2024 09:02

Ruadhri17 requested review from albinsuresh, didier-wenzek, jarhodes314 and rina23q as code owners October 16, 2024 09:02

Ruadhri17 marked this pull request as draft October 16, 2024 09:53

Ruadhri17 force-pushed the custom-operation-topic branch from 1fcdbbd to e902e6e Compare October 16, 2024 18:21

Ruadhri17 had a problem deploying to Test Pull Request October 16, 2024 18:21 — with GitHub Actions Failure

Ruadhri17 force-pushed the custom-operation-topic branch from e902e6e to fadb1d1 Compare October 16, 2024 18:26

Ruadhri17 had a problem deploying to Test Pull Request October 16, 2024 18:26 — with GitHub Actions Failure

Ruadhri17 force-pushed the custom-operation-topic branch from fadb1d1 to 27bc06d Compare October 16, 2024 19:10

Ruadhri17 had a problem deploying to Test Pull Request October 16, 2024 19:10 — with GitHub Actions Failure

Ruadhri17 temporarily deployed to Test Pull Request October 16, 2024 20:20 — with GitHub Actions Inactive

Ruadhri17 had a problem deploying to Test Auto October 16, 2024 20:26 — with GitHub Actions Failure

Ruadhri17 force-pushed the custom-operation-topic branch from 121f19c to 29a8439 Compare October 16, 2024 20:30

Ruadhri17 temporarily deployed to Test Pull Request October 16, 2024 20:30 — with GitHub Actions Inactive

Ruadhri17 marked this pull request as ready for review October 16, 2024 20:30

Ruadhri17 requested review from a team and gligorisaev as code owners October 16, 2024 20:30

Ruadhri17 had a problem deploying to Test Auto October 16, 2024 20:36 — with GitHub Actions Failure

gligorisaev reviewed Oct 17, 2024

View reviewed changes

reubenmiller reviewed Oct 17, 2024

View reviewed changes

rina23q reviewed Oct 18, 2024

View reviewed changes

Ruadhri17 mentioned this pull request Oct 23, 2024

Missing system test for smartREST templates #3199

Closed

Ruadhri17 force-pushed the custom-operation-topic branch from 7942628 to 1640988 Compare October 23, 2024 17:00

Ruadhri17 temporarily deployed to Test Pull Request October 23, 2024 17:00 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Auto October 23, 2024 17:06 — with GitHub Actions Inactive

Ruadhri17 force-pushed the custom-operation-topic branch from 1640988 to 33ebe9a Compare October 28, 2024 17:19

Ruadhri17 had a problem deploying to Test Pull Request October 28, 2024 17:19 — with GitHub Actions Failure

Ruadhri17 force-pushed the custom-operation-topic branch from 33ebe9a to f8ee8f8 Compare October 28, 2024 17:21

Ruadhri17 temporarily deployed to Test Pull Request October 28, 2024 17:21 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Auto October 28, 2024 17:45 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Pull Request October 29, 2024 14:56 — with GitHub Actions Inactive

Ruadhri17 had a problem deploying to Test Auto October 29, 2024 15:03 — with GitHub Actions Failure

didier-wenzek reviewed Oct 29, 2024

View reviewed changes

Ruadhri17 temporarily deployed to Test Pull Request October 29, 2024 15:28 — with GitHub Actions Inactive

Ruadhri17 had a problem deploying to Test Auto October 29, 2024 15:36 — with GitHub Actions Failure

Ruadhri17 temporarily deployed to Test Pull Request October 29, 2024 15:43 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Auto October 29, 2024 15:49 — with GitHub Actions Inactive

didier-wenzek reviewed Oct 29, 2024

View reviewed changes

didier-wenzek approved these changes Oct 29, 2024

View reviewed changes

Ruadhri17 added 3 commits October 29, 2024 16:30

feat: support user topics when handling custom operations

85a09f3

Signed-off-by: Krzysztof Piotrowski <K.R.Piotrowski17@gmail.com>

test: add system test for custom topic

24b14d2

Signed-off-by: Krzysztof Piotrowski <K.R.Piotrowski17@gmail.com>

fix: replace incorrect topic prefix in smartrest test

0ca2b20

Signed-off-by: Krzysztof Piotrowski <K.R.Piotrowski17@gmail.com>

Ruadhri17 force-pushed the custom-operation-topic branch from 4c8e045 to 0ca2b20 Compare October 29, 2024 16:34

Ruadhri17 temporarily deployed to Test Pull Request October 29, 2024 16:34 — with GitHub Actions Inactive

Ruadhri17 enabled auto-merge October 29, 2024 16:35

Ruadhri17 had a problem deploying to Test Auto October 29, 2024 16:40 — with GitHub Actions Failure

Ruadhri17 temporarily deployed to Test Auto October 29, 2024 17:08 — with GitHub Actions Inactive

Ruadhri17 added this pull request to the merge queue Oct 29, 2024

Merged via the queue into thin-edge:main with commit 77c7a01 Oct 29, 2024

rina23q mentioned this pull request Nov 4, 2024

Add system test for the SmartREST templates #3202

Closed

11 tasks

		@@ -0,0 +1,4 @@
		[exec]
		topic = "${.topic.root_prefix}/custom/topic/one"

	if record.inject_values_into_template(topic) == topic_name =>
	if topic == topic_name =>

Conversation

Ruadhri17 commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

github-actions bot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

codecov bot commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gligorisaev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reubenmiller Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rina23q left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ruadhri17 commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

reubenmiller Oct 23, 2024 •

edited

Loading