Cumulocity mapper using actors #1909 #1922
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
albinsuresh
force-pushed
the
imp/1909/refactor-c8y-mapper-with-actors
branch
2 times, most recently
from
9419294
to
c027b33
Compare
albinsuresh
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Robot Results
Passed Tests
|
albinsuresh
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
albinsuresh
force-pushed
the
imp/1909/refactor-c8y-mapper-with-actors
branch
from
81fdb7b
to
31ff3cb
Compare
albinsuresh
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
albinsuresh
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
albinsuresh
force-pushed
the
imp/1909/refactor-c8y-mapper-with-actors
branch
from
cab723b
to
77b9c4e
Compare
albinsuresh
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Comment on lines
+113
to
+115
|
|
||
| #[error(transparent)] | ||
| FromC8YRestError(#[from] C8YRestError), |
There was a problem hiding this comment.
This error is specific to C8Y and should not be added to core::ConversionError.
And, by the way, there is already a case for it in c8y::CumulocityMapperError.
There was a problem hiding this comment.
Still here - but okay the issue being beyond the scope of this PR. The core::ConversionError is already a mix of unrelated errors. This will have to be fix in a follow-up task.
There was a problem hiding this comment.
Oops, missed it. Will do it, as I'll have to resolve this anyway as part of moving the c8y-mapper into extensions/c8y_mapper_ext.
There was a problem hiding this comment.
I ended up moving the Converter trait and ConversionError enum from core to c8y_mapper_ext as no other crate was using it so it didn't make sense to keep them as separate core abstractions.
| @@ -10,8 +10,10 @@ homepage = { workspace = true } | |||
| repository = { workspace = true } | |||
|
|
|||
| [dependencies] | |||
| assert-json-diff = "2.0" | |||
There was a problem hiding this comment.
[follow-up task]
We need to add test-helpers feature flag.
Comment on lines
10
to
11
| //HIPPO Rename EventId to String as there could be many other String responses as well and this macro doesn't allow another String variant | ||
| fan_in_message_type!(C8YRestResponse[EventId, Unit, Bool]: Debug); |
There was a problem hiding this comment.
this macro doesn't allow another String variant
And cannot. If you receive pure Strings from different sources how can you distinguish them to process them appropriately?
fan_in_message_type!(C8YRestResponse[EventId, Unit, Bool]: Debug);
These response types are not really self contained. This is okayish for now as c8y_http_proxy is systematically used with a request-response pattern. But if for some reason we start to send concurrently several requests to the proxy, then we will have to replace these types with specific ones that give the context of the response.
An event id for what? What is true / false?
There was a problem hiding this comment.
If you receive pure Strings from different sources how can you distinguish them to process them appropriately?
I was hoping to create a different alias like type JwtToken = String and use that aliased name as another enum variant. But I guess, both EventId and JwtToken aliases are substituted to String before the macro generates the variants, leading to the conflict. So I can either rename the existing EventId alias to just String so that it can be used everywhere or hand create the C8YRestResponse enum as follows:
enum C8YRestResponse {
EventId(String),
JwtToke(String),
...
albinsuresh
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
albinsuresh
force-pushed
the
imp/1909/refactor-c8y-mapper-with-actors
branch
from
c2da0ab
to
5450cac
Compare
albinsuresh
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
albinsuresh
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
albinsuresh
force-pushed
the
imp/1909/refactor-c8y-mapper-with-actors
branch
from
97bf553
to
1c85072
Compare
albinsuresh
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
rina23q
reviewed
Apr 28, 2023
Comment on lines
-42
to
+44
| SoftwareListRequest::topic_name(), | ||
| "tedge/commands/req/software/list" | ||
| SoftwareListRequest::topic(), | ||
| Topic::new_unchecked("tedge/commands/req/software/list") |
There was a problem hiding this comment.
[unrelated] I really don't see the point of this kind of tests ...
albinsuresh
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
albinsuresh
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
albinsuresh
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
albinsuresh
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
|
There's further cleanup that can be done in the C8y mapper code, but haven't done that in this PR as it is out-of-scope for this refactoring work. But, they'd still be worth doing in follow-up PRs. Here are a few such improvements:
|
albinsuresh
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
didier-wenzek
approved these changes
May 3, 2023
| @@ -2,6 +2,7 @@ pub mod file; | |||
| pub mod fs; | |||
| pub mod paths; | |||
| pub mod signals; | |||
| pub mod size_threshold; | |||
There was a problem hiding this comment.
[Note] This is typically the kind of code I see no point to abstract. This is just a size comparison but adds a coupling to the mqtt_channel crate.
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Instead of moving the c8y mapper into
extensions, I've updated the existing module itself just so that the diff is clear and easy to review. Once the changes are approved, I'll move it toextensions/c8y_mapper_ext.CumulocityConverterextensions/c8y_mapper_ext.Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments