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

Deprecate old TEdgeConfig #2143

Merged

Conversation

didier-wenzek
Copy link
Contributor

@didier-wenzek didier-wenzek commented Aug 10, 2023

Proposed changes

  • Deprecate old TEdgeConfig::mqtt_config
  • Remove remaining use of the old TEdgeConfig
  • Remove all the settings definitions
  • Remove old TEdgeConfig
  • Rename new::TEdgeConfig as TEdgeConfig

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

#2019

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

@didier-wenzek didier-wenzek temporarily deployed to Test Pull Request August 10, 2023 13:44 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
251 0 5 251 100

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #2143 (9b9c474) into main (339d536) will increase coverage by 1.0%.
Report is 4 commits behind head on main.
The diff coverage is 84.4%.

❗ Current head 9b9c474 differs from pull request most recent head 3fc6e72. Consider uploading reports for the commit 3fc6e72 to get more accurate results

Additional details and impacted files
Files Changed Coverage Δ
crates/bin/c8y-device-management/src/main.rs 0.0% <0.0%> (ø)
crates/common/tedge_config/src/lib.rs 0.0% <0.0%> (ø)
crates/common/tedge_config/src/mqtt_config.rs 0.0% <ø> (ø)
...edge_config/src/tedge_config_cli/config_setting.rs 0.0% <ø> (-36.4%) ⬇️
crates/core/tedge/src/cli/certificate/cli.rs 57.1% <0.0%> (ø)
crates/core/tedge/src/cli/config/cli.rs 76.6% <0.0%> (ø)
crates/core/tedge/src/cli/config/commands/list.rs 92.2% <ø> (ø)
crates/core/tedge/src/cli/connect/command.rs 0.0% <0.0%> (ø)
crates/core/tedge/src/cli/connect/jwt_token.rs 51.2% <ø> (ø)
crates/core/tedge/src/cli/init.rs 0.0% <0.0%> (ø)
... and 39 more

... and 12 files with indirect coverage changes

pub use self::tedge_config_cli::tedge_config::*;
pub use self::tedge_config_cli::tedge_config_defaults::*;
pub use self::tedge_config_cli::tedge_config_location::*;
pub use self::tedge_config_cli::tedge_config_repository::*;
pub use camino::Utf8Path as Path;
pub use camino::Utf8PathBuf as PathBuf;
pub use certificate::CertificateError;

/// loads the new tedge config from system default
pub fn get_new_tedge_config() -> Result<TEdgeConfig, TEdgeConfigError> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be deprecated as using a default location.

This is used in a single place: crates/core/tedge/src/cli/certificate/upload.rs.

I will fix that in a follow-up PR.

Comment on lines 14 to 15
pub struct TEdgeConfigRepository {
config_location: TEdgeConfigLocation,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no more points to have a struct TEdgeConfigRepository as this is now just a TEdgeConfigLocation with a load() method.

I will fix that in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given how many times the load() method is called (24 times in 23 files), we probably have multiple copies of TEdgeConfig per process. We could lazily initialize it and only return a reference to the single instance. The struct is not very large, so it's not a big problem, but perhaps this will be easier to do sooner rather than later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given how many times the load() method is called (24 times in 23 files), we probably have multiple copies of TEdgeConfig per process. We could lazily initialize it and only return a reference to the single instance. The struct is not very large, so it's not a big problem, but perhaps this will be easier to do sooner rather than later.

Added to #2153

@reubenmiller
Copy link
Contributor

@didier-wenzek you'll have to rebase from main to fix the workflow failures (due to the changes introduced in #2138)

@didier-wenzek didier-wenzek force-pushed the refactor/deprecate-old-tedge-config branch from 730faa7 to 8fd2026 Compare August 10, 2023 16:00
@didier-wenzek didier-wenzek temporarily deployed to Test Pull Request August 10, 2023 16:13 — with GitHub Actions Inactive
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the remaining defaults not be removed as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed.

  • The first 3 are no more used => removed
  • The two latter are related to the file-transfer service => moved them to tedge_api.

Comment on lines 14 to 15
pub struct TEdgeConfigRepository {
config_location: TEdgeConfigLocation,
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how many times the load() method is called (24 times in 23 files), we probably have multiple copies of TEdgeConfig per process. We could lazily initialize it and only return a reference to the single instance. The struct is not very large, so it's not a big problem, but perhaps this will be easier to do sooner rather than later.

@didier-wenzek didier-wenzek temporarily deployed to Test Pull Request August 11, 2023 16:48 — with GitHub Actions Inactive
One has to use new::TEdgeConfig::mqtt_config.

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek didier-wenzek force-pushed the refactor/deprecate-old-tedge-config branch from 9b9c474 to 3fc6e72 Compare August 11, 2023 17:13
@didier-wenzek didier-wenzek temporarily deployed to Test Pull Request August 11, 2023 17:23 — with GitHub Actions Inactive
@didier-wenzek didier-wenzek merged commit fefb27f into thin-edge:main Aug 11, 2023
18 checks passed
@didier-wenzek didier-wenzek deleted the refactor/deprecate-old-tedge-config branch August 11, 2023 19:43
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