Cleanup Empty AutoAliasing Objects #1064

iwahbe · 2023-05-01T21:11:31Z

This PR modifies token mapping to not add extra fields for otherwise empty objects. This is necessary because AutoName sets a full tfbridge.SchemaInfo, it does not modify one.

github-actions · 2023-05-01T21:15:01Z

Diff for pulumi-azuread with merge commit 16a4ff2

github-actions · 2023-05-01T21:15:21Z

Diff for pulumi-random with merge commit 16a4ff2

github-actions · 2023-05-01T21:29:37Z

Diff for pulumi-gcp with merge commit 16a4ff2

github-actions · 2023-05-01T21:40:03Z

Diff for pulumi-azure with merge commit 16a4ff2

t0yv0 · 2023-05-01T22:03:39Z

How does this connect to the user problem prompting this PR, would it be possible to elaborate? The tests are at a level of indirection, no elucidating new tests are added, and it's not obvious how the problem is being fixed.

I believe you will recover old behavior by putting p.SetAutonaming before the call to x.AutoAliasing.

After this PR do we believe that the order of p.SetAutonaming and x.AutoAliasing does not matter anymore?

Does editing the existing test not weaken the MaxItems one test strength?

I'm going to be able to review tomorrow morning but it's not an obvious fix to review in a tired state.

guineveresaenger

I would like some comments that describe the recursion pattern, and the purpose of each inline function.
Can we also link the Docker PR that failed, with an explanation of why that happened?

Thank you for the quick turnaround.

pkg/tfbridge/x/token_test.go

guineveresaenger · 2023-05-01T22:25:19Z

pkg/tfbridge/x/tokens.go

 	}

-	var walk func(*fieldHistory, *b.SchemaInfo, shim.Schema)
-	walk = func(h *fieldHistory, info *b.SchemaInfo, schema shim.Schema) {
+	walk = func(h *fieldHistory, info *b.SchemaInfo, schema shim.Schema) (hasH bool, hasI bool) {


I think there is a lot of state context here that gets confusing because the variable names are so similar.
For example, we return hasH and hasI here but on line 667 we assign them to keepH and keepI and it would be great to know what is prompting us to "keep" info or history.

I added that comment.

guineveresaenger · 2023-05-01T22:27:34Z

pkg/tfbridge/x/tokens.go

-			f(k, v)
+			h, hasH := getNonNil(hist, k)
+			i, hasI := getNonNil(info, k)
+			keepH, keepI := walk(h, i, v)


I'm not sure what is being kept here.

Should we keep history/info?
Are we checking whether history or info are being kept?

We should keep it when (1) it already existed or (2) we stored useful information in it. I added some comments explaining what we are trying to do that will hopefully clarify.

pkg/tfbridge/x/tokens.go

github-actions · 2023-05-01T23:20:39Z

Diff for pulumi-azuread with merge commit dd58726

github-actions · 2023-05-01T23:21:47Z

Diff for pulumi-random with merge commit dd58726

github-actions · 2023-05-01T23:28:46Z

Diff for pulumi-gcp with merge commit dd58726

github-actions · 2023-05-01T23:37:42Z

Diff for pulumi-azure with merge commit dd58726

github-actions · 2023-05-02T00:08:13Z

Diff for pulumi-azuread with merge commit 1a05aa0

github-actions · 2023-05-02T00:09:00Z

Diff for pulumi-random with merge commit 1a05aa0

github-actions · 2023-05-02T00:15:19Z

Diff for pulumi-azuread with merge commit 4c501ad

github-actions · 2023-05-02T00:15:42Z

Diff for pulumi-azuread with merge commit 5ea397d

github-actions · 2023-05-02T00:15:45Z

Diff for pulumi-random with merge commit 4c501ad

github-actions · 2023-05-02T00:16:02Z

Diff for pulumi-random with merge commit 5ea397d

github-actions · 2023-05-02T00:20:43Z

Diff for pulumi-gcp with merge commit 1a05aa0

iwahbe · 2023-05-02T00:22:54Z

How does this connect to the user problem prompting this PR, would it be possible to elaborate? The tests are at a level of indirection, no elucidating new tests are added, and it's not obvious how the problem is being fixed.
I believe you will recover old behavior by putting p.SetAutonaming before the call to x.AutoAliasing.
After this PR do we believe that the order of p.SetAutonaming and x.AutoAliasing does not matter anymore?

Does editing the existing test not weaken the MaxItems one test strength?

I'm going to be able to review tomorrow morning but it's not an obvious fix to review in a tired state.

After this PR we do believe that p.SetAutonaming and x.AutoAliasing should be callable in any order.¹ We want this property to avoid confusion.

The test was more specific than necessary. Specifically, it was testing for a property that was not necessary for the desired behavior that we want to change:

- assert.Nil(t, info.Resources["pkg_r1"].Fields["f2"].MaxItemsOne)
+ assert.Nil(t, info.Resources["pkg_r1"].Fields["f2"])

Previously, info.Resources["pkg_r1"].Fields["f2"] was equal to &tfbridge.SchemaInfo{}, and we only cared that info.Resources["pkg_r1"].Fields["f2"].MaxItemsOne was nil. We are tightening the requirements to ensure that the full thing is nil.

What we actually cared about is this property (expressed in TS style pseudo-code):

info?.Resources?["pkg_r1"]?.Fields?["f2"]?.MaxItemsOne == nil

Because of how painful it is to express this in go, I abbreviated to asserting MaxItemsOne was nil.

I believe that SetAutonaming will break at runtime when it is applied to a non-string field, so I'm discounting the possibility of a list type with auto-naming called name. ↩

iwahbe · 2023-05-02T00:23:55Z

pkg/tfbridge/x/token_test.go

+// (*ProviderInfo).SetAutonaming only applies to fields that are not present in their
+// resource's property map. We need to make sure that unless we mark a field as
+// `MaxItemsOne: nonNil` for some non-nil value, we don't leave that field entry behind
+// since that will disable SetAutonaming.
+func TestMaxItemsOneAliasingWithAutoNaming(t *testing.T) {


@guineveresaenger @t0yv0 Sorry for the confusing PR. I have added our motivating example as a test case with an explanation of the underlying logic for the change.

Brilliant. Oof, so SetAutonaming does look at whether the SchemaInfo is present or no.

Saw this code:

if _, hasfield := res.Fields[nameProperty]; !hasfield { if res.Fields == nil { res.Fields = make(map[string]*SchemaInfo) } res.Fields[nameProperty] = AutoName(nameProperty, maxLength, separator) }

I would rephrase

(*ProviderInfo).SetAutonaming only applies to fields that are not present in their
// resource's property map.

With

(*ProviderInfo).SetAutonaming skips fields that have a SchemaInfo already defined in ResourceInfo.Fields

I always confuse the Info things with he provider maps from shim.SchemaMap.

github-actions · 2023-05-02T00:24:44Z

Diff for pulumi-azure with merge commit 1a05aa0

github-actions · 2023-05-02T00:25:43Z

Diff for pulumi-gcp with merge commit 4c501ad

github-actions · 2023-05-02T00:28:17Z

Diff for pulumi-gcp with merge commit 5ea397d

github-actions · 2023-05-02T00:31:53Z

Diff for pulumi-azure with merge commit 4c501ad

github-actions · 2023-05-02T00:33:00Z

Diff for pulumi-azure with merge commit 5ea397d

github-actions · 2023-05-02T00:39:26Z

Diff for pulumi-azuread with merge commit 78dbc7b

github-actions · 2023-05-02T00:39:44Z

Diff for pulumi-random with merge commit 78dbc7b

github-actions · 2023-05-02T00:50:48Z

Diff for pulumi-gcp with merge commit 78dbc7b

github-actions · 2023-05-02T00:55:53Z

Diff for pulumi-azure with merge commit 78dbc7b

t0yv0 · 2023-05-02T20:25:55Z

Because of how painful it is to express this in go, I abbreviated to asserting MaxItemsOne was nil.

Yes that's true. Yes. I think we try not to care anywhere about the different flavors of "missing", not to attach semantics to nil vs missing key here, so what could work is getter functions that isolate the caller from nils, like this:

assert.Nil(t, info.Resource("pkg_r1").Field("f2").MaxItemsOne)

But perhaps for another day.

pkg/tfbridge/x/tokens.go

t0yv0 · 2023-05-02T20:52:56Z

Well this is interesting, it works around the immediate problem but looks a tad error prone. I'm a bit worried about this quirk we found in SetAutonaming behavior, and perhaps there's still conditions where the order of application matters.

Consider this:

autoaliasing runs and injects a MaxItems=1 specification, creating a SchemaInfo in ResourceInfo.Fields
then autonaming runs and finds that whoops, the user set ResourceInfo.Fields["spock"] so I'm going to skip it

To the end user this looks like we accidentally disabled auto naming.

Could we do better? Could we enforce the order so SetAutonaming is always before aliasing?

jazzyfresh

LGTM, especially with the new test

github-actions · 2023-05-02T21:38:37Z

Diff for pulumi-random with merge commit 07fcd2f

github-actions · 2023-05-02T21:39:08Z

Diff for pulumi-azuread with merge commit 07fcd2f

github-actions · 2023-05-02T21:49:12Z

Diff for pulumi-gcp with merge commit 07fcd2f

iwahbe · 2023-05-02T21:53:50Z

Well this is interesting, it works around the immediate problem but looks a tad error prone. I'm a bit worried about this quirk we found in SetAutonaming behavior, and perhaps there's still conditions where the order of application matters.

Unfortunately, I think the quirk is here to stay. I've seen plenty of tk: { Fields: map[string]*tfbridge.SchemaInfo{ "name": {Name: "name" } } }. AFAIK, this exists only to disable auto-naming.

AutoNaming should have been more selective by checking if *SchemaInfo.Defaults was nil instead of *SchemaInfo, but I think that change needs to wait for bridge v4.0.0.

Consider this:

autoaliasing runs and injects a MaxItems=1 specification, creating a SchemaInfo in ResourceInfo.Fields

then autonaming runs and finds that whoops, the user set ResourceInfo.Fields["spock"] so I'm going to skip it

To the end user this looks like we accidentally disabled auto naming.

If AutoAliasing saw a resource and injected ResourceInfo["spock"].MaxItemsOne = True(), then SetAutonames would not be affected, since it only effects fields called "name".

If AutoAliasing saw a resource and injected ResourceInfo["name"].MaxItemsOne = True(), it would imply that ResourceMap.Get(resourceName).Schema().Get("name").Type is either a List, Map or Set (something that MaxItemsOne makes sense on). If that is the case, then SetAutonaming would be invalid, since it assumes that the name field is of type String.

The end user will never observe AutoAliasing disabling a valid auto name.

Could we do better? Could we enforce the order so SetAutonaming is always before aliasing?

We could enforce the ordering, but I don't think we should.

We impose accidental complexity on our users. Since we can't order enforce in the type system, this is a runtime panic when developing a provider. We should avoid this where possible.
This cuts down the size of bridge-metadata.json to only keep the data it needs. This makes diffs much more meaningful.

github-actions · 2023-05-02T22:01:03Z

Diff for pulumi-azure with merge commit 07fcd2f

t0yv0 · 2023-05-02T22:28:15Z

Brilliant, so the cases don't overlap based on type. Autonaming is interested in name type=string and maxitems=1 aliasing is looking at type=list[x] or type=set[x]. That's great.

AutoNaming should have been more selective by checking if *SchemaInfo.Defaults was nil instead of *SchemaInfo, but I think that change needs to wait for bridge v4.0.0.

I like that. Care to backlog and label say impact/breaking? We can then find all those things when contemplating 4.0.

guineveresaenger

thank you for the extra comments and improved varnames! 💟

iwahbe · 2023-05-02T23:02:01Z

Brilliant, so the cases don't overlap based on type. Autonaming is interested in name type=string and maxitems=1 aliasing is looking at type=list[x] or type=set[x]. That's great.

AutoNaming should have been more selective by checking if *SchemaInfo.Defaults was nil instead of *SchemaInfo, but I think that change needs to wait for bridge v4.0.0.

I like that. Care to backlog and label say impact/breaking? We can then find all those things when contemplating 4.0.

Follow up issue is #1069.

iwahbe self-assigned this May 1, 2023

iwahbe added a commit to pulumi/pulumi-docker that referenced this pull request May 1, 2023

Pin to pulumi/pulumi-terraform-bridge#1064

c513234

iwahbe mentioned this pull request May 1, 2023

Upgrade pulumi terraform bridge to v3.45.3 pulumi/pulumi-docker#617

Closed

iwahbe requested review from jazzyfresh and guineveresaenger May 1, 2023 21:20

guineveresaenger reviewed May 1, 2023

View reviewed changes

Simplify Internal API

45502e3

iwahbe force-pushed the iwahbe/auto-aliasing-cleanup branch from 5a895da to ecab2c4 Compare May 2, 2023 00:11

iwahbe commented May 2, 2023

View reviewed changes

iwahbe force-pushed the iwahbe/auto-aliasing-cleanup branch from ecab2c4 to 2b93875 Compare May 2, 2023 00:35

iwahbe requested a review from guineveresaenger May 2, 2023 16:27

t0yv0 reviewed May 2, 2023

View reviewed changes

pkg/tfbridge/x/tokens.go Outdated Show resolved Hide resolved

jazzyfresh approved these changes May 2, 2023

View reviewed changes

Don't create extra elements

7747fa1

iwahbe force-pushed the iwahbe/auto-aliasing-cleanup branch from 2b93875 to 7747fa1 Compare May 2, 2023 21:34

t0yv0 self-requested a review May 2, 2023 22:31

t0yv0 approved these changes May 2, 2023

View reviewed changes

guineveresaenger approved these changes May 2, 2023

View reviewed changes

iwahbe mentioned this pull request May 2, 2023

SetAutonaming based on .DefaultInto fields #1069

Open

iwahbe merged commit 753c829 into master May 2, 2023
8 checks passed

iwahbe deleted the iwahbe/auto-aliasing-cleanup branch May 2, 2023 23:02

iwahbe added a commit to pulumi/pulumi-docker that referenced this pull request May 2, 2023

Pin to pulumi/pulumi-terraform-bridge#1064

ad4435e

iwahbe mentioned this pull request May 3, 2023

Upgrade terraform-provider-spotinst to v1.115.0 pulumi/pulumi-spotinst#310

Merged

t0yv0 mentioned this pull request May 4, 2023

Upgrade terraform-provider-fastly to v4.3.2 pulumi/pulumi-fastly#252

Closed

aq17 mentioned this pull request May 4, 2023

Upgrade terraform-provider-mongodbatlas to v1.9.0 pulumi/pulumi-mongodbatlas#214

Closed

Cleanup Empty AutoAliasing Objects #1064

Cleanup Empty AutoAliasing Objects #1064

Conversation

iwahbe commented May 1, 2023 • edited

github-actions bot commented May 1, 2023

github-actions bot commented May 1, 2023

github-actions bot commented May 1, 2023

github-actions bot commented May 1, 2023

t0yv0 commented May 1, 2023

guineveresaenger left a comment

Choose a reason for hiding this comment

guineveresaenger May 1, 2023

Choose a reason for hiding this comment

iwahbe May 2, 2023

Choose a reason for hiding this comment

guineveresaenger May 1, 2023

Choose a reason for hiding this comment

iwahbe May 2, 2023

Choose a reason for hiding this comment

github-actions bot commented May 1, 2023

github-actions bot commented May 1, 2023

github-actions bot commented May 1, 2023

github-actions bot commented May 1, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

iwahbe commented May 2, 2023

Footnotes

iwahbe May 2, 2023

Choose a reason for hiding this comment

t0yv0 May 2, 2023

Choose a reason for hiding this comment

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

t0yv0 commented May 2, 2023

t0yv0 commented May 2, 2023

jazzyfresh left a comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

iwahbe commented May 2, 2023

github-actions bot commented May 2, 2023

t0yv0 commented May 2, 2023

guineveresaenger left a comment

Choose a reason for hiding this comment

iwahbe commented May 2, 2023

iwahbe commented May 1, 2023 •

edited