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.

Sign up for GitHub

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

Jump to bottom

C8y Mapper consumes Uploader Actor #2361

Merged
merged 7 commits into from
Nov 9, 2023
Merged

C8y Mapper consumes Uploader Actor #2361

merged 7 commits into from
Nov 9, 2023

Conversation

rina23q
Copy link
Member

@rina23q rina23q commented Oct 22, 2023 •
edited
Loading

Proposed changes

This PR adds Uploader Actor to c8y-mapper and makes it consumed by log_upload and config_snapshot operations. It is a refactoring, so all existing tests must pass.

Todo:

Follow-up:

  • Update Uploader Actor to detect content-type per file content. (if UTF8, use text/plain, else, applications/octet-stream)

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

#2345

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

@rina23q rina23q force-pushed the improve/2345/c8y-mapper-uses-uploader-actor branch 2 times, most recently from b51c68a to 5c106e2 Compare October 22, 2023 21:13
@rina23q rina23q temporarily deployed to Test Pull Request October 22, 2023 21:24 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2023 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
358 0 3 358 100 58m10.249s

@rina23q rina23q force-pushed the improve/2345/c8y-mapper-uses-uploader-actor branch from 83c5baa to 64fd16e Compare October 29, 2023 15:42
@codecov
Copy link

codecov bot commented Oct 29, 2023 •
edited
Loading

Codecov Report

Merging #2361 (901d4a4) into main (731b47e) will increase coverage by 0.1%.
Report is 1 commits behind head on main.
The diff coverage is 83.2%.

Additional details and impacted files
Files Coverage Δ
crates/core/c8y_api/src/http_proxy.rs 75.1% <100.0%> (+5.6%) ⬆️
crates/core/c8y_api/src/json_c8y.rs 89.8% <ø> (+2.8%) ⬆️
crates/extensions/c8y_mapper_ext/src/converter.rs 81.1% <100.0%> (+<0.1%) ⬆️
crates/extensions/tedge_uploader_ext/src/actor.rs 83.8% <100.0%> (+2.7%) ⬆️
crates/common/upload/src/upload.rs 92.0% <86.6%> (-0.4%) ⬇️
crates/core/tedge_mapper/src/c8y/mapper.rs 0.0% <0.0%> (ø)
crates/extensions/c8y_mapper_ext/src/actor.rs 76.7% <79.2%> (+0.5%) ⬆️
...extensions/c8y_mapper_ext/src/config_operations.rs 88.1% <79.3%> (+0.6%) ⬆️
crates/extensions/c8y_mapper_ext/src/log_upload.rs 90.4% <92.6%> (+10.4%) ⬆️
crates/extensions/c8y_mapper_ext/src/tests.rs 91.6% <14.5%> (-0.2%) ⬇️

... and 3 files with indirect coverage changes

@rina23q rina23q force-pushed the improve/2345/c8y-mapper-uses-uploader-actor branch from 64fd16e to a429a0b Compare October 29, 2023 16:17
@rina23q rina23q marked this pull request as ready for review October 29, 2023 16:55
@rina23q rina23q force-pushed the improve/2345/c8y-mapper-uses-uploader-actor branch from 1c8c06f to 86ff5a7 Compare October 30, 2023 15:50
albinsuresh
albinsuresh reviewed Nov 3, 2023
View reviewed 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.

The main changes LGTM.

crates/common/upload/src/upload.rs Show resolved Hide resolved
crates/core/c8y_api/src/json_c8y.rs Outdated Show resolved Hide resolved
crates/core/c8y_api/src/json_c8y.rs Outdated Show resolved Hide resolved
crates/core/c8y_api/src/json_c8y.rs Outdated Show resolved Hide resolved
crates/extensions/c8y_mapper_ext/src/tests.rs Show resolved Hide resolved
crates/extensions/c8y_mapper_ext/src/tests.rs Outdated Show resolved Hide resolved
crates/extensions/c8y_mapper_ext/src/converter.rs Outdated Show resolved Hide resolved
crates/extensions/c8y_mapper_ext/src/log_upload.rs Show resolved Hide resolved
crates/extensions/c8y_mapper_ext/src/actor.rs Outdated Show resolved Hide resolved
crates/extensions/c8y_mapper_ext/src/actor.rs Outdated Show resolved Hide resolved
@rina23q rina23q force-pushed the improve/2345/c8y-mapper-uses-uploader-actor branch from 1546cdb to 9a06cc4 Compare November 7, 2023 22:03
@rina23q
Add uploader actor to c8y-mapper
22ed865 
Signed-off-by: Rina Fujino <[email protected]>
@rina23q
Add utf8_pathbuf and utf8_path methods to TempTedgeFile
ffda4d7 
Signed-off-by: Rina Fujino <[email protected]>
@rina23q
Apply the uploader actor to the log_upload feature of c8y-mapper
350c003 
Uploading a file to cloud without an additional uploader actor may block
the whole c8y-mapper processing if it takes too long.

Fix the potential problem by introducing uploader actor to c8y-mapper

Signed-off-by: Rina Fujino <[email protected]>
@rina23q
Make content-type configurable for uploader
4c7661a 
Logfile request should post the file with content-type "text/plain",
otherwise, c8y doesn't show the preview of the content in event.

By default, uploader takes "application/octet-stream" as content-type
(no change from previous behavior). If other type is required, it can be
changed by calling the "with_content_type()" method.

Signed-off-by: Rina Fujino <[email protected]>
@rina23q
Apply the uploader actor to the config_snapshot feature of c8y-mapper
cf38be4 
Signed-off-by: Rina Fujino <[email protected]>
@rina23q rina23q force-pushed the improve/2345/c8y-mapper-uses-uploader-actor branch from 9a06cc4 to cdddc5f Compare November 8, 2023 14:48
@rina23q rina23q temporarily deployed to Test Pull Request November 8, 2023 14:55 — with GitHub Actions Inactive
albinsuresh
albinsuresh approved these changes Nov 9, 2023
View reviewed 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

crates/extensions/c8y_mapper_ext/src/config_operations.rs Outdated Show resolved Hide resolved
crates/extensions/c8y_mapper_ext/src/tests.rs Outdated Show resolved Hide resolved
@rina23q rina23q force-pushed the improve/2345/c8y-mapper-uses-uploader-actor branch from cdddc5f to 901d4a4 Compare November 9, 2023 12:07
@rina23q rina23q temporarily deployed to Test Pull Request November 9, 2023 12:14 — with GitHub Actions Inactive
@rina23q rina23q merged commit 4f4f6b8 into thin-edge:main Nov 9, 2023
@rina23q rina23q deleted the improve/2345/c8y-mapper-uses-uploader-actor branch November 9, 2023 13:06
@rina23q rina23q mentioned this pull request Nov 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albinsuresh albinsuresh albinsuresh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rina23q @albinsuresh