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

refactor: add support to define_tedge_config for (optionally) multi-value fields #3126

Merged
merged 32 commits into from
Oct 7, 2024

Conversation

jarhodes314
Copy link
Contributor

@jarhodes314 jarhodes314 commented Sep 20, 2024

Proposed changes

In order to add support multiple Cumulocity connections, we need the data model defined for tedge.toml to support multiple cloud connections. This PR adds support in define_tedge_config! for a #[tedge_config(multi)] attribute, which allows the field to store either the underlying struct directly or a hashmap of strings to structs (which provides us with multiple named configurations for a single key).

More concretely, this will allow tedge_config to support configurations like:

[c8y.edge]
url = "https://edge.example.com"
proxy.bind.port = 8001
bridge.topic_prefix = "c8y-edge"

[c8y.cloud]
url = "https://cloud.example.com"
proxy.bind.port = 8002
bridge.topic_prefix = "c8y-cloud"

[mqtt]
bridge.built_in = true

while still also supporting as it does currently:

[c8y]
url = "https://c8y.example.com"

This PR doesn't make any changes to the existing tedge_config definitions and therefore doesn't change the behaviour of thin-edge, hence the refactoring classification.

Still TODO:

  • Update key parsing logic
  • Update tedge config list logic
  • Rename unused fields in match blocks to _
  • Other TODOs left in code

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

Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
@jarhodes314 jarhodes314 added theme:configuration Theme: Configuration management theme:c8y Theme: Cumulocity related topics labels Sep 20, 2024
crates/common/tedge_config_macros/examples/multi.rs Outdated Show resolved Hide resolved
crates/common/tedge_config_macros/examples/multi.rs Outdated Show resolved Hide resolved
crates/common/tedge_config_macros/impl/src/dto.rs Outdated Show resolved Hide resolved
crates/common/tedge_config_macros/impl/src/query.rs Outdated Show resolved Hide resolved
crates/common/tedge_config_macros/impl/src/query.rs Outdated Show resolved Hide resolved
crates/common/tedge_config_macros/impl/src/reader.rs Outdated Show resolved Hide resolved
crates/common/tedge_config_macros/src/multi.rs Outdated Show resolved Hide resolved
crates/common/tedge_config_macros/src/multi.rs Outdated Show resolved Hide resolved
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Copy link
Contributor

github-actions bot commented Sep 20, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
505 0 2 505 100 1h33m8.767507999s

Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Co-authored-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
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. Thank you for this tour de force.

Despite its underneath complexity, tedge config is simple to use both by end users and developers (despite or thanks to this complexity?). However, we reached a point where things are really complicated and I would be very careful about adding additional features.

new_parents.push(PathItem::Dynamic(*span));
}
} else {
// This key has diverged from the currrent key's parents, so empty the list
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This key has diverged from the currrent key's parents, so empty the list
// This key has diverged from the current key's parents, so empty the list

Comment on lines 47 to 49
#[error("Unexpected profile {1} for the non multi-profile property {0}")]
#[error(
"You are trying to access a profile `{1}` of {0}, but profiles are not enabled for {0}"
)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a preference for the former error message.

@jarhodes314 jarhodes314 added this pull request to the merge queue Oct 7, 2024
Merged via the queue into thin-edge:main with commit 300fb77 Oct 7, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:c8y Theme: Cumulocity related topics theme:configuration Theme: Configuration management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants