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

fix(#2370): use bridge topic id in service_monitor #2837

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Bravo555
Copy link
Contributor

@Bravo555 Bravo555 commented Apr 23, 2024

Proposed changes

Use the correct bridge topic id when converting health status message.

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

@Bravo555 Bravo555 force-pushed the fix/2370/bridge-health-status branch from d32f40b to 4d83766 Compare April 24, 2024 08:44
Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.3%. Comparing base (6c5f45c) to head (98ccfe7).
Report is 36 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/core/tedge_api/src/mqtt_topics.rs 87.4% <ø> (-0.2%) ⬇️
crates/extensions/c8y_mapper_ext/src/converter.rs 83.3% <100.0%> (ø)
crates/extensions/c8y_mapper_ext/src/lib.rs 88.8% <ø> (ø)
...s/extensions/c8y_mapper_ext/src/service_monitor.rs 96.4% <100.0%> (+<0.1%) ⬆️

... and 26 files with indirect coverage changes

@Bravo555 Bravo555 force-pushed the fix/2370/bridge-health-status branch from 4d83766 to ce6e55e Compare April 30, 2024 08:13
@Bravo555 Bravo555 temporarily deployed to Test Pull Request April 30, 2024 08:13 — with GitHub Actions Inactive
Copy link
Contributor

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
429 0 3 429 100 0s

@Bravo555 Bravo555 marked this pull request as ready for review April 30, 2024 10:51
@Bravo555 Bravo555 marked this pull request as draft April 30, 2024 10:53
@Bravo555 Bravo555 force-pushed the fix/2370/bridge-health-status branch from ce6e55e to 98ccfe7 Compare April 30, 2024 10:58
@Bravo555 Bravo555 temporarily deployed to Test Pull Request April 30, 2024 10:58 — with GitHub Actions Inactive
@Bravo555 Bravo555 marked this pull request as ready for review April 30, 2024 10:58
Comment on lines -31 to +32
if entity.topic_id.is_bridge_health_topic() {
// the bridge itself is not registered as a service, only the mapper
if entity.topic_id == crate::C8Y_BRIDGE_TOPIC_ID {
Copy link
Contributor

@didier-wenzek didier-wenzek Apr 30, 2024

Choose a reason for hiding this comment

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

With this change, the az bridge is not reported as a service on c8y. This could make sense, but is this what we expect?

@@ -14,6 +14,8 @@ pub mod service_monitor;
#[cfg(test)]
mod tests;

pub(crate) const C8Y_BRIDGE_TOPIC_ID: &str = "device/main/service/mosquitto-c8y-bridge";
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't assume this hardcoded device/main topic id even for the main device since we support overriding device_topic_id via the config. That being said, I'm not fully sure if all tedge components properly respect that config setting yet either. I'm fairly sure that it is still hardcoded in many places.

@Bravo555 Bravo555 marked this pull request as draft May 7, 2024 09:06
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.

3 participants