fix: make device profile skip field optional and skip operation fragment when it is set to true #3417
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ Additional details and impacted files📢 Thoughts on this report? Let us know! |
Robot Results
|
didier-wenzek
left a comment
There was a problem hiding this comment.
minor: The docs still use the keyword skip instead of @skip in one place (The "skip" field is optional and the value is false, by default.)
| }, | ||
| { | ||
| "operation": "config_update", | ||
| "@skip": true, |
There was a problem hiding this comment.
I don't get how this test checks that the config update is skipped, but software & firmware update applied.
There was a problem hiding this comment.
If config update has not been skipped, operation fragment message would be sent:
// Expect `120` smartrest message on `c8y/s/us`.
assert_received_contains_str(
&mut mqtt,
[(
"c8y/s/us",
"120,path/config/test-config,http://www.my.url,test-config",
)],
)
So it not about checking if config update was skipped (that is already covered by one of our system tests), but more about if we skip fragment message when it is marked as skipped.
crates/extensions/c8y_mapper_ext/src/operations/handlers/device_profile.rs
Outdated
Show resolved
Hide resolved
| pub struct DeviceProfileOperation { | ||
| #[serde(flatten)] | ||
| pub operation: OperationPayload, | ||
| #[serde(default, rename = "@skip")] |
There was a problem hiding this comment.
I don't think we can do that now as this has been part of a public API, esp a mandatory field. So, if we change it now, it might affect existing users.
There was a problem hiding this comment.
Scratch that. It is indeed @skip that the agent supports:
| &config.config_type, | ||
| &config.server_url.unwrap_or_default(), | ||
| Some(&config.name), | ||
| if !device_profile_operation.skip { |
There was a problem hiding this comment.
This change is the source of confusion. The actual skip functionality of device profile is not this, but implemented by the agent here:
This change only makes sure that the mapper, while processing the successful response, also ignores the operations that are marked to be skipped. But i reality this will never happen as the mapper will never send a request to the agent with an operation marked as skipped, since C8Y doesn't support a skip field. So, all operations received in the c8y_DeviceProfile payload are mapped to tedge operations with @skip field as false. So, it's not going to get a response with an operation skipped either, unless the agent does something weird.
827ea0b to
9f102ec
Compare
|
@Ruadhri17 I'd recommend rebasing as there has been a lot of system test improvements to fix flaky tests (e.g. the two failing tests on the last run have been fixed). |
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
9f102ec to
bd5b275
Compare
Proposed changes
This PR fixes an issue where the
skipfield in the device profile was treated as mandatory by serde. Now, if the field is omitted, it defaults tofalse. Additionally, the field has been renamed to@skipso that it is properly recognized by the workflow and aligns with our documentation. Finally, this PR addresses a problem where the mapper did not verify whether an operation fragment sent to the cloud was marked as skipped.Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments