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

Default configuration for tedge operation plugins #2637

Merged

Conversation

albinsuresh
Copy link
Contributor

@albinsuresh albinsuresh commented Jan 31, 2024

Proposed changes

Provide default plugin configurations for tedge-config-plugin and tedge-log-plugin that'll work with both main and child devices.

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

#2447

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

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (9dbd1ad) 75.8% compared to head (6bb5198) 75.8%.
Report is 72 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/extensions/tedge_config_manager/src/lib.rs 87.7% <94.7%> (+20.4%) ⬆️
...rates/extensions/tedge_config_manager/src/tests.rs 89.9% <96.1%> (+0.5%) ⬆️
crates/extensions/tedge_log_manager/src/lib.rs 83.7% <87.5%> (+19.1%) ⬆️
crates/extensions/tedge_log_manager/src/tests.rs 89.0% <93.7%> (+0.2%) ⬆️
plugins/tedge_log_plugin/src/lib.rs 0.0% <0.0%> (ø)
crates/extensions/tedge_log_manager/src/config.rs 0.0% <0.0%> (ø)
crates/core/tedge_agent/src/agent.rs 0.0% <0.0%> (ø)

... and 6 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Jan 31, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
387 0 3 387 100 48m14.552999999s

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.

Approved

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.

I confirm my approval, after the changes to generate config examples using the config directory set with --config-dir.

"{}/plugins/tedge-log-plugin.toml",
tempdir.path().to_string_lossy()
);
let expected_config = toml::toml! {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this test is helpful: the expected_config is built using exactly the same code as by the unit under test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this test just to make sure that any future changes to the default config are also always tested. Since the paths used are dynamic (derived from the temp directory), I had no choice but to use a similar mechanism to generate the expected configs as well.

let plugin_config_toml: Table = from_str(&plugin_config_content).unwrap();

let agent_logs_path = format!("{}/agent/software-*", tempdir.path().to_string_lossy());
let expected_config = toml! {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test helpful? The expected and actual values are built using a copy of the same code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as above.

@albinsuresh albinsuresh force-pushed the imp/2447/default-configs-tedge-plugins branch from bacce8b to 6bb5198 Compare February 1, 2024 13:53
@albinsuresh albinsuresh added this pull request to the merge queue Feb 1, 2024
Merged via the queue into thin-edge:main with commit 3296707 Feb 1, 2024
20 checks passed
@albinsuresh albinsuresh deleted the imp/2447/default-configs-tedge-plugins branch February 2, 2024 09:36
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.

None yet

3 participants