-
Notifications
You must be signed in to change notification settings - Fork 69
refactor: Deserialise separate mapper config file using existing logic for tedge.toml #3889
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
base: main
Are you sure you want to change the base?
refactor: Deserialise separate mapper config file using existing logic for tedge.toml #3889
Conversation
…c for tedge.toml Signed-off-by: James Rhodes <[email protected]>
|
|
||
| async fn execute(&self, tedge_config: TEdgeConfig) -> Result<(), MaybeFancy<anyhow::Error>> { | ||
| restrict_cloud_config_update("add", &self.key, &tedge_config).await?; | ||
| restrict_cloud_config_access("add", &self.key, &tedge_config).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, all the tedge config commands are still as before, however I intend to fix that (it's just slightly tricky figuring out exactly how to achieve the fine details, but the broad "generate a TEdgeConfigDto" for the mapper config aspect is working and in use for deserialisation).
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
| match key.split_once(".") { | ||
| Some(("c8y", rest)) => Some((CloudType::C8y, extract_profile_name(rest))), | ||
| Some(("az", rest)) => Some((CloudType::Az, extract_profile_name(rest))), | ||
| Some(("aws", rest)) => Some((CloudType::Aws, extract_profile_name(rest))), | ||
| _ => None, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a nice idea to move this logic out of restrict_cloud_config_acces. We have now two independent steps clearly separated.
Robot Results
Failed Tests
|
| + Send | ||
| + Sync | ||
| + 'static | ||
| Sized + ExpectedCloudType + FromCloudConfig + Send + Sync + 'static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Sized + ExpectedCloudType + FromCloudConfig + Send + Sync + 'static | |
| Sized + ExpectedCloudType + FromCloudConfig + Send + Sync + std::fmt::Debug + 'static |
So that the MapperConfig can also be Debug.
|
|
||
| #[derive(Debug, Document)] | ||
| pub struct MapperMapperConfig<M> { | ||
| pub struct MapperMapperConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub struct MapperMapperConfig { | |
| pub struct CommonMapperConfig { |
|
|
||
| pub include: BridgeIncludeConfig, | ||
| pub struct C8yBridgeConfig { | ||
| pub include: C8yBridgeIncludeConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could have been left as a generic mapper config, no? As it is about the local bridge connection which is valid for any mapper, whether they use it or not?
| /// Cumulocity-specific mapper configuration fields | ||
| #[derive(Debug, Deserialize, Document)] | ||
| #[serde(default)] | ||
| pub struct C8yMapperSpecificConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vision is to remove the duplication between this struct and the TEdgeConfigDtoC8y generated by the define_tedge_config! macro in a subsequent step, right? Or this is the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should be the case. I think my ideal vision at this stage is for the shared configurations to be defined as a single mapper group in the define_tedge_config! macro to avoid duplication, and that will define the separate cloud configurations as well as the generalised mapper config that can be used internally to remove duplication in the various mappers.
At the moment, I think I am tending towards a solution where TEdgeConfig is proactively populated with the mapper configuration tomls where appropriate, as opposed to reading them on demand, as that makes implementing tedge config commands far simpler, and reduces the amount of code split we need between the handling the two ways of configuring mappers.
Signed-off-by: James Rhodes <[email protected]>
didier-wenzek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This work is making progress in the correct direction.
tedge config get c8y.urlis correctly reading/etc/tedge/mappers/c8y.tomltedge config get c8y.url --profile foois correctly reading/etc/tedge/mappers/c8y.d/foo.tomltedge config list urlis correctly listing c8y settings in/etc/tedge/mappers/c8y.tomland/etc/tedge/mappers/c8y.d/foo.toml, ignoring those incorrectly added to/etc/tedge/tedge.tomltedge connect c8y --testandsudo tedge connect c8yare working as expected.
minor:
tedge connect c8y --testandsudo tedge connect c8ycomplain that "Both /etc/tedge/mappers/c8y.toml and tedge.toml [c8y] exist.", even when there is no[c8y]section in/etc/tedge/tedge.toml
Proposed changes
Use the existing
TEdgeConfigC8yDto(etc.) structs to deserialise separate mapper config files, with a view to supporting thetedge configCLI fully.Still TODO
tedge configsupportTypes of changes
Paste Link to the issue
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments