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.

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

Support software type for c8y advanced software management in 10.14 #2613

Conversation

rina23q
Copy link
Member

@rina23q rina23q commented Jan 22, 2024

Proposed changes

Since the version 10.14, c8y starts supporting "type" field for software list and "softwareType" field for software update operation.

Change the Rust code accordingly:

  1. Software list
    Stop combining version and software type with the connector "::". The module type will be declared as "type" in c8y JSON and the version field holds its pure version only.

  2. Software update
    Different behaviour if a received operation contains "softwareType" field or not. If yes, softwareType will be used as its module type and version will be used as it is. If no, mapper tries to get the module type from the received version field as we initially implemented.

Todo:

  • Rust change
  • Robot test change
  • Doc change

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

#1352

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

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fb82765) 75.8% compared to head (8bbfa2b) 75.9%.
Report is 11 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/core/c8y_api/src/json_c8y.rs 90.5% <100.0%> (-0.4%) ⬇️
crates/core/c8y_api/src/json_c8y_deserializer.rs 95.1% <100.0%> (+0.9%) ⬆️

... and 14 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Jan 22, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
381 0 3 381 100 57m24.674s

@rina23q rina23q force-pushed the improve/1352/supoort-type-field-for-software-management branch from 74733a1 to e82cc3d Compare January 24, 2024 09:58
@reubenmiller reubenmiller marked this pull request as ready for review January 25, 2024 13:53
@rina23q rina23q marked this pull request as draft January 25, 2024 15:38
@rina23q rina23q force-pushed the improve/1352/supoort-type-field-for-software-management branch from e82cc3d to 0aa82d3 Compare January 25, 2024 15:45
@rina23q rina23q force-pushed the improve/1352/supoort-type-field-for-software-management branch from 0aa82d3 to 041f84b Compare January 25, 2024 16:04
@rina23q rina23q force-pushed the improve/1352/supoort-type-field-for-software-management branch from 041f84b to 3126a0f Compare January 26, 2024 07:53
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Doc change (need to check)?

Yes, the doc has to be updated.

The sentence "thin-edge.io ships a default plugin supporting debian packages from both apt repositories as well as remote locations. If you prefer to use packages from an apt repository, select the Provide a file path option and give an empty space (' ')." is really unclear.

I would simply tell that if no version or no path are applicable then a string of empty spaces are accepted by thin-edge meaning that this field is non applicable. We can stress that this is the case for the apt plugin when one wants to install the latest version of the package (.i.e. no version) using the package repository configured on the device (i.e. no path/url).

@reubenmiller does it make sense to accept the string "NA" as well as white-spaces to mean that the field is non applicable?

@rina23q rina23q marked this pull request as ready for review January 28, 2024 11:07
@reubenmiller
Copy link
Contributor

Doc change (need to check)?

Yes, the doc has to be updated.

The sentence "thin-edge.io ships a default plugin supporting debian packages from both apt repositories as well as remote locations. If you prefer to use packages from an apt repository, select the Provide a file path option and give an empty space (' ')." is really unclear.

I would simply tell that if no version or no path are applicable then a string of empty spaces are accepted by thin-edge meaning that this field is non applicable. We can stress that this is the case for the apt plugin when one wants to install the latest version of the package (.i.e. no version) using the package repository configured on the device (i.e. no path/url).

@reubenmiller does it make sense to accept the string "NA" as well as white-spaces to mean that the field is non applicable?

I don't think it is worth it...from an API view, just leaving out the url is fine. I believe the requirement to use whitespace in the url is only a UI form validation issue...so we could put a change request to remove the UI validation to also allow an option to not treat a missing url as an error.

@rina23q rina23q force-pushed the improve/1352/supoort-type-field-for-software-management branch from 0907051 to 83248be Compare January 29, 2024 12:41
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved

Since the version 10.14, c8y starts supporting "type" field for software
list and "softwareType" field for software update operation.

Change the Rust code accordingly:
1) Software list
Stop combining version and software type with the connector "::". Module
type will be declared as "type" and the version field holds its pure
version only.

2) Software update
Different behaviour if a received operation contains "softwareType"
field or not. If yes, softwareType will be used as its module type and
version will be used as it is. If no, mapper tries to get the module
type from the received version field as we initially implemented.

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
@rina23q rina23q force-pushed the improve/1352/supoort-type-field-for-software-management branch from 83248be to 8bbfa2b Compare January 29, 2024 14:28
@reubenmiller reubenmiller added this pull request to the merge queue Jan 29, 2024
Merged via the queue into thin-edge:main with commit 0fbfa4f Jan 29, 2024
20 checks passed
rina23q added a commit to rina23q/thin-edge.io that referenced this pull request Feb 1, 2024
Software type is reported as "type" in the PR thin-edge#2613, however, it should
have been "softwareType" according to c8y's document. Refer to here.
https://cumulocity.com/guides/reference/device-management-library/#software

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
rina23q added a commit to rina23q/thin-edge.io that referenced this pull request Feb 1, 2024
Software type is reported as "type" in the PR thin-edge#2613, however, it should
have been "softwareType" according to c8y's document. Refer to here.
https://cumulocity.com/guides/reference/device-management-library/#software

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants