[CIT-514] Add send software list via http #400

makr11st · 2021-08-24T11:18:10Z

Proposed changes

Add feature to publish software list to c8y via http.

Types of changes

What types of changes does your code introduce to thin-edge.io?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactorings 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)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

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)

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

…/CIT-514_sw_list_over_http

codecov-commenter · 2021-08-24T12:11:11Z

Codecov Report

Merging #400 (ba4f913) into main (3b7cf7a) will decrease coverage by 0.67%.
The diff coverage is 57.92%.

@@            Coverage Diff             @@
##             main     #400      +/-   ##
==========================================
- Coverage   64.73%   64.06%   -0.68%     
==========================================
  Files         105      106       +1     
  Lines        6667     6809     +142     
==========================================
+ Hits         4316     4362      +46     
- Misses       2351     2447      +96

Impacted Files	Coverage Δ
...e_mapper/src/sm_c8y_mapper/smartrest_serializer.rs	`87.80% <ø> (-3.81%)`	⬇️
mapper/tedge_mapper/src/sm_c8y_mapper/tests.rs	`0.00% <ø> (ø)`
mapper/tedge_mapper/src/sm_c8y_mapper/mapper.rs	`9.37% <18.36%> (+9.37%)`	⬆️
mapper/tedge_mapper/src/sm_c8y_mapper/json_c8y.rs	`93.58% <93.58%> (ø)`
...mapper/src/sm_c8y_mapper/smartrest_deserializer.rs	`96.70% <100.00%> (+0.52%)`	⬆️
tedge/src/cli/disconnect/cli.rs	`0.00% <0.00%> (ø)`
tedge/src/cli/connect/command.rs	`0.00% <0.00%> (ø)`
tedge/src/system_services/services.rs	`0.00% <0.00%> (ø)`
tedge/src/cli/connect/bridge_config.rs	`97.02% <0.00%> (ø)`
tedge/src/system_services/managers/bsd.rs	`0.00% <0.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b7cf7a...ba4f913. Read the comment docs.

PradeepKiruvale

There are many unwraps, should replace them with proper error handling.

PradeepKiruvale · 2021-08-24T12:37:20Z

common/mqtt_client/src/lib.rs

+{
+    fn from(topic: T) -> Self {
+        Topic {
+            name: topic.to_string(),


use into() instead of to_string()

As it is a generic function over type T and type T is bound to Display using into() is not possible, you have to use lower level function as for example to_string()

It's more appropriate to implement TryFrom and to call Topic::new().

Indeed a string might not be a valid topic name.

mapper/tedge_mapper/src/sm_c8y_mapper/mapper.rs

makr11st · 2021-08-24T13:43:51Z

There are many unwraps, should replace them with proper error handling.

Thanks, yes, I'm working on it, this is still draft.

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

PradeepKiruvale · 2021-08-25T05:24:24Z

mapper/tedge_mapper/src/sm_c8y_mapper/mapper.rs

@@ -27,7 +28,8 @@ impl TEdgeComponent for CumulocitySoftwareManagementMapper {
        let mqtt_config = mqtt_config(&tedge_config)?;
        let mqtt_client = Client::connect("SM-C8Y-Mapper", &mqtt_config).await?;

-        let sm_mapper = CumulocitySoftwareManagement::new(mqtt_client);
+        let mut sm_mapper = CumulocitySoftwareManagement::new(mqtt_client, tedge_config);
+        let () = sm_mapper.init().await?;


why init is async here, can it run without init?

This is a little tricky one, this init may fail and it seemed to be fair for it to be separated from the whole run method such that if you need to handle any error with it it would be easier, additionally it requires &mut self on the object to modify it (initialize c8y_client_id from external sources) and to not need to make entire run method mut.

mapper/tedge_mapper/src/sm_c8y_mapper/mapper.rs

…/CIT-514_sw_list_over_http

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

didier-wenzek · 2021-08-25T14:13:29Z

common/mqtt_client/src/lib.rs

+{
+    fn from(topic: T) -> Self {
+        Topic {
+            name: topic.to_string(),


It's more appropriate to implement TryFrom and to call Topic::new().

Indeed a string might not be a valid topic name.

mapper/tedge_mapper/src/sm_c8y_mapper/error.rs

sm/json_sm/src/lib.rs

mapper/tedge_mapper/src/component.rs

mapper/tedge_mapper/src/sm_c8y_mapper/mapper.rs

didier-wenzek · 2021-08-25T16:06:01Z

mapper/tedge_mapper/src/sm_c8y_mapper/mapper.rs

+        match tokio::time::timeout(Duration::from_secs(10), subscriber.next()).await {
+            Ok(Some(msg)) => msg.payload_str()?.to_string(),
+            Ok(None) => return Err(SMCumulocityMapperError::InvalidMqttMessage),
+            Err(err) => return Err(SMCumulocityMapperError::FromElapsed(err)),
+        };


What if we receive some msg that is not a JwtToken (i.e. with a code that is not 71)?

Is there a guaranty to receive only JwtToken on c8y/s/dat?

There is no explicit note that only the token can be send there, but also there is no mention in the documentation that the topic is used for anything else.

mapper/tedge_mapper/src/sm_c8y_mapper/smartrest_deserializer.rs

add try_new to jwt struct, remove from for Topic Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

didier-wenzek

Approved.

PradeepKiruvale · 2021-08-27T10:53:46Z

mapper/tedge_mapper/src/sm_c8y_mapper/smartrest_deserializer.rs

@@ -157,6 +157,44 @@ impl SmartRestUpdateSoftwareModule {
    }
 }

+type JwtToken = String;
+
+#[derive(Debug, Serialize, Deserialize, PartialEq)]


did not get why do we need serialize, deserialize here?

We don't, we only need deserialize.

…/CIT-514_sw_list_over_http

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

* Add send software list via http * Add tests for c8y structs * Add new tests, change pannicable string building to safe version Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

* [CIT-527] software management doc * update pics * move docs to tutorials * Update summary * fix typos * [CIT-471] Add mapper integration tests (#397) * [CIT-471] Add mapper integration tests Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com> Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com> Co-authored-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com> * [CIT-525] enable sm services (#396) * [CIT-525] extend tedge dis/connect to dis/enable sm services * enable services postinstall * fix minor issues * address review comments * update the println * remove to_do macro * add system tests * add use_agent flag * refactor common code * address review comments * address review comment * Update tedge/src/cli/connect/command.rs * update log Co-authored-by: Pradeep Kumar K J <pradeepkumar.kj@sofwareag.com> Co-authored-by: Sebastian Büttner <sebastian.buettner@softwareag.com> * Also build the tedge_agent debian package on github (#399) * Also build the sm agent package Signed-off-by: Michael Abel <info@abel-ikt.de> * Also build the package for amd64 * tenant-admin hint (#340) Introducing hint to check user rights in cumulocity on HTTP error 503 * [CIT-522] update the mqtt port document (#398) * update the mqtt port document * fix typo * address review comments Co-authored-by: Pradeep Kumar K J <pradeepkumar.kj@sofwareag.com> Co-authored-by: Sebastian Büttner <sebastian.buettner@softwareag.com> * [CIT-514] Add send software list via http (#400) * Add send software list via http * Add tests for c8y structs * Add new tests, change pannicable string building to safe version Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com> * [CIT-542] tedge-agent requires apt plugin as dependency (#406) * [CIT-551] Make the workflow (temporarily) continue even when tarpaulin fails. (#407) Signed-off-by: Michael Abel <info@abel-ikt.de> * [CIT-532] Subscribe to cloud operations only after device creation (#404) * [CIT-532] Subscribe to cloud operations only after device creation * [CIT-551] Make the job pass (temporarily) on job level as well (#408) * Make it continue even when tarpaulin fails Signed-off-by: Michael Abel <info@abel-ikt.de> * [CIT-551] Make the job pass on job level as well * fix a typo * address review comments * Update software-management.md Co-authored-by: Pradeep Kumar K J <pradeepkumar.kj@sofwareag.com> Co-authored-by: Rina Fujino <18257209+rina23q@users.noreply.github.com> Co-authored-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com> Co-authored-by: Sebastian Büttner <sebastian.buettner@softwareag.com> Co-authored-by: Michael Abel <75477722+abelikt@users.noreply.github.com> Co-authored-by: Lukasz Woznicki <75632179+makr11st@users.noreply.github.com> Co-authored-by: Albin Suresh <albin.suresh@softwareag.com>

Lukasz Woznicki added 2 commits August 24, 2021 12:16

Add send software list via http

1e33e2c

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

Add tests for c8y structs

5f72151

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

makr11st force-pushed the feature/CIT-514_sw_list_over_http branch from f0f3759 to 5f72151 Compare August 24, 2021 11:37

Merge branch 'main' of github.com:thin-edge/thin-edge.io into feature…

0d54e80

…/CIT-514_sw_list_over_http

PradeepKiruvale reviewed Aug 24, 2021

View reviewed changes

Remove unwraps

0cf19bf

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

makr11st force-pushed the feature/CIT-514_sw_list_over_http branch 2 times, most recently from c25eb52 to 3e3dc74 Compare August 24, 2021 20:35

Add new tests, change pannicble string building to safe version

c04813a

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

makr11st force-pushed the feature/CIT-514_sw_list_over_http branch from 3e3dc74 to c04813a Compare August 24, 2021 20:36

Move functions to return clean results.

35c6025

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

makr11st requested review from didier-wenzek and PradeepKiruvale August 24, 2021 21:06

makr11st marked this pull request as ready for review August 24, 2021 21:09

Add missing test for serializer.

dd36cf8

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

makr11st force-pushed the feature/CIT-514_sw_list_over_http branch from 1287dcb to dd36cf8 Compare August 24, 2021 21:20

PradeepKiruvale reviewed Aug 25, 2021

View reviewed changes

makr11st changed the title ~~Add send software list via http~~ [CIT-514] Add send software list via http Aug 25, 2021

PradeepKiruvale reviewed Aug 25, 2021

View reviewed changes

mapper/tedge_mapper/src/sm_c8y_mapper/mapper.rs Show resolved Hide resolved

mapper/tedge_mapper/src/sm_c8y_mapper/mapper.rs Show resolved Hide resolved

Lukasz Woznicki added 2 commits August 25, 2021 16:45

Merge branch 'main' of github.com:thin-edge/thin-edge.io into feature…

0883542

…/CIT-514_sw_list_over_http

Remove trait init method

c6de861

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

didier-wenzek reviewed Aug 25, 2021

View reviewed changes

makr11st force-pushed the feature/CIT-514_sw_list_over_http branch 2 times, most recently from f27b78a to bb19293 Compare August 26, 2021 08:12

makr11st requested review from didier-wenzek and PradeepKiruvale August 26, 2021 08:14

Address PR comments: remove redundant imports, move code into if block,

44db750

add try_new to jwt struct, remove from for Topic Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

makr11st force-pushed the feature/CIT-514_sw_list_over_http branch from bb19293 to 44db750 Compare August 26, 2021 11:51

Add retry timer on set internal id

ba4f913

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

makr11st force-pushed the feature/CIT-514_sw_list_over_http branch from c8726fd to ba4f913 Compare August 26, 2021 12:16

didier-wenzek approved these changes Aug 27, 2021

View reviewed changes

PradeepKiruvale approved these changes Aug 27, 2021

View reviewed changes

Lukasz Woznicki added 2 commits August 27, 2021 14:54

Merge branch 'main' of github.com:thin-edge/thin-edge.io into feature…

8de22f4

…/CIT-514_sw_list_over_http

Remove serialize

fda085d

Signed-off-by: Lukasz Woznicki <lukasz.woznicki@softwareag.com>

makr11st force-pushed the feature/CIT-514_sw_list_over_http branch from a058605 to fda085d Compare August 27, 2021 13:58

makr11st merged commit 54acbbe into thin-edge:main Aug 27, 2021

makr11st deleted the feature/CIT-514_sw_list_over_http branch September 30, 2021 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIT-514] Add send software list via http #400

[CIT-514] Add send software list via http #400

makr11st commented Aug 24, 2021

codecov-commenter commented Aug 24, 2021 •

edited

Loading

PradeepKiruvale left a comment

PradeepKiruvale Aug 24, 2021

makr11st Aug 24, 2021

didier-wenzek Aug 25, 2021

makr11st commented Aug 24, 2021

PradeepKiruvale Aug 25, 2021

makr11st Aug 25, 2021

didier-wenzek Aug 25, 2021

didier-wenzek Aug 25, 2021

makr11st Aug 26, 2021

didier-wenzek left a comment

PradeepKiruvale Aug 27, 2021

makr11st Aug 27, 2021

[CIT-514] Add send software list via http #400

[CIT-514] Add send software list via http #400

Conversation

makr11st commented Aug 24, 2021

Proposed changes

Types of changes

Checklist

codecov-commenter commented Aug 24, 2021 • edited Loading

Codecov Report

PradeepKiruvale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makr11st commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 24, 2021 •

edited

Loading