Entity store: change constructors to remove redundant clones and rename fields #2176
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
Bravo555
changed the title
Bravo555
changed the title
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Robot Results
|
Comment on lines
15
to
22
| /// Represents an "MQTT entity identifier" portion of the MQTT topic | ||
| /// Represents an "Entity MQTT address" portion of the MQTT topic | ||
| /// | ||
| /// Example: | ||
| /// - topic: `te/device/dev1/service/myservice/m//my_measurement | ||
| /// - topic: `te/device/dev1/service/myservice/m//my_measurement` | ||
| /// - entity id: `device/dev1/service/myservice` | ||
| /// | ||
| /// <https://thin-edge.github.io/thin-edge.io/next/references/mqtt-api/#group-identifier> | ||
| type EntityId = String; | ||
| type EntityMqttAddress = String; |
There was a problem hiding this comment.
@Bravo555, @reubenmiller, @albinsuresh
I'm not fully happy with this proposal for a new name:
Pros:
EntityIdis definitely confusing, knowing that device id is already used with a different meaning: (the main device id is the CN of its certificate while its entity identifier isdevice/main//.
Cons:
- The specifications uses the term entity identifier.
- The address suffix of
EntityMqttAddressmight be confused with some kind of network address. - This identifier is used only for MQTT topics. Hence, I would expect the term topic rather than address
What about EntityTopicId or EntityMqttTopicId ?
Alternative
Another way to solve the issue is t:
- keep the terms
EntityIdand entity identifier, when speaking about topics. - use the terms
EntityNameand entity name, when speaking about the CommonName (CN) of these devices.
Important
In any case, the terms used in the code base must be the same as those used in the docs.
There was a problem hiding this comment.
We agreed to use the name EntityTopicId for entity identifiers in the new topic scheme.
Comment on lines
167
to
170
|
|
||
| let external_id = message | ||
| .external_id | ||
| .unwrap_or_else(|| self.derive_external_id(&message.mqtt_id)); | ||
| let entity_id = message | ||
| .entity_id | ||
| .unwrap_or_else(|| self.derive_entity_id(&message.mqtt_id)); |
There was a problem hiding this comment.
This change is really good one, as external id has a specific meaning in the context of c8y.
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Codecov Report
Additional details and impacted files
|
Because we need to clone anyway while constructing `EntityTopic` and `EntityRegistrationTopic`, the `TryFrom` impls were changed to convert from references instead of values. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Because the name `external_id` was not used anywhere else to refer to the identifiers of devices and services on the cloud, it was renamed in favour of `entity_id`, which covers device ids and service ids. `mqtt_id` was renamed to `topic_id`. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
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
EntityTopicandEntityRegistrationMessageTryIntoimpls to take references instead of values, removingneedless clones
external_idtoentity_id(it was already used in thin-edge documentation as device id or service id) andmqtt_idtotopic_idTypes of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments