Enable preview for create and update #1891

corymhall · 2024-12-06T17:48:55Z

Currently if you update a resource, all outputs are shown as computed
during preview. This means that downstream resources might show a
replacement in the plan if they are using one of those output values.

There are some outputs that we know will not change unless the resource
is replaced (I'm referring to these as "stable" outputs). For stable
outputs we can copy the value from the state in order to show an
accurate preview.

The problem is that the CCAPI schema has no way of determining
programmatically which outputs are stable and which are not. Because of
this, this PR introduces a heuristic to determine if an output is a
stable output.

Is the value a readOnlyProperty in the schema? This indicates that
it is a computed output property
Is the property the resource id, arn or name? If the id, arn, or
name is a computed property then we can assume that it is a stable
property. I don't know of any cases where these change outside of a
resource replacement.

This is not 100% coverage of stable properties, but it should get us
most of the impactful properties. To get close to 100% coverage we will
probably need a schema overlay where we can contribute to mapping these
stable properties for each resource.

Note: as part of this I had to add readOnly properties to metadata.json

closes #1141, re #1892

github-actions · 2024-12-06T17:50:06Z

Does the PR have any schema changes?

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

codecov · 2024-12-06T18:02:56Z

Codecov Report

Attention: Patch coverage is 87.63441% with 23 lines in your changes missing coverage. Please review.

Project coverage is 50.37%. Comparing base (7ca5a58) to head (784e3ad).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
provider/pkg/provider/provider.go	71.42%	8 Missing and 4 partials ⚠️
provider/pkg/resources/cfn_custom_resource.go	0.00%	4 Missing ⚠️
provider/pkg/resources/extension_resource.go	0.00%	4 Missing ⚠️
provider/pkg/outputs/previewOutputs.go	97.61%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1891      +/-   ##
==========================================
+ Coverage   49.36%   50.37%   +1.00%     
==========================================
  Files          47       48       +1     
  Lines        6814     6998     +184     
==========================================
+ Hits         3364     3525     +161     
- Misses       3206     3224      +18     
- Partials      244      249       +5

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

flostadler · 2024-12-09T18:38:42Z

provider/pkg/provider/previewOutputs.go

+// - `aws-native:securityhub:PolicyAssociation` has a property `associationIdentifier`
+//   - TODO[pulumi/aws-native#1892]: in some cases this property is the `primaryIdentifier`. Could we use that as another heuristic?
+//     a readonly property that is also a primary identifier? It doesn't catch all cases, but would catch more
+func isStableOutput(propName string, resourceTypeName tokens.TypeName) bool {


flostadler · 2024-12-09T18:42:01Z

provider/pkg/provider/previewOutputs.go

+	readOnly []string,
+) resource.PropertyMap {
+	result := resource.PropertyMap{}
+	// Then this is an Extension resource which has all outputs in an "outputs" property


Wouldn't CFN custom resources end up in the same if statement here?
They're also custom resources.

Could we make this more explicit?

Yeah maybe I should just split it into separate functions.

I moved these to be methods on the CustomResource type since that seems to be the only good way of telling them apart.

t0yv0 · 2024-12-09T20:05:43Z

provider/pkg/provider/provider.go

+		if err != nil {
+			return nil, err
+		}
+		return &pulumirpc.UpdateResponse{Properties: previewState}, nil


Id is omitted because it's unknown?

Id isn't part of the UpdateResponse

Ah. Well I meant to comment on CreateResponse. I've checked the bridge code it's a little complicated, in CreateResponse it's returning plannedState.ID() and encoding unknowns as "". This makes me suspect sometimes it can predict the ID and send it out during preview. But perhaps omitting it works just as well.

t0yv0 · 2024-12-09T20:16:05Z

provider/pkg/provider/previewOutputs.go

+	case inputValue.IsObject() && strings.HasPrefix(prop.Ref, "#/types/"):
+		typeName := strings.TrimPrefix(prop.Ref, "#/types/")
+		if t, ok := types[typeName]; ok {
+			v := previewResourceOutputs(inputValue.ObjectValue(), types, t.Properties, readOnly)


This call here is no longer addressing a resource, but a nested object type, but the naming is confusing and it's hard to be sure this always does the right thing.

I can rename it to something else, but why do you think it's hard to be sure this always does the right thing?

That's a good point, I had to look at it with fresh eyes this morning, I think it's good! It does the right thing. readOnly properties are specified using paths into conforming objects, and jumping into #ref properties does the right thing. You have it tested too. I noticed some wrinkle inconsistencies that I added some more comments about but I'm comfortable with this now, thank you!

provider/pkg/provider/provider.go

provider/pkg/provider/previewOutputs.go

t0yv0 · 2024-12-09T20:33:38Z

Let's discuss briefly tomorrow, are recursive cases strictly necessary - perhaps worth shipping a smaller much simpler change first to test the waters (covering ARN properties) and then extend as needed with some additional testing? Or perhaps we'll decide to ship as-is. Thanks.

provider/pkg/provider/previewOutputs_test.go

provider/pkg/provider/previewOutputs.go

t0yv0 · 2024-12-10T15:47:08Z

This made me curious to poke at codecov. Besides the map case which is certainly interesting, there is this bit that's potentially interesting to cover:

+ 	// we can't handle arrays because we don't know which item in the array
+ 	// the value corresponds to
+ 	if isArrayProperty(propName) {
+ 		return false
+ 	}
+

provider/pkg/provider/previewOutputs.go

provider/pkg/provider/previewOutputs_test.go

t0yv0 · 2024-12-10T16:04:56Z

I think there's a consistency issue with capitalization of our metadata (pre-this PR).

In CF schema, we have Dns property of "#/definitions/DnsMap" type like this:

    "DnsMap" : {
      "description" : "A map of DNS names for the AppSync API.",
      "type" : "object",
      "additionalProperties" : false,
      "properties" : {
        "Realtime" : {
          "type" : "string"
        },
        "Http" : {
          "type" : "string"
        }
      }
    },

pulumi-aws-native/aws-cloudformation-schema/aws-appsync-api.json

Line 229 in 3288f39

"Dns" : {

But in the metadata we have:

#/types/aws-native:appsync:ApiDnsMap

            "readOnly": [
                "apiArn",
                "apiId",
                "dns",
                "dns/Http",
                "dns/Realtime"
            ],


        "aws-native:appsync:ApiDnsMap": {
            "type": "object",
            "properties": {
                "http": {
                    "type": "string",
                    "description": "The domain name of the Api's HTTP endpoint."
                },
                "realtime": {
                    "type": "string",
                    "description": "The domain name of the Api's real-time endpoint."
                }
            }
        },

These are not captialized consistently. "dns/Http" and "dns/Realtime".

t0yv0 · 2024-12-10T16:07:45Z

Ah "Note: as part of this I had to add readOnly properties to metadata.json" - that's a separate piece. Let's maybe get that in as a separate quick PR and figure out the capitalization there? Thanks a ton!

corymhall · 2024-12-10T18:00:21Z

This change is part of the following stack:

Enable preview for create and update #1891 ◀

_{Change managed by git-spice.}

Currently if you update a resource, all outputs are shown as computed during preview. This means that downstream resources might show a replacement in the plan if they are using one of those output values. There are some outputs that we know will not change unless the resource is replaced (I'm referring to these as "stable" outputs). For stable outputs we can copy the value from the state in order to show an accurate preview. The problem is that the CCAPI schema has no way of determining programmatically which outputs are stable and which are not. Because of this, this PR introduces a heuristic to determine if an output is a stable output. - Is the value a `readOnlyProperty` in the schema? This indicates that it is a computed output property - Is the property the resource `id`, `arn` or `name`? If the id, arn, or name is a computed property then we can assume that it is a stable property. I don't know of any cases where these change outside of a resource replacement. This is not 100% coverage of stable properties, but it should get us most of the impactful properties. To get close to 100% coverage we will probably need a schema overlay where we can contribute to mapping these stable properties for each resource. closes #1141

t0yv0 · 2024-12-11T15:50:55Z

provider/pkg/provider/previewOutputs.go

+				}
+			} else {
+				key := resource.PropertyKey(readOnlyProp)
+				if output, ok := outputsFromPriorState[key]; ok {


This branch will not consider outpusFromInputs? Why?

outputsFromInputs will have all possible outputs properties because of previewResourceOutputs, so if it's a stable property and it exists in outputsFromPriorState then copy it over.

t0yv0 · 2024-12-11T15:54:26Z

provider/pkg/provider/previewOutputs.go

+// will have `name`, `id`, and `arn` properties, and sometimes they will have `resourceName`, `resourceId`,
+// and `resourceArn` properties.
+func populateStableOutputs(
+	outputsFromInputs resource.PropertyMap,


This code would read much easier to me if the argument was called newInputs and the result of the function was a separate map:

outputsFromInputs := newInputs.Copy()

Like relying on modifying the first parameter is highly unexpected.

t0yv0 · 2024-12-11T15:57:42Z

provider/pkg/provider/previewOutputs.go

+				key := resource.PropertyKey(current)
+				if outputFromInput, ok := outputsFromInputs[key]; ok {
+					if outputFromState, ok := outputsFromPriorState[key]; ok {
+						outputsFromInputs[key] = resource.NewObjectProperty(populateStableOutputs(


Will this only fire up when both inputs and prior state is present? Curious if this will exclude the case when an input is present but output not.. Ah ok that may be expected because this is only working with 'stable' info propagation.

How about when a state entry is present but the input entry is not? Why will it skip this?

I think I've changed my mind about a previous comment you made about this being part of previewResourceOutputs. This function is meant to only be called with the output of previewResourceOutputs so outputsFromInputs contains all the possible output properties already.

If an entry is not present in the input, but is in the prior state then it will be marked as computed (as part of the previewResourceOutputs function.

provider/pkg/provider/provider.go

t0yv0 · 2024-12-11T16:02:19Z

provider/pkg/provider/previewOutputs.go

+	readOnly []string,
+) resource.PropertyValue {
+	switch {
+	case inputValue.IsSecret():


provider/pkg/provider/previewOutputs.go

- combine entrypoint into a single function for both create and update

t0yv0 · 2024-12-12T19:53:03Z

provider/pkg/outputs/previewOutputs.go

+) resource.PropertyMap {
+	outputsFromInputs := previewResourceOutputs(newInputs, types, outputTypes, readOnlyProperties)
+	if outputsFromPriorState != nil {
+		return populateStableOutputs(outputsFromInputs, *outputsFromPriorState, readOnlyProperties, resourceTypeName)


t0yv0 · 2024-12-12T19:53:26Z

provider/pkg/outputs/previewOutputs.go

+// will have `name`, `id`, and `arn` properties, and sometimes they will have `resourceName`, `resourceId`,
+// and `resourceArn` properties.
+func populateStableOutputs(
+	// outputsFromInputs has to be the output of previewResourceOutputs


t0yv0 · 2024-12-12T19:53:54Z

provider/pkg/outputs/previewOutputs.go

+			return resource.NewObjectProperty(v)
+		}
+		// AdditionalProperties (map types)
+	case inputValue.IsObject() && prop.AdditionalProperties != nil:


t0yv0

🚢

flostadler

Love it!

flostadler · 2024-12-12T20:31:54Z

examples/examples_nodejs_test.go

+	test := pulumitest.NewPulumiTest(t, filepath.Join(cwd, "stable-outputs-preview"), options...)
+	test.SetConfig(t, "lambdaDescription", "Lambda 1")
+	defer func() {
+		test.Destroy(t)


Doesn't pulumitest auto destroy by default?

flostadler · 2024-12-12T20:33:35Z

examples/stable-outputs-preview/index.ts

+
+
+const bucket = new ccapi.s3.Bucket('bucket');
+const object = new aws.s3.BucketObjectv2(


Meta: Kind of frustrating that many rather basic use cases require layering aws classic on top of aws native in order to make it usable
:(

flostadler · 2024-12-12T20:36:31Z

examples/stable-outputs-preview/zip.ts

+// eslint-disable-next-line @typescript-eslint/no-var-requires
+const archiver = require('archiver');
+
+export function zipDirectory(directory: string, outputFile: string): Promise<string> {


Couldn't we do that by using an AssetArchive?

flostadler · 2024-12-12T20:46:52Z

provider/pkg/outputs/previewOutputs.go

+// - `aws-native:securityhub:PolicyAssociation` has a property `associationIdentifier`
+//   - TODO[pulumi/aws-native#1892]: in some cases this property is the `primaryIdentifier`. Could we use that as another heuristic?
+//     a readonly property that is also a primary identifier? It doesn't catch all cases, but would catch more
+func isStableOutput(propName string, resourceTypeName tokens.TypeName) bool {


flostadler · 2024-12-12T20:54:30Z

provider/pkg/resources/cfn_custom_resource.go

+
+// cfn custom resources have outputs returned in a "data" property
+// since it can be any arbitrary data, we mark the entire thing as computed
+func (c *cfnCustomResource) PreviewCustomResourceOutputs() resource.PropertyMap {


Small nit: this should be covered by a unit ideally

flostadler · 2024-12-12T20:54:47Z

provider/pkg/resources/custom.go

@@ -21,4 +21,6 @@ type CustomResource interface {
 	Update(ctx context.Context, urn resource.URN, id string, inputs, oldInputs, state resource.PropertyMap, timeout time.Duration) (resource.PropertyMap, error)
 	// Delete removes the resource from the cloud provider.
 	Delete(ctx context.Context, urn resource.URN, id string, inputs, state resource.PropertyMap, timeout time.Duration) error
+	// PreviewCustomResourceOutputs returns the outputs of the resource based on the inputs and the output properties in the resource schema.
+	PreviewCustomResourceOutputs() resource.PropertyMap


corymhall force-pushed the corymhall/stable-properties branch from c2f761c to bb662bd Compare December 6, 2024 18:00

corymhall mentioned this pull request Dec 6, 2024

Preview is not always correct for stable output properties #1892

Open

corymhall requested review from flostadler and t0yv0 December 6, 2024 18:10

flostadler reviewed Dec 9, 2024

View reviewed changes

t0yv0 reviewed Dec 9, 2024

View reviewed changes

provider/pkg/provider/provider.go Outdated Show resolved Hide resolved

t0yv0 reviewed Dec 9, 2024

View reviewed changes

provider/pkg/provider/previewOutputs.go Outdated Show resolved Hide resolved

t0yv0 reviewed Dec 9, 2024

View reviewed changes

provider/pkg/provider/previewOutputs.go Outdated Show resolved Hide resolved

t0yv0 reviewed Dec 10, 2024

View reviewed changes

provider/pkg/provider/previewOutputs_test.go Outdated Show resolved Hide resolved

t0yv0 reviewed Dec 10, 2024

View reviewed changes

provider/pkg/provider/previewOutputs.go Outdated Show resolved Hide resolved

t0yv0 reviewed Dec 10, 2024

View reviewed changes

provider/pkg/provider/previewOutputs.go Outdated Show resolved Hide resolved

t0yv0 reviewed Dec 10, 2024

View reviewed changes

provider/pkg/provider/previewOutputs_test.go Outdated Show resolved Hide resolved

corymhall changed the base branch from master to corymhall/read-only-metadata December 10, 2024 18:00

corymhall force-pushed the corymhall/stable-properties branch from 3288f39 to fc89719 Compare December 10, 2024 18:00

corymhall mentioned this pull request Dec 11, 2024

chore: add readOnlyProperties to metadata #1898

Merged

corymhall force-pushed the corymhall/stable-properties branch from a38f447 to 1f68e18 Compare December 11, 2024 15:18

Base automatically changed from corymhall/read-only-metadata to master December 11, 2024 15:33

corymhall added 4 commits December 11, 2024 10:37

remove package-lock file

ea2a53b

updating todo

20b9d99

update metadata.json

b539008

corymhall added 3 commits December 11, 2024 10:37

Updates based on review

db8ebd3

fix typo

85b7c41

fixing metadata.json

a4ee708

corymhall force-pushed the corymhall/stable-properties branch from 8b6a67d to a4ee708 Compare December 11, 2024 15:38

t0yv0 reviewed Dec 11, 2024

View reviewed changes

provider/pkg/provider/provider.go Outdated Show resolved Hide resolved

t0yv0 reviewed Dec 11, 2024

View reviewed changes

provider/pkg/provider/previewOutputs.go Outdated Show resolved Hide resolved

t0yv0 reviewed Dec 11, 2024

View reviewed changes

provider/pkg/provider/previewOutputs.go Outdated Show resolved Hide resolved

t0yv0 reviewed Dec 11, 2024

View reviewed changes

provider/pkg/provider/previewOutputs.go Outdated Show resolved Hide resolved

corymhall added 2 commits December 11, 2024 12:19

refactor preview outputs into it's own package

a05520a

- combine entrypoint into a single function for both create and update

additionalProperties will not have readOnly

784e3ad

corymhall requested review from flostadler and t0yv0 December 12, 2024 12:55

t0yv0 reviewed Dec 12, 2024

View reviewed changes

t0yv0 approved these changes Dec 12, 2024

View reviewed changes

flostadler approved these changes Dec 12, 2024

View reviewed changes

corymhall merged commit 6008c70 into master Dec 12, 2024
18 checks passed

corymhall deleted the corymhall/stable-properties branch December 12, 2024 20:35

flostadler approved these changes Dec 12, 2024

View reviewed changes

mjeffryes assigned corymhall Dec 17, 2024



		const bucket = new ccapi.s3.Bucket('bucket');
		const object = new aws.s3.BucketObjectv2(

Enable preview for create and update #1891

Enable preview for create and update #1891

Conversation

corymhall commented Dec 6, 2024 • edited Loading

github-actions bot commented Dec 6, 2024 • edited Loading

Does the PR have any schema changes?

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Dec 9, 2024 • edited Loading

t0yv0 commented Dec 10, 2024

t0yv0 commented Dec 10, 2024

t0yv0 commented Dec 10, 2024

corymhall commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 left a comment

Choose a reason for hiding this comment

flostadler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corymhall commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

t0yv0 Dec 9, 2024 •

edited

Loading

t0yv0 commented Dec 9, 2024 •

edited

Loading

corymhall commented Dec 10, 2024 •

edited

Loading