Skip to content

refactor(cli): use CommonArgs for common arguments#3273

Merged
Bravo555 merged 1 commit intothin-edge:mainfrom
Bravo555:improve/log-level
Dec 2, 2024
Merged

refactor(cli): use CommonArgs for common arguments#3273
Bravo555 merged 1 commit intothin-edge:mainfrom
Bravo555:improve/log-level

Conversation

@Bravo555
Copy link
Copy Markdown
Member

@Bravo555 Bravo555 commented Nov 29, 2024

Proposed changes

As suggested in #3271 (comment), since we want both log_args and config_dir be used by all thin-edge components, and because log_args also needs the config_dir to use in log_init(), use a shared CommonArgs in all thin-edge components to parse these arguments.

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

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 28 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/core/tedge_write/src/bin.rs 0.00% 5 Missing ⚠️
plugins/tedge_apt_plugin/src/lib.rs 71.42% 3 Missing and 1 partial ⚠️
crates/core/tedge_mapper/src/lib.rs 0.00% 3 Missing ⚠️
crates/core/tedge_watchdog/src/lib.rs 0.00% 3 Missing ⚠️
crates/core/tedge_watchdog/src/systemd_watchdog.rs 0.00% 3 Missing ⚠️
crates/common/tedge_config/src/cli.rs 0.00% 2 Missing ⚠️
crates/core/tedge/src/main.rs 66.66% 2 Missing ⚠️
crates/core/tedge_agent/src/lib.rs 0.00% 2 Missing ⚠️
plugins/c8y_firmware_plugin/src/lib.rs 0.00% 2 Missing ⚠️
plugins/c8y_remote_access_plugin/src/input.rs 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@Bravo555
cli: use CommonArgs for common arguments
2b52f14 
Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@Bravo555 Bravo555 temporarily deployed to Test Pull Request November 29, 2024 14:01 — with GitHub Actions Inactive
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 29, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
527 0 2 527 100 1h34m3.594657999s

albinsuresh
albinsuresh approved these changes Dec 2, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

LGTM. I'm assuming that the two-step parsing that we discussed where the args to parsed to the CommonArgs first, to enable logging for all components from tedge::main itself, followed by a secondary parsing into component specific arg structs wasn't feasible.

didier-wenzek
didier-wenzek approved these changes Dec 2, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@Bravo555 Bravo555 added this pull request to the merge queue Dec 2, 2024
Merged via the queue into thin-edge:main with commit e614446 Dec 2, 2024
@reubenmiller reubenmiller added theme:cli Theme: cli related topics refactoring Developer value labels Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albinsuresh albinsuresh albinsuresh approved these changes

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

@rina23q rina23q Awaiting requested review from rina23q rina23q is a code owner

Assignees

No one assigned

Labels

refactoring Developer value theme:cli Theme: cli related topics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Bravo555 @albinsuresh @didier-wenzek @reubenmiller