Portal Dashboard: Treat MarkdownPartMetadata as untyped JSON

[mikhailshilkov]

Problem

As explained in #1696 and #3023, the azure-native:portal:Dashboard resource is currently ususable due to lack of specific types for dashboard parts. The API supports a multitude of part types, but the open API spec only describes one of them. The result of this is that we generate a discriminated union which is so restricted that it can't be used in practice.

We opened https://github.com/Azure/azure-rest-api-specs/issues/27465 upstream but received no feedback yet. The work to describe all other dashboard part types looks very involved - there are many types with many-many properties. Unfortunately, we don't anticipate reliable type definitions any time soon.

Proposal

As suggested in #3023, this PR introduces a type override to remove the only strongly typed part definition and replace it with a generic DashboardMetadataPart type. The type accepts arbitrary collections of inputs and settings, which enables passing configuration for any part type.

Note that technically this is a breaking change. However, I'm quite convinced that the previous type system prevented anyone from creating any non-trivial dashboards, so we are not breaking any practical user scenarios.

Implementation details

Historically, we implement this kind changes as a special case in our generation logic. However, this change is kind of tricky to do in the generation pass, because we need to introduce an new type, change an existing type, and then (ideally) remove unused types too.

Because of that, and to avoid further bloat of inline generation logic, I decided to introduce a new mechanism in customer resources. It allows specifying "overrides" for schema and metadata types, that would then replace the original types with the same name.

Additionally, I added a final-pass on the schema that deletes all unused object types. The good news is that no previous types were unused, so only the Dashboard Markdown types are being removed now. This also helps with the existing snapshot test: it elides the Dashboard types and therefore the snapshot is unchanged.

Testing

In addition to a few unit tests, I added two end-2-end tests that create a dashboard from TypeScript and C#. The TypeScript one was created by importing an elaborate dashboard created manually in Azure portal. Unfortunately, our C# import is broken beyond repair for untyped dictionaries, so I defined a much simpler dashboard manually.

Resolves #1696 Resolves #3023

[github-actions[bot]]

Does the PR have any schema changes?

Found 18 breaking changes:

Types

• 🟡 "azure-native:portal:DashboardParts": properties: "metadata" type changed from "#/types/azure-native:portal:MarkdownPartMetadata" to "#/types/azure-native:portal:DashboardPartMetadata"
• 🟡 "azure-native:portal:DashboardPartsResponse": properties: "metadata" type changed from "#/types/azure-native:portal:MarkdownPartMetadataResponse" to "#/types/azure-native:portal:DashboardPartMetadataResponse"
• 🔴 "azure-native:portal:MarkdownPartMetadata" missing
• 🔴 "azure-native:portal:MarkdownPartMetadataContent" missing
• 🔴 "azure-native:portal:MarkdownPartMetadataResponse" missing
• 🔴 "azure-native:portal:MarkdownPartMetadataResponseContent" missing
• 🔴 "azure-native:portal:MarkdownPartMetadataResponseSettings" missing
• 🔴 "azure-native:portal:MarkdownPartMetadataResponseSettingsSettings" missing
• 🔴 "azure-native:portal:MarkdownPartMetadataSettings" missing
• 🔴 "azure-native:portal:MarkdownPartMetadataSettingsSettings" missing
• 🔴 "azure-native:workloads:AppServicePlanConfiguration" missing
• 🔴 "azure-native:workloads:AppServicePlanConfigurationResponse" missing
• 🔴 "azure-native:workloads:ErrorAdditionalInfoResponse" missing
• 🔴 "azure-native:workloads:ErrorDetailResponse" missing
• 🔴 "azure-native:workloads:ManagedResourceGroupConfiguration" missing
• 🔴 "azure-native:workloads:ManagedResourceGroupConfigurationResponse" missing
• 🔴 "azure-native:workloads:ManagedServiceIdentity" missing
• 🔴 "azure-native:workloads:ManagedServiceIdentityResponse" missing No new resources/functions.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 95.72193% with 8 lines in your changes missing coverage. Please review.

Project coverage is 57.81%. Comparing base (9ad4f0e) to head (88cd790).

Files	Patch %	Lines
provider/pkg/gen/types.go	83.33%	5 Missing and 1 partial :warning:
provider/pkg/gen/schema.go	90.90%	1 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3123      +/-   ##
==========================================
+ Coverage   56.88%   57.81%   +0.92%     
==========================================
  Files          66       67       +1     
  Lines        8099     8284     +185     
==========================================
+ Hits         4607     4789     +182     
- Misses       3056     3058       +2     
- Partials      436      437       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[mikhailshilkov]

@danielrbradley All good questions - I had similar thoughts. See my answers below.

we could apply the transformation for the types generated for just a specific resource

There is no such thing as types scoped to a single resource. Types are contained within a module, and multiple resources of that module can rely on overlapping types. If we wanted to get a list of all types that a given resource depends on, we'd need to traverse its properties recursively and build out a list. And yet, again, it would not be an exclusive list.

I actually think that we need cross-resource and cross-module transformations. We could use them for existing hard-coded generation tweaks like SubResource.ID expansion, User Assigned Identities shape, maintained sub-resource collection, etc.

it's also not complete as it doesn't allow for transformation of the resource's schema itself

That's straightforward to add later, I don't see why I need to do so in this PR without a use case.

the order of transformations could be an issue as they could easily interact with each other.

That's fair. Do you have ideas how to overcome this? I could add a test that runs transformations in a few random orders and compares the result. Since we are talking about schema types here, if the order becomes important, we will start getting spurious diffs on schema generation, which is already a test of a kind.

We're iterating through every generated type which I expect adds quite a bit of redundant processing

It's an iteration through map keys and parsing each key, which is ~O(N) for N in a few thousands. I expect it to take a few ms max. What am I missing?

We're also using a hard-coded list to try and identify the types associated with the resource. If the schema is updated this will break.

That's fair. Do you have suggestions here? How can we transform the types without relying on their names? The best I can think of is to check the type shape and fail loud and clear if it's not what we expect, forcing a maintainer to resolve it.

If you have a better approach on these in mind, I'm all ears!

[danielrbradley]

@mikhailshilkov I could see a few alternatives here...

1. For easier maintainability, we could just write the component completely manually - and skip the resource being generated at all. This would move us away from a half-way house of generating then kudging.
2. Another option would be skip the resource during the main generation, then when creating the custom resource call into the main generation code but from an isolated context so we get a complete list of the types related to just this one resource, then customise this. Then we could merge the resource & associated types into the main schema.
3. Currently our context during generation is for the whole packageGenerator, but we could create a new context per resource being generated something like resourceGenerator wherein we could load transformations for that specific resource (so resource transformations are registered before generation, keyed by resource) then these transformations would be called while generating all types associated to that resource.
4. A larger change would be to refactor the generation process to avoid mutating any global context - where every resource returns its own resource and associated types, then we use a process to merge each resources types after. This would allow for very simpler intercepting of the values being returned rather than intercepting while they're being generated (just before they're added to the global list).

[mikhailshilkov]

@danielrbradley I refactored the implementation away from all-mighty transformations. Instead, the custom resource is now specifying "overrides" for schema and metadata types, that then replace the original types with the same name.

Additionally, I added a final-pass on the schema that deletes all unused object types. The good news is that no previous types were unused, so only the Dashboard Markdown types are being removed now. This also helps with the existing snapshot test: it elides the Dashboard types and therefore the snapshot is unchanged.

Let me know if you think it's a step in the right direction.

[pulumi-bot]

This PR has been shipped in release v2.48.0.