Add support for resources to be represented as hash map as part of ARM Language 2.0 #1210
Conversation
| #[test] | ||
| fn test_deserialize_resources_array() { | ||
| let config_json = r#"{ | ||
| "$schema": "https://raw.githubusercontent.com/PowerShell/DSC/main/schemas/2024/04/config/document.json", |
There was a problem hiding this comment.
Shouldn't the schema be: https://aka.ms/dsc/schemas/v3/bundled/config/document.json?
There was a problem hiding this comment.
Yes, these are generated and I'm probably going to delete them now that I've been pointed at the Pester tests.
| #[test] | ||
| fn test_deserialize_resources_object() { | ||
| let config_json = r#"{ | ||
| "$schema": "https://raw.githubusercontent.com/PowerShell/DSC/main/schemas/2024/04/config/document.json", |
| pub resources: Vec<Resource>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub variables: Option<Map<String, Value>>, | ||
| /// Irrelevant Bicep metadata from using the extension |
There was a problem hiding this comment.
Is this documented somewhere?
There was a problem hiding this comment.
Seems like it might be coming from https://learn.microsoft.com/en-us/azure/azure-resource-manager/bicep/bicep-import, but they don't show what the resulting ARM JSON looks like
There was a problem hiding this comment.
I put an example of the resulting ARM in the PR description. Since using the DSC extension for Bicep results in it importing the extension, this metadata gets added to the output (along with the other fields).
There was a problem hiding this comment.
I missed that :)
Seems unfortunate Bicep created a new property for this rather than sticking it into metadata
| let value = Value::deserialize(deserializer)?; | ||
|
|
||
| match value { | ||
| Value::Array(resources) => { |
There was a problem hiding this comment.
Do you need to validate that the items in the array are of type Resource?
There was a problem hiding this comment.
I don't think so. If they're not, we'd be in the exact same situation as the existing code, the deserialization will fail and pass up the error.
There was a problem hiding this comment.
It would be good to just have a Pester test case cover this then
| .map(|(name, resource)| { | ||
| let mut resource: Resource = serde_json::from_value(resource).map_err(serde::de::Error::custom)?; | ||
| // TODO: Decide what to do if resource.name is already set. | ||
| resource.name = name; |
There was a problem hiding this comment.
Looking at the ARM docs https://learn.microsoft.com/en-us/azure/azure-resource-manager/templates/resource-declaration#use-symbolic-name, I don't think you should just clobber the name with the symbolic name. Instead, looking at https://learn.microsoft.com/en-us/azure/azure-resource-manager/templates/resource-dependency, it appears that symbolic names can be used in dependsOn in addition to resourceId() in the classical sense. So I think what we may need to eventually (this can be a follow-up PR) do is maintain a list of symbolic names and for resolving dependencies allow for either symbolic name or resourceid().
For this PR, I would just change the TODO comment to reflect what I said above.
| } | ||
|
|
||
| #[test] | ||
| fn test_deserialize_resources_array() { |
There was a problem hiding this comment.
It's ok to keep a few unit tests here, but would prefer to also add some Pester tests.
| pub api_version: Option<String>, | ||
| /// A friendly name for the resource instance | ||
| #[serde(default)] | ||
| pub name: String, // friendly unique instance name |
There was a problem hiding this comment.
By the way, this is saying that if the name field doesn't exist (such as in the case of ARMv2 where symbolic names are used instead) that it just be an empty string.
There was a problem hiding this comment.
In ARM, keep in mind that name is an identity of a ARM resource, so it's not the friendly name usage we have in DSC. The symbolic name is more like the friendly name (although with character restrictions).
SteveL-MSFT
changed the title
resources as an object3 participants
Okay so I tried a bunch of different ways and settled on this being the simplest approach. In short, a custom serde deserializer function let's us take
resourcesas either an array or an object and produce what the existing implementation expects: aVec<Resource>such that after the initial deserialization, nothing else needs to change. This means that later ordering should work (though when I testeddependsOnin Bicep, I got a different error).I can at least now pass through this Bicep directly to DSC and it gets to the point that it attempts to execute the resource:
which produces this ARM/JSON:
{ "$schema": "https://aka.ms/dsc/schemas/v3/bundled/config/document.json", "languageVersion": "2.0", "contentVersion": "1.0.0.0", "metadata": { "_generator": { "name": "bicep", "version": "0.38.33.27573", "templateHash": "9007474339958309780" } }, "imports": { "dsc": { "provider": "DesiredStateConfiguration", "version": "0.1.0" } }, "resources": { "myEcho": { "import": "dsc", "type": "Microsoft.DSC.Debug/Echo@1.0.0", "properties": { "output": "Hello world!" } } } }Perhaps we should gate this behind a feature-flag, but between this and publishing a basic version of the Bicep extension to an MCR, we can resume demos but with fancy IntelliSense and working versions!
For the fields noted as irrelevant, I sure would like to use
#[serde(skip)]but it is incompatible withdeny_unknown_fields. Perhaps we should revisit that serde flag, possibly using the linked workaround withIgnoredAnyand warnings instead of failures.I have to admit that the additional tests were generated by Claude, though it was pretty good at it!