Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix send_event() in c8y_http_proxy to create events under child devices #2421

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

rina23q
Copy link
Member

@rina23q rina23q commented Nov 6, 2023

Proposed changes

c8y-mapper should create an event for a child device via HTTP when the event size exceeds the MQTT threshold. However, it was created for the main device instead. This PR fixes the issue.

The root cause how to get the device_id (= c8y's internal device ID) in this part of the code.

// If the incoming event does not have any source mentioned explicitly,
// it is associated to the main device by default.
let device_id = c8y_event
.source
.map_or_else(|| self.end_point.device_id.clone(), |v| v.id);

c8y_event.source needs to hold an internal ID, however, it's too early to expect to have an internal ID as source. If we have to use the current implementation, one has to get the device's internal ID in the mapper's converter level (= the c8y_event object in the the below code has to have its internal ID as source). Querying an internal ID should be definitely hidden inside c8y_http_proxy API.

let _ = self.http_proxy.send_event(c8y_event).await?;

For this reason, I introduced a new struct CreateEvent to parse device's external ID to c8y_http_proxy API.

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

#2394

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

Copy link
Contributor

github-actions bot commented Nov 6, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
357 0 3 357 100 1h1m8.257999999s

@rina23q rina23q force-pushed the bugfix/2394/fix-send-event-api branch from 809ff44 to 7168155 Compare November 6, 2023 11:28
@rina23q rina23q temporarily deployed to Test Pull Request November 6, 2023 11:41 — with GitHub Actions Inactive
@rina23q rina23q marked this pull request as ready for review November 6, 2023 13:01
@rina23q rina23q temporarily deployed to Test Pull Request November 6, 2023 13:50 — with GitHub Actions Inactive
@rina23q rina23q requested a review from albinsuresh November 6, 2023 14:21
rina23q and others added 3 commits November 7, 2023 16:05
c8y-mapper should create an event for a child device via HTTP when the
event size exceeds the MQTT threshold. However, it was created for
main device instead. This commit fixes the issue.

Signed-off-by: Rina Fujino <[email protected]>
Use combination of yes, head and tr to generate a large string as the commands are executed on the device under `sh` and not `bash` by default (as bash is not already installed)

Signed-off-by: Reuben Miller <[email protected]>
@rina23q rina23q force-pushed the bugfix/2394/fix-send-event-api branch from 86e3d83 to 300c14b Compare November 7, 2023 15:06
@rina23q rina23q temporarily deployed to Test Pull Request November 7, 2023 15:12 — with GitHub Actions Inactive
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #2421 (300c14b) into main (9dea122) will increase coverage by 0.0%.
Report is 12 commits behind head on main.
The diff coverage is 82.9%.

Additional details and impacted files
Files Coverage Δ
crates/extensions/c8y_http_proxy/src/handle.rs 73.3% <100.0%> (ø)
crates/extensions/c8y_http_proxy/src/tests.rs 91.7% <100.0%> (-0.1%) ⬇️
crates/extensions/c8y_http_proxy/src/actor.rs 49.4% <75.0%> (-0.5%) ⬇️
crates/extensions/c8y_http_proxy/src/messages.rs 26.6% <0.0%> (-2.0%) ⬇️
crates/extensions/c8y_mapper_ext/src/converter.rs 81.0% <83.8%> (+0.6%) ⬆️

... and 6 files with indirect coverage changes

Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rina23q rina23q merged commit 4fcec78 into thin-edge:main Nov 8, 2023
@rina23q rina23q deleted the bugfix/2394/fix-send-event-api branch November 8, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants