Custom resource schema modifications by danielrbradley · Pull Request #3571 · pulumi/pulumi-azure-native

danielrbradley · 2024-09-12T11:31:40Z

Add a more understandable and unified approach to modifying existing parts of the schema.

Use a function to define a resource's schema–resource & types in both the schema and metadata. This is given any existing declaration which it is allowed to modify.

If newly added types conflict with an existing type in the schema, we will raise an error.
Allows modifying any of the types associated with the custom resource.
Allow modification of both the schema and the metadata in one abstration.
Allows creation of brand new resources - the existing resource will just be nil.

This allows the removal of the custom resource Types, Schema and Meta fields along with the associated SchemaMixins, SchemaTypeMixins and MetaMixins functions.

github-actions · 2024-09-12T11:36:27Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

codecov · 2024-09-12T11:48:40Z

Codecov Report

Attention: Patch coverage is 81.86813% with 33 lines in your changes missing coverage. Please review.

Project coverage is 59.83%. Comparing base (cf9de59) to head (10d7d04).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
...sources/customresources/custom_portal_dashboard.go	85.36%	6 Missing and 6 partials ⚠️
...r/pkg/resources/customresources/customresources.go	76.59%	6 Missing and 5 partials ⚠️
provider/pkg/resources/typeVisitor.go	82.22%	6 Missing and 2 partials ⚠️
provider/pkg/gen/schema.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3571      +/-   ##
==========================================
+ Coverage   58.98%   59.83%   +0.84%     
==========================================
  Files          68       69       +1     
  Lines        8658     8786     +128     
==========================================
+ Hits         5107     5257     +150     
+ Misses       3081     3037      -44     
- Partials      470      492      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

thomas11

LGTM overall, nice work!

Some thoughts:

I'm not sure why we need to discover all referenced types upfront, since most custom resources won't even use them. Could we offer findTypesForResource as a utility for custom resources instead, or even get away with just TypeLookupFunc for resources that know which types they want to access?
When two custom resources touch the same resource or type, the result depends on the order of iteration customresources.ApplySchemaModifications. It's sufficiently unlikely so it's ok, I think, but even better would be to keep track of modifications and warn if something is touched a second time.
Do we need MetaTypes when SchemaModifications can also add metadata?

danielrbradley · 2024-09-12T12:40:10Z

I'm not sure why we need to discover all referenced types upfront, since most custom resources won't even use them. Could we offer findTypesForResource as a utility for custom resources instead, or even get away with just TypeLookupFunc for resources that know which types they want to access?

It felt like an easier API if you get passed values and you return values rather than callbacks. It avoids questions like "what happens if they call the callback lots of times" etc too. The types will only be searched if the custom resource implements the SchemaModifications function (which is only the Dashbaord right now).

When two custom resources touch the same resource or type, the result depends on the order of iteration customresources.ApplySchemaModifications. It's sufficiently unlikely so it's ok, I think, but even better would be to keep track of modifications and warn if something is touched a second time.

Yes, good catch on the map iteration. Interestingly this is also the case for all the mixin functions too. I'll change them all to iterate by name for consistency at least.

Do we need MetaTypes when SchemaModifications can also add metadata?

Not strictly, but it's a simpler form to use if only adding and not modifying anything existing.

thomas11 · 2024-09-12T12:44:15Z

I'm not sure why we need to discover all referenced types upfront, since most custom resources won't even use them. Could we offer findTypesForResource as a utility for custom resources instead, or even get away with just TypeLookupFunc for resources that know which types they want to access?

It felt like an easier API if you get passed values and you return values rather than callbacks. It avoids questions like "what happens if they call the callback lots of times" etc too. The types will only be searched if the custom resource implements the SchemaModifications function (which is only the Dashbaord right now).

Makes sense!

Do we need MetaTypes when SchemaModifications can also add metadata?

Not strictly, but it's a simpler form to use if only adding and not modifying anything existing.

In isolation, it's simpler, but having two ways to do the same thing is not simpler IMO (think of debugging).

There's also nothing keeping people from the footgun of using both.

danielrbradley · 2024-09-12T13:16:23Z

Do we need MetaTypes when SchemaModifications can also add metadata?

Not strictly, but it's a simpler form to use if only adding and not modifying anything existing.

In isolation, it's simpler, but having two ways to do the same thing is not simpler IMO (think of debugging).

There's also nothing keeping people from the footgun of using both.

Ok, I like the move to just a single function for defining either new or edits for the whole schema as once. I'll just remove the fix from this PR for now, but also make the ResourceDefinition a pointer so it's usable where there's not an existing resource:

func(resource *ResourceDefinition) (ResourceDefinition, error)

danielrbradley · 2024-09-12T15:53:31Z

Rebased and squashed reworking following review .. and implemented the dashboard changes.

mikhailshilkov

The approach makes sense on the first glance. I'll review tomorrow - hopefully after you add unit and integration tests. I think we also still want the asserts about the shape of the existing resource that we discussed before.

Do we also want to override resources with explicit versions, since this is much easier with this design?

mikhailshilkov

This looks like a step in the right direction, thank you! Should we check for possibilities to rewrite some existing schema modifications in this style?

mjeffryes

I agree with the general direction here, but there are a lot of details to sort out still. I suspect, it will be easier to reason about as a pure refactor, independent of restoring the dashboard customization.

Would like to see this fully replace all the custom edits in genMixins as well.

danielrbradley · 2024-09-19T11:58:31Z

I've rebased, addressed the minor comments and reworked the Portal schema to be much more similar to the existing code, with the addition of asserts over each type we're replacing to flag when there's changes upstream which affect the parts we're overwriting.

mikhailshilkov

The changes since my last review look good. Still missing test coverage.

mikhailshilkov · 2024-09-23T08:46:52Z

+	if err != nil {
+		t.Fatal(err)
+	}
+	snaps.MatchSnapshot(t, generationResult.Schema)


In my opinion, snapshot tests are no substitute to proper unit tests. It was okay to add them post-factum for all the existing code that we had no tests for, but I suggest we don't use them as an excuse not to write tests for net-new non-trivial functionality.

Your current PR is a nice illustration of my case: as far as I can tell, your snapshot is actually not testing anything at all. The only portal resource that the snapshot contains is azure-native:portal/v20200901preview:Dashboard, which is not processed by the custom resource you implemented, so it just passes nil to the Schema method, and the method exists immediately. Yet, the "test" provides an impression that we are testing something, and it's hard to tell otherwise without looking into the details.

@thomas11 @mjeffryes I'd love to get your opinion on this testing approach and whether it matches mine or not.

Yes, it looks like the snapshot currently misses generating the default resource, I'll take a look at fixing that when I get opportunity.

@mikhailshilkov Perhaps if you're envisioning something specific with the unit tests that you think is more effective, you could contribute it?

Apart from the snaps vs unit tests question, a test that passes without asserting anything is a bug. Can we add some assertions here to prevent that? Along the lines of "generationResult contains foo".

In general, I'm all for unit tests over snap-based tests, but for things the portal resource where you have a bunch of opaque JSON, snaps can still a good fit.

How can we test anything except the happy path with snapshot tests? I guess we miss a mechanism to amend snapshots to cause errors/edge cases and assert the outcomes?

My understanding is that snapshot tests, as we're using them, are strictly for the happy path.

mikhailshilkov · 2024-09-23T08:50:55Z

+					TypeSpec: schema.TypeSpec{
+						Type: "string",
+					},
+					Description: "The dashboard part metadata type.\nExpected value is 'Extension/HubsExtension/PartType/MarkdownPart'.",


On the second pass, I'm not sure I like the strictness of this. We will have to re-write this code in case upstream changes the description of a type that we are throwing away, right? That seems unnecessary fragile?

This is why the original design I proposed attempted to derrive the modified schema from the original schema in order to be more flexible, but the feedback on that was that it was too complex & fragile. The frequency of updates hasn't been too high previously so we can always refine these asserts if needed later.

mikhailshilkov · 2024-09-23T08:51:36Z


+			existingMarkdownPartMetadataType := resource.Types["azure-native:portal:MarkdownPartMetadata"]
+			if !reflect.DeepEqual(existingMarkdownPartMetadataType, expectedMarkdownPartMetadataType("")) {
+				return nil, fmt.Errorf("unexpected azure-native:portal:MarkdownPartMetadata type: %#v", existingMarkdownPartMetadataType)


Should we provide guidance to future maintainers about what they need to do in this situation?

Sure, can refine these message further, though having any messages is already a big improvement over the current failure mode.

…rces

- Use single function to all the modification or creation of new resources, in the schema and meta as well as creating or modifying the associated types. - Apply modifications in a consistent order. - Implement finding types via type visitor. From review feedback: - Use codegen.SortedKeys - Use early return - Rename 'ok's - Ensure original corresponding meta types exist

- Mirror existing implementation - Add asserts before replacing types - This clearly documents what we're expecting to be present before we add our replacements.

Ensure that changes to the custom portal schema and metadata are visible. - Fix default dashboard version by applying transformations. - Add not nil assertions. - Use JSON matchers instead of generic matchers for better snapshots.

- Rename Schema to LegacySchema. - Rename SchemaF to Schema. - Add deprecation notices to old properties.

Much less verbose snapshot and encodes the language sections correctly.

pulumi-bot · 2024-09-30T13:50:19Z

This PR has been shipped in release v2.64.0.

mikhailshilkov · 2024-09-30T15:17:27Z

Note that the "Dashboard Fix" section of the description was not correct anymore, so I deleted it

danielrbradley self-assigned this Sep 12, 2024

thomas11 approved these changes Sep 12, 2024

View reviewed changes

danielrbradley force-pushed the custom-resource-schema-modifications branch from 64411c7 to d3a4fe2 Compare September 12, 2024 15:47

danielrbradley marked this pull request as ready for review September 12, 2024 15:52

danielrbradley requested review from mikhailshilkov and thomas11 September 12, 2024 15:55

mikhailshilkov reviewed Sep 12, 2024

View reviewed changes

mikhailshilkov reviewed Sep 16, 2024

View reviewed changes

mikhailshilkov mentioned this pull request Sep 16, 2024

Re-apply Portal Dashboard: Treat Part's Metadata as untyped JSON #3566

Merged

mjeffryes reviewed Sep 16, 2024

View reviewed changes

Comment thread provider/pkg/gen/schema.go

Comment thread provider/pkg/resources/customresources/custom_portal_dashboard.go Outdated

Comment thread provider/pkg/resources/customresources/custom_portal_dashboard.go Outdated

Comment thread provider/pkg/resources/customresources/customresources.go

danielrbradley force-pushed the custom-resource-schema-modifications branch from 2fed4e2 to 5e12e73 Compare September 19, 2024 11:56

danielrbradley requested review from mikhailshilkov and mjeffryes September 19, 2024 11:58

mikhailshilkov reviewed Sep 20, 2024

View reviewed changes

Comment thread provider/pkg/resources/customresources/customresources.go Outdated

danielrbradley requested a review from mikhailshilkov September 20, 2024 15:11

mikhailshilkov reviewed Sep 23, 2024

View reviewed changes

danielrbradley requested a review from mikhailshilkov September 25, 2024 07:59

thomas11 reviewed Sep 30, 2024

View reviewed changes

Comment thread provider/pkg/gen/schema_test.go Outdated

danielrbradley added 4 commits September 30, 2024 10:45

Move types visitor to avoid import cycle when using from custom resou…

f2718d6

…rces

Implement dashboard customisations

3a6f92d

- Mirror existing implementation - Add asserts before replacing types - This clearly documents what we're expecting to be present before we add our replacements.

Add dashboard snapshot test

844e7e6

Ensure that changes to the custom portal schema and metadata are visible. - Fix default dashboard version by applying transformations. - Add not nil assertions. - Use JSON matchers instead of generic matchers for better snapshots.

danielrbradley added 3 commits September 30, 2024 10:45

Deprecate old schema properties

586e10d

- Rename Schema to LegacySchema. - Rename SchemaF to Schema. - Add deprecation notices to old properties.

Switch vnet test to JSON matchers

cf7dbb6

Much less verbose snapshot and encodes the language sections correctly.

Fix test helper typo

10d7d04

danielrbradley force-pushed the custom-resource-schema-modifications branch from 46f3fe1 to 10d7d04 Compare September 30, 2024 09:45

danielrbradley enabled auto-merge September 30, 2024 09:46

thomas11 approved these changes Sep 30, 2024

View reviewed changes

danielrbradley merged commit 3610e47 into master Sep 30, 2024

danielrbradley deleted the custom-resource-schema-modifications branch September 30, 2024 10:22

pulumi-bot mentioned this pull request Sep 30, 2024

Dashboards are unusable again #3562

Closed

dependabot Bot mentioned this pull request Aug 18, 2025

Bump Pulumi and Pulumi.AzureNative merca/data-and-stuff#118

Closed

dependabot Bot mentioned this pull request Aug 25, 2025

Bump Pulumi and Pulumi.AzureNative merca/data-and-stuff#123

Closed

This was referenced Sep 1, 2025

Bump Pulumi and Pulumi.AzureNative merca/data-and-stuff#130

Closed

Bump Pulumi and Pulumi.AzureNative merca/data-and-stuff#138

Closed

dependabot Bot mentioned this pull request Sep 15, 2025

Bump Pulumi and Pulumi.AzureNative merca/data-and-stuff#145

Closed

dependabot Bot mentioned this pull request Sep 22, 2025

Bump Pulumi and Pulumi.AzureNative merca/data-and-stuff#152

Closed

Conversation

danielrbradley commented Sep 12, 2024 • edited by mikhailshilkov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 12, 2024

Does the PR have any schema changes?

Uh oh!

codecov Bot commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thomas11 left a comment

Choose a reason for hiding this comment

Uh oh!

danielrbradley commented Sep 12, 2024

Uh oh!

thomas11 commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielrbradley commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielrbradley commented Sep 12, 2024

Uh oh!

mikhailshilkov left a comment

Choose a reason for hiding this comment

Uh oh!

mikhailshilkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjeffryes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielrbradley commented Sep 19, 2024

Uh oh!

mikhailshilkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pulumi-bot commented Sep 30, 2024

Uh oh!

mikhailshilkov commented Sep 30, 2024

Uh oh!

danielrbradley commented Sep 12, 2024 •

edited by mikhailshilkov

Loading

codecov Bot commented Sep 12, 2024 •

edited

Loading

thomas11 commented Sep 12, 2024 •

edited

Loading

danielrbradley commented Sep 12, 2024 •

edited

Loading