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

refactor: Refactor health status code #2919

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

albinsuresh
Copy link
Contributor

@albinsuresh albinsuresh commented Jun 5, 2024

Proposed changes

Refactor health status refactoring code, including addressing review comments from #2903

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

Copy link
Contributor

github-actions bot commented Jun 5, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
452 0 3 452 100 1h6m55.904892999s

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.

This PR is a nice step forward, grouping all health-check related logic in one place . However, some constants have still to be made private, in order to avoid inappropriate uses.

crates/core/tedge_api/src/health.rs Outdated Show resolved Hide resolved
crates/core/tedge_api/src/health.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jarhodes314 jarhodes314 left a comment

Choose a reason for hiding this comment

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

I'm happy with the changes asides from the aforementioned pub const issues

crates/extensions/c8y_mapper_ext/src/config.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 79.73856% with 31 lines in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (f186b3f) to head (4914cff).
Report is 3 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/core/c8y_api/src/utils.rs 0.0% <ø> (-69.0%) ⬇️
crates/core/tedge_api/src/health.rs 95.8% <100.0%> (+3.1%) ⬆️
crates/core/tedge_api/src/mqtt_topics.rs 88.1% <ø> (+1.1%) ⬆️
crates/extensions/c8y_mapper_ext/src/actor.rs 82.3% <100.0%> (ø)
crates/extensions/c8y_mapper_ext/src/converter.rs 83.7% <100.0%> (-0.1%) ⬇️
...s/extensions/c8y_mapper_ext/src/service_monitor.rs 97.7% <100.0%> (ø)
crates/extensions/c8y_mapper_ext/src/tests.rs 92.6% <100.0%> (-0.1%) ⬇️
crates/extensions/tedge_mqtt_bridge/src/lib.rs 89.1% <100.0%> (ø)
crates/extensions/c8y_mapper_ext/src/config.rs 48.2% <87.5%> (+0.5%) ⬆️
crates/core/tedge_mapper/src/c8y/mapper.rs 0.0% <0.0%> (ø)
... and 3 more

... and 3 files with indirect coverage changes

mqtt_schema: &MqttSchema,
bridge_service_topic: &Topic,
) -> Result<Self, HealthTopicError> {
if let Ok((topic_id, Channel::Health)) = mqtt_schema.entity_channel_of(&message.topic) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR, but this highlights an issue I got several time: this method uses a specific mqtt_schema only to parse a topic. The MQTT root prefix doesn't really matter and it would be more convenient to have a parse method returning all the component of a topic is set (i.e. not only the topic_id and channel but also the root prefix).

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.

This PR is a nice step forward, grouping all health-check related logic in one place .

@albinsuresh albinsuresh added this pull request to the merge queue Jun 7, 2024
Merged via the queue into thin-edge:main with commit 4861be8 Jun 7, 2024
33 checks passed
@albinsuresh albinsuresh deleted the imp/cleanup-health-check branch June 14, 2024 05:24
@reubenmiller reubenmiller added the theme:monitoring Theme: Service monitoring and watchdogs label Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:monitoring Theme: Service monitoring and watchdogs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants