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: plugins don't update their internal representation of config files on file change #2262

Merged

Conversation

Ruadhri17
Copy link
Contributor

@Ruadhri17 Ruadhri17 commented Sep 13, 2023

Proposed changes

Fix the problem of not updating the internal representation of config files. This applies to all plugins that use FsFileWatcher:

  • tedge-log-plugin
  • c8y-log-plugin
  • c8y-configuration-plugin

Additionally this PR fixes unsafe unwrap while sending mqtt message inside the c8y-configuration-plugin (that could lead to rust panic if there was any problem with MQTT actor)

The integration tests were also extended to cover these scenarios to ensure it functions as expected (taken from #2260):

  • add library function to check service status of child devices and switch to new tedge interface
  • use recent timestamp instead of 1970 as this is more aligned to how the UI behaves
  • fix timestamp bug where the tests were using a fixed timezone which meant that the local time was being erroneously cast to UTC leading to unexpected dateFrom/dateTo values
  • use tedge config to control te topic instead of flags
  • add notes about when workarounds in tests can be removed

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

@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request September 13, 2023 20:10 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #2262 (8e1b1c6) into main (07d88c2) will increase coverage by 0.0%.
The diff coverage is 63.6%.

Additional details and impacted files
Files Changed Coverage Δ
crates/extensions/c8y_config_manager/src/actor.rs 58.7% <33.3%> (+0.5%) ⬆️
crates/extensions/c8y_log_manager/src/actor.rs 75.9% <75.0%> (-0.3%) ⬇️
crates/extensions/tedge_log_manager/src/actor.rs 68.9% <75.0%> (-0.8%) ⬇️

... and 4 files with indirect coverage changes

@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
273 0 5 273 100 51m5.924999999s

reubenmiller and others added 7 commits September 14, 2023 12:53
* use recent timestamp instead of 1970
* use tedge config to control te topic instead of flags
* add notes about when workaround can be removed

Signed-off-by: Reuben Miller <[email protected]>
@reubenmiller reubenmiller force-pushed the fix-update-config-on-change-in-plugins branch from d5cb111 to 8e1b1c6 Compare September 14, 2023 02:53
@reubenmiller
Copy link
Contributor

@Ruadhri17 I merged the changes from #2260 as the integration tests were covering this aspect, so we can get confirmation that everything works as expected before merging (I also rebased ontop of the latest from main)

@reubenmiller reubenmiller temporarily deployed to Test Pull Request September 14, 2023 03:07 — with GitHub Actions Inactive
@albinsuresh
Copy link
Contributor

The fix looks good. Happy to approve once my queries are addressed.

@Ruadhri17 Ruadhri17 merged commit ea332aa into thin-edge:main Sep 14, 2023
18 checks passed
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