Add configuration object #379
Conversation
| @@ -74,6 +85,13 @@ impl TryFrom<ank_base::State> for State { | |||
| .into_iter() | |||
| .map(|(k, v)| Ok((k.to_owned(), v.try_into()?))) | |||
| .collect::<Result<HashMap<String, StoredWorkloadSpec>, String>>()?, | |||
| configs: item | |||
There was a problem hiding this comment.
Its Friday evening, but I am currently thinking about these lines of code: Why is the item.workloads call above (I cannot select it in the web page since it was not changed) throwing an error if it is missing? Couldn't be the workloads optional as well in the proto message?
There was a problem hiding this comment.
You are right. I have also changed it for workloads.
|
@christoph-hamm: With the current object construction the filtering of config items via filter masks is not possible. |
| .collect(), | ||
| configs: HashMap::new(), |
There was a problem hiding this comment.
Why is the workload referencing some config items but in the configs field they do not exist? My expectation to the generate_test_... functions is that I get always valid test dummy data.
Below are also other functions that contain references to configs but no configuration.
If it is conflicting in the utests, then I would suggest to create two separate functions, one that delivers me workload s and states without configs at all and the other ones delivering test data including configs.
Then we can maybe also get rid of the conditional set in the server_state.rs utest where configs is set to None in some cases.
|
Review part II is done. Comments published. |
| ("ref1".into(), "config.path.1".into()), | ||
| ("ref2".into(), "config.path.2".into()), |
There was a problem hiding this comment.
Like mentioned in the issue #267, we decided to go with just one level of referencing configs in workloads. I will implement in the upcoming changes for workload configs also a naming convention for config key names to prevent messing up the filtering (same regex like for agents and workloads)
Can you change all test data occurrences to a version without the extended path with dots. Just one level of config key reference?
Issues: #267
Definition of Done
The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.