fix: Ignore extra HealthStatus fields in service monitor (maintain single HealthStatus)#3133
Closed
Bravo555 wants to merge 2 commits intothin-edge:mainfrom
Closed
Conversation
Features were added because without them it was impossible to build and check `tedge-watchdog` by itself using a `-p` flag, i.e: `cargo build -p tedge-watchdog` (`hide_env_values` requires `env` and `default_value` variant that uses `OsString` required `string`) Note that adding these features in workspace Cargo.toml should not add more code to the binary as these features were already used because other crate required them, so the only clap instance we're pulling have these enabled. Only now we're making these features available to all the workspace crates (including tedge-watchdog). Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
…al values Allows deserialization of HealthStatus to succeed if errors happen on some fields (e.g. PID is not a number). This is achieved by the `NoneIfErr` deserialization wrapper that ignores errors and puts in `None` instead. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
11 tasks
Codecov ReportAll modified and coverable lines are covered by tests ✅ Additional details and impacted files📢 Thoughts on this report? Let us know! |
Contributor
didier-wenzek
left a comment
There was a problem hiding this comment.
Thank you for this alternative implementation for #3132.
Pros (of this proposal):
- A single
HealthStatusstruct - More natural if the watchdog is fixed to use the
statusfield.
Cons:
- More complex, even if simpler than what you were expecting.
- It's really not obvious that the mapper uses only the
statusfield and the watchdog thepidandtimefields.
For the latter point, I finally prefer the first proposal i.e. #3132.
@Bravo555 What's your point of view?
Contributor
Robot Results
|
Member
Author
If the team prefers the first proposal we can go with it, there's not much code built on top so we're free to change it later if it's no longer fit for purpose. As for addressing the |
Contributor
I'm okay first the first proposal. Approving it. |
HealthStatus)HealthStatus)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
This is an alternative fix to #3132. It contains the same test, but differs in the approach to how we should deserialize
HealthStatus.In #3132, we use separate structs in
tedge_apiandtedge_watchdog. This allows each component to deserialize only mapper and watchdog to deserialize only fields they require, but could be argued duplicates structs and makes it harder to see what are possible fields for this type of message because they're spread out.In this PR, an alternative approach is taken where we use a single
tedge_api::HealthStatusstruct which just fills in a fallbackNoneon deserialization errors for other fields. Here we keep all the fields in a single place but ignore errors, which is not desirable. I thought it's going to be more complex but it turned out simpler than I anticipated.Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments