Skip to content

fix: c8y operation fragments for firmware/software update and device profile#3362

Merged
Ruadhri17 merged 4 commits intothin-edge:mainfrom
Ruadhri17:operation-fragments-fix
Feb 18, 2025
Merged

fix: c8y operation fragments for firmware/software update and device profile#3362
Ruadhri17 merged 4 commits intothin-edge:mainfrom
Ruadhri17:operation-fragments-fix

Conversation

@Ruadhri17
Copy link
Copy Markdown
Contributor

@Ruadhri17 Ruadhri17 commented Jan 27, 2025

Proposed changes

This PR adds operation fragments messages for device profile and switch the order of those messages for firmware/software update so that they are sent before the succesful state change.

Additionally, the structure of device profile command was changed as it was not deserializing properly. The remote_url field was changed to String from Option<String> as it is the mandatory field (this does not affect code as it was derived from mandatory value anyway).

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


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

@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from 450a075 to ee5b094 Compare January 27, 2025 09:53
@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from ee5b094 to 845563f Compare January 27, 2025 09:53
@Ruadhri17 Ruadhri17 marked this pull request as draft January 27, 2025 10:13
@reubenmiller reubenmiller changed the title fix c8y operation fragments for firmware/software update and device profile fix: c8y operation fragments for firmware/software update and device profile Jan 27, 2025
@reubenmiller reubenmiller added theme:software Theme: Software management theme:firmware Theme: Firmware update topics theme:profile Device Profile labels Jan 27, 2025
@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from 845563f to 0bc67d4 Compare January 27, 2025 13:07
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request January 27, 2025 13:07 — with GitHub Actions Inactive
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 27, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 99.61240% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...xtensions/c8y_mapper_ext/src/operations/convert.rs 92.30% 1 Missing ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 27, 2025 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
556 0 2 556 100 1h28m59.038874s

@Ruadhri17 Ruadhri17 marked this pull request as ready for review January 27, 2025 15:17
albinsuresh
albinsuresh reviewed Jan 28, 2025
View reviewed changes
@albinsuresh
Copy link
Copy Markdown
Contributor

albinsuresh commented Jan 28, 2025

The Set currently installed configuration SmartREST message (120) is also missing while the device profile is applied.

@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from 0bc67d4 to 9af954b Compare January 29, 2025 16:00
@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from 9af954b to 6f3e142 Compare January 29, 2025 17:39
@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from 6f3e142 to 2ff567b Compare January 29, 2025 17:49
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request January 29, 2025 17:49 — with GitHub Actions Inactive
@albinsuresh
Copy link
Copy Markdown
Contributor

albinsuresh commented Jan 31, 2025

The Set currently installed configuration SmartREST message (120) is also missing while the device profile is applied.

Updated the source ticket #3162 with some findings around the 120 message. Please refer to it before implementing the same.

@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from 4aa42e7 to 28f7360 Compare February 11, 2025 19:27
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request February 11, 2025 19:27 — with GitHub Actions Inactive
albinsuresh
albinsuresh reviewed Feb 13, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Very close, but yet one step away.

As discussed offline, even after a successful application of the device profile, the config entries still appear with a "Not installed on the device" warning, despite sending the 120 message and replacing the proxy url with the tenant URL.

I investigated and understood why this is happening. Even though I trigger the operation from my tenant albin.latest.stage.c8y.io, the url filed in the request payload contains the value https://t37070943.latest.stage.c8y.io/inventory/binaries/4119377 with the actual tenant id t37070943 instead of the alias albin in it. When we send the 120 message, the URL must match what was received in the request. But, even when the proxy URL is converted using the new c8y_url_from_proxy_url function, the host is replaced with what was configured as c8y.url (albin.latest...) and not the one it received in the request. This is the cause of the warning.

To fix this without any disruptive changes (by changing the remoteUrl value itself), I'd suggest that we pass an additional cloud_url field in the config_update request payload with the same value from the c8y request. When the request completes, this same value can be used to send the 120 message. With such a change, we wouldn't need the c8y_url_from_proxy_url conversion anymore.

@Ruadhri17
fix: change device profile command payload structure to deserialize c…
f7bbd25 
…orrectly

Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from 28f7360 to aa5e9d1 Compare February 13, 2025 17:38
@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from aa5e9d1 to 9b73639 Compare February 13, 2025 17:53
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request February 13, 2025 17:53 — with GitHub Actions Inactive
albinsuresh
albinsuresh reviewed Feb 14, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

Though the unit test coverage is good enough, it would be better to update the existing config_update and device_profile system tests to validate the newly added c8y_Firmware and c8y_Configuration_XYZ fragments as well, esp since most of the recent fixes were about minor mistakes in those fragment contents.

@Ruadhri17 Ruadhri17 requested a review from a team as a code owner February 17, 2025 17:21
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request February 17, 2025 17:21 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from c75a678 to 617fb90 Compare February 17, 2025 18:58
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request February 17, 2025 18:58 — with GitHub Actions Inactive
albinsuresh
albinsuresh approved these changes Feb 18, 2025
View reviewed changes
Copy link
Copy Markdown
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. Thank you for the persistent efforts on this PR.

@Ruadhri17
feat: add fragment updates for device profile
361d88a 
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
@Ruadhri17
feat: add fragment for config update
580e897 
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
@Ruadhri17 Ruadhri17 force-pushed the operation-fragments-fix branch from 617fb90 to 580e897 Compare February 18, 2025 22:41
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request February 18, 2025 22:41 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 enabled auto-merge February 18, 2025 22:42
@Ruadhri17 Ruadhri17 added this pull request to the merge queue Feb 18, 2025
Merged via the queue into thin-edge:main with commit 83b65bd Feb 18, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reubenmiller reubenmiller reubenmiller left review comments

@rina23q rina23q rina23q left review comments

@albinsuresh albinsuresh albinsuresh approved these changes

@didier-wenzek didier-wenzek Awaiting requested review from didier-wenzek didier-wenzek is a code owner

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

Assignees

No one assigned

Labels

theme:firmware Theme: Firmware update topics theme:profile Device Profile theme:software Theme: Software management

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Ruadhri17 @albinsuresh @reubenmiller @rina23q