Skip to content

feat: add cloud profile support to tedge-mapper#3174

Merged
jarhodes314 merged 9 commits intothin-edge:mainfrom
jarhodes314:feat/multi-c8y-mappers
Oct 16, 2024
Merged

feat: add cloud profile support to tedge-mapper#3174
jarhodes314 merged 9 commits intothin-edge:mainfrom
jarhodes314:feat/multi-c8y-mappers

Conversation

@jarhodes314
Copy link
Copy Markdown
Contributor

@jarhodes314 jarhodes314 commented Oct 8, 2024

Proposed changes

This adds support to tedge-mapper c8y for different Cumulocity profiles, using the #[tedge_config(multi)] attribute added in #3126. The profile can be selected using either the --profile argument, or by setting the C8Y_PROFILE environment variable. Once it is configured with a profile, tedge-mapper c8y will set the C8Y_PROFILE variable for itself, ensuring that any child processes from custom operations etc. will use that c8y profile.

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

@jarhodes314 jarhodes314 added the theme:c8y Theme: Cumulocity related topics label Oct 8, 2024
@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request October 8, 2024 10:53 — with GitHub Actions Inactive
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 8, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 27.63158% with 330 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/core/tedge/src/cli/connect/command.rs 0.00% 63 Missing ⚠️
crates/core/tedge/src/cli/refresh_bridges.rs 0.00% 25 Missing ⚠️
crates/core/tedge_mapper/src/lib.rs 0.00% 23 Missing ⚠️
plugins/c8y_remote_access_plugin/src/lib.rs 0.00% 23 Missing ⚠️
crates/extensions/c8y_mapper_ext/src/config.rs 0.00% 20 Missing ⚠️
crates/common/tedge_config_macros/src/multi.rs 81.73% 17 Missing and 2 partials ⚠️
.../tedge_config/src/tedge_config_cli/tedge_config.rs 53.84% 12 Missing and 6 partials ⚠️
crates/core/tedge_mapper/src/c8y/mapper.rs 0.00% 16 Missing ⚠️
crates/extensions/c8y_http_proxy/src/lib.rs 0.00% 14 Missing ⚠️
crates/core/c8y_api/src/http_proxy.rs 0.00% 12 Missing ⚠️
... and 15 more
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 8, 2024 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
508 0 2 508 100 1h38m37.064419s

didier-wenzek
didier-wenzek reviewed Oct 8, 2024
View reviewed changes
Copy link
Copy Markdown
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.

What has been updated is correct, but the system tests highlight a missing fix for health checks. I will be happy to approve when fixed.

@jarhodes314 jarhodes314 temporarily deployed to Test Pull Request October 9, 2024 14:46 — with GitHub Actions Inactive
@reubenmiller reubenmiller changed the title feat: add c8y-profile support to tedge-mapper feat: add profile support to tedge-mapper Oct 10, 2024
@jarhodes314 jarhodes314 force-pushed the feat/multi-c8y-mappers branch from acf227b to 1c5c8d1 Compare October 10, 2024 10:14
@jarhodes314
Copy link
Copy Markdown
Contributor Author

jarhodes314 commented Oct 10, 2024

So I've now understood the healthcheck issue. The problem is that in the system test, c8y.availability.interval is set, but no other c8y keys are set. At the moment, this is erroneously causing the c8y config field to be deserialised as a named Cumulocity profile with the unknown property c8y.interval set, rather than as an unnamed Cumulocity with c8y.availability.interval set.

When we then run tedge config set c8y.url, this reads the config, decides that c8y is set to the default value and sets c8y.url before saving tedge.toml. Since the availability interval is not encoded in the DTO due to the incorrect deserialisation, this value is simply discarded.

So I'll try and come up with a resolution for this broken deserialisation logic. I think this issue also highlights an issue with the availability tests. We don't currently verify the requiredInterval property is actually set to the correct value for the device, and this made debugging the issue tricky (admittedly mostly due to my lack of familiarity with the feature not causing me to investigate that property sooner).

@jarhodes314
Copy link
Copy Markdown
Contributor Author

jarhodes314 commented Oct 10, 2024 •
edited
Loading

So I've now understood the healthcheck issue. The problem is that in the system test, c8y.availability.interval is set, but no other c8y keys are set. At the moment, this is erroneously causing the c8y config field to be deserialised as a named Cumulocity profile with the unknown property c8y.interval set, rather than as an unnamed Cumulocity with c8y.availability.interval set.

I can think of two possible solutions to this issue:

  1. Add #[serde(deny_unknown_fields)] to TEdgeConfigDtoC8y, TEdgeConfigDtoAz, etc. This would prevent us from coping with certain deprecated configurations, although we have automatically updated the relevant tedge.toml files since 0.12, although the toml should be preserved just fine for someone to manually fix it.
  2. Give profile names some distinguishing feature (such as a prefix of @), so it is completely unambiguous whether we are attempting to use profiles are not.
  3. Change the enum tagging in MultiDto so that profiled representations are externally tagged.

Examples of what the keys look like in these cases:

  1. c8y.cloud.url, c8y.cloud.availability.interval (unchanged from right now)
  2. c8y.@cloud.url, c8y.@cloud.availability.interval (@ is just a suggestion, anything that isn't a valid rust identifier should work just fine for this method
  3. c8y.profiles.cloud.url, c8y.profiles.cloud.availability.interval

As far as error handling is concerned (specifically in the case of deserialising a toml file, this shouldn't affect the tedge config CLI), I think both of 2 & 3 are easily compatible with the behaviour we have currently, warning on an unrecognised key rather than having a hard error, and reading pre 0.12 keys just fine. I'll do some more experiments tomorrow to try and more concretely prove this is the case. With option 1, the error messages are a lot less helpful, something along the lines of data did not match any variant of untagged enum MultiDto.

Thoughts?

@didier-wenzek
Copy link
Copy Markdown
Contributor

didier-wenzek commented Oct 10, 2024

The problem is that in the system test, c8y.availability.interval is set, but no other c8y keys are set. At the moment, this is erroneously causing the c8y config field to be deserialised as a named Cumulocity profile with the unknown property c8y.interval set, rather than as an unnamed Cumulocity with c8y.availability.interval set.

We really have to fix that. Such user errors are not so unlikely and would surely cause confusion and time waste..

I have clear preference for the second proposal:

  1. Give profile names some distinguishing feature (such as a prefix of @), so it is completely unambiguous whether we are attempting to use profiles are not.

This is simple and effective.

@jarhodes314 jarhodes314 requested review from a team and gligorisaev as code owners October 11, 2024 10:43
rina23q
rina23q reviewed Oct 11, 2024
View reviewed changes
rina23q
rina23q reviewed Oct 11, 2024
View reviewed changes
didier-wenzek
didier-wenzek approved these changes Oct 14, 2024
View reviewed changes
Copy link
Copy Markdown
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. Thank you for this sustained effort.

jarhodes314
jarhodes314 commented Oct 14, 2024
View reviewed changes
jarhodes314 and others added 8 commits October 15, 2024 12:37
@jarhodes314
feat: add c8y-profile support to tedge-mapper
40e7e67 
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
@jarhodes314
Add profile support for AWS and Azure
503578a 
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
@jarhodes314
Assert we set requiredInterval correctly
05b1166 
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
@jarhodes314 @rina23q
Update tests/RobotFramework/tests/cumulocity/availability/heartbeat.r…
d2eb278 
…obot

Co-authored-by: Rina Fujino <rina.fujino.23@gmail.com>
@jarhodes314 @rina23q
Update tests/RobotFramework/tests/cumulocity/availability/heartbeat.r…
9bbb342 
…obot

Co-authored-by: Rina Fujino <rina.fujino.23@gmail.com>
@jarhodes314
Require prefixed profile names and tidy up remaining hardcoded mosqui…
9a594e9 
…tto config paths

Signed-off-by: James Rhodes <jarhodes314@gmail.com>
@jarhodes314
Run formatter
1ba93ff
@jarhodes314
Hide the profile argument
d362cc0
@jarhodes314 jarhodes314 force-pushed the feat/multi-c8y-mappers branch from 2ba64b9 to d362cc0 Compare October 15, 2024 11:37
@jarhodes314
Remove dead code
6d79294 
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
@jarhodes314 jarhodes314 force-pushed the feat/multi-c8y-mappers branch from baea6f7 to 6d79294 Compare October 16, 2024 09:21
@jarhodes314 jarhodes314 enabled auto-merge October 16, 2024 09:22
@jarhodes314 jarhodes314 added this pull request to the merge queue Oct 16, 2024
Merged via the queue into thin-edge:main with commit 09c1d4c Oct 16, 2024
@jarhodes314 jarhodes314 deleted the feat/multi-c8y-mappers branch October 16, 2024 10:59
@jarhodes314 jarhodes314 mentioned this pull request Oct 24, 2024
Closed
@reubenmiller reubenmiller changed the title feat: add profile support to tedge-mapper feat: add cloud profile support to tedge-mapper Dec 16, 2024
@reubenmiller reubenmiller added the skip-release-notes Don't include the ticket in the auto generated release notes label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rina23q rina23q rina23q left review comments

@didier-wenzek didier-wenzek didier-wenzek approved these changes

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

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

@gligorisaev gligorisaev Awaiting requested review from gligorisaev

Assignees

No one assigned

Labels

skip-release-notes Don't include the ticket in the auto generated release notes theme:c8y Theme: Cumulocity related topics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jarhodes314 @didier-wenzek @rina23q @reubenmiller