Correctly handle MaxItems: 1 for extractSchemaInputs #1812

iwahbe · 2024-03-29T15:54:48Z

This corrects MaxItems: 1 handling for extractSchemaInputs. As part of that, I have separated the handling of map<string,elem> and object types to their own function. This vastly simplifies the code.

Tests are incoming shortly.

codecov · 2024-03-29T15:58:43Z

Codecov Report

Attention: Patch coverage is 93.54839% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 60.19%. Comparing base (3a2ed13) to head (f8c2609).
Report is 20 commits behind head on master.

Files	Patch %	Lines
pkg/tfbridge/schema.go	93.54%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1812      +/-   ##
==========================================
- Coverage   60.60%   60.19%   -0.42%     
==========================================
  Files         303      310       +7     
  Lines       42348    42771     +423     
==========================================
+ Hits        25667    25745      +78     
- Misses      15208    15554     +346     
+ Partials     1473     1472       -1

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

pkg/tfbridge/schema.go

iwahbe · 2024-03-29T16:06:27Z

pkg/tfbridge/schema.go

+		// To match previous behavior, we insert the default key for Map types.
+		//
+		// TODO: We should probably remove the extraneous defaultsKey here.
 		v[defaultsKey] = resource.NewArrayProperty([]resource.PropertyValue{})


Before this PR, we didn't clearly distinguish between Map and Object very well, so we inserted the defaults list in all cases. To avoid changing a bunch of existing tests, I'm leaving the old behavior as is for now.

Map's can't have field defaults, so I'm pretty sure that this is wrong. Fixing is a followup PR job.

pkg/tfbridge/schema.go

t0yv0 · 2024-03-29T16:47:57Z

pkg/tfbridge/schema.go

+			tfs = e
+		case shim.Resource:
+			if state.IsObject() {
+				return resource.NewProperty(


should this be NewObjectProperty to be more type-precise (nit)

Can you clarify what you mean by "more type-precise". NewObjectProperty is more explicit, but I don't see why we shouldn't use type inference. NewProperty is fully type safe:

// PropertyValueType enumerates the actual types that may be stored in a PropertyValue. // //nolint:lll type PropertyValueType interface { bool | float64 | string | *asset.Asset | *archive.Archive | Computed | Output | *Secret | ResourceReference | []PropertyValue | PropertyMap } // NewProperty creates a new PropertyValue. func NewProperty[T PropertyValueType](v T) PropertyValue { return PropertyValue{v} }

Just out of habit, I'd use least general primitives, in case the underlying type switches to string, that's a bug. But it's purely theoretical so disregard the nit.

t0yv0 · 2024-03-29T16:51:36Z

Thanks for wrangling this, looking forward to the tests. Not the easiest code to work on.

Fixes pulumi/pulumi-cloudflare#554

pkg/tfbridge/schema_test.go

t0yv0 · 2024-03-29T17:39:37Z

pkg/tfbridge/schema.go

+		ps = &SchemaInfo{}
+	}
+
+	for IsMaxItemsOne(tfs, ps) {


It's fantastic to get quick fix out! IN the longer term maybe worth looking into abstracting out the traversal using path-based or some such methods, so the codebase deals with these idiosyncrasies in one place and gets it right once and for all.

Follow-up to #1812. - 6d6bb4f removes an unused parameter. - 26fc890 refactors `extractInputsObject` out of `extractInputs`, creating an explicit code path for objects. - 12618d5 removes an unused parameter. This area is under test in the bridge, making it relatively safe to refactor.

This resolves the immediate customer pain with #554. Fixes #554

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@pulumi/cloudflare](https://pulumi.io) ([source](https://togithub.com/pulumi/pulumi-cloudflare)) | dependencies | minor | [`5.23.0` -> `5.24.1`](https://renovatebot.com/diffs/npm/@pulumi%2fcloudflare/5.23.0/5.24.1) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>pulumi/pulumi-cloudflare (@pulumi/cloudflare)</summary> ### [`v5.24.1`](https://togithub.com/pulumi/pulumi-cloudflare/releases/tag/v5.24.1) [Compare Source](https://togithub.com/pulumi/pulumi-cloudflare/compare/v5.24.0...v5.24.1) #### Changelog - [`b2c32fe`](https://togithub.com/pulumi/pulumi-cloudflare/commit/b2c32fe7) Bump google.golang.org/protobuf in /examples/record/go - [`bcb25f7`](https://togithub.com/pulumi/pulumi-cloudflare/commit/bcb25f74) Combined dependencies PR ([#691](https://togithub.com/pulumi/pulumi-cloudflare/issues/691)) - [`085504b`](https://togithub.com/pulumi/pulumi-cloudflare/commit/085504bf) Include [pulumi/pulumi-terraform-bridge#1812](https://togithub.com/pulumi/pulumi-terraform-bridge/issues/1812) ([#708](https://togithub.com/pulumi/pulumi-cloudflare/issues/708)) - [`06772bf`](https://togithub.com/pulumi/pulumi-cloudflare/commit/06772bf8) Merge remote-tracking branch 'origin/dependabot/go_modules/examples/record/go/google.golang.org/protobuf-1.33.0' into combined-pr-branch - [`b3b3a65`](https://togithub.com/pulumi/pulumi-cloudflare/commit/b3b3a658) Upgrade pulumi-terraform-bridge to v3.79.0 ([#702](https://togithub.com/pulumi/pulumi-cloudflare/issues/702)) ### [`v5.24.0`](https://togithub.com/pulumi/pulumi-cloudflare/releases/tag/v5.24.0) [Compare Source](https://togithub.com/pulumi/pulumi-cloudflare/compare/v5.23.0...v5.24.0) #### Changelog - [`5c0da33`](https://togithub.com/pulumi/pulumi-cloudflare/commit/5c0da335) Mark `cloudflare:Record` as `DeleteBeforeReplace: true` ([#693](https://togithub.com/pulumi/pulumi-cloudflare/issues/693)) - [`19bc5a0`](https://togithub.com/pulumi/pulumi-cloudflare/commit/19bc5a0d) Remove unused field mappings ([#695](https://togithub.com/pulumi/pulumi-cloudflare/issues/695)) - [`4748d37`](https://togithub.com/pulumi/pulumi-cloudflare/commit/4748d37e) Update GitHub Actions workflows. ([#698](https://togithub.com/pulumi/pulumi-cloudflare/issues/698)) - [`1b5e3ce`](https://togithub.com/pulumi/pulumi-cloudflare/commit/1b5e3ce3) Upgrade terraform-provider-cloudflare to v4.28.0 ([#697](https://togithub.com/pulumi/pulumi-cloudflare/issues/697)) - [`64ff681`](https://togithub.com/pulumi/pulumi-cloudflare/commit/64ff681a) falseUpdate GitHub Actions workflows. ([#694](https://togithub.com/pulumi/pulumi-cloudflare/issues/694)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>

iwahbe requested review from t0yv0 and VenelinMartinov March 29, 2024 15:54

iwahbe self-assigned this Mar 29, 2024

iwahbe force-pushed the iwahbe/cloudflare-554/max-item-one-handling-for-extract-schema-inputs branch from 8ffcc83 to 5e453f9 Compare March 29, 2024 16:02

iwahbe commented Mar 29, 2024

View reviewed changes

t0yv0 reviewed Mar 29, 2024

View reviewed changes

pkg/tfbridge/schema.go Outdated Show resolved Hide resolved

t0yv0 reviewed Mar 29, 2024

View reviewed changes

pkg/tfbridge/schema.go Show resolved Hide resolved

t0yv0 reviewed Mar 29, 2024

View reviewed changes

iwahbe force-pushed the iwahbe/cloudflare-554/max-item-one-handling-for-extract-schema-inputs branch from 459d269 to 39d4dc9 Compare March 29, 2024 17:22

iwahbe added 2 commits March 29, 2024 18:23

Correctly handle MaxItems: 1 for extractSchemaInputs

263c6e7

Fixes pulumi/pulumi-cloudflare#554

Add tests

bc6f82a

iwahbe force-pushed the iwahbe/cloudflare-554/max-item-one-handling-for-extract-schema-inputs branch from 39d4dc9 to bc6f82a Compare March 29, 2024 17:23

Add comment as described in PR review.

c3c6712

iwahbe requested a review from t0yv0 March 29, 2024 17:26

t0yv0 reviewed Mar 29, 2024

View reviewed changes

pkg/tfbridge/schema_test.go Outdated Show resolved Hide resolved

t0yv0 reviewed Mar 29, 2024

View reviewed changes

pkg/tfbridge/schema_test.go Show resolved Hide resolved

t0yv0 reviewed Mar 29, 2024

View reviewed changes

t0yv0 approved these changes Mar 29, 2024

View reviewed changes

iwahbe enabled auto-merge (squash) April 1, 2024 09:48

iwahbe disabled auto-merge April 1, 2024 09:48

iwahbe enabled auto-merge (squash) April 1, 2024 09:48

implement test in sdkv2

f8c2609

iwahbe force-pushed the iwahbe/cloudflare-554/max-item-one-handling-for-extract-schema-inputs branch from d935eba to f8c2609 Compare April 1, 2024 10:05

iwahbe merged commit b9afd77 into master Apr 1, 2024
9 checks passed

iwahbe deleted the iwahbe/cloudflare-554/max-item-one-handling-for-extract-schema-inputs branch April 1, 2024 10:21

iwahbe mentioned this pull request Apr 1, 2024

Remove rawNames from extractInputs and extractSchemaInputs #1820

Merged

iwahbe added a commit to pulumi/pulumi-cloudflare that referenced this pull request Apr 2, 2024

Include pulumi/pulumi-terraform-bridge#1812

f16b4b9

This resolves the immediate customer pain with #554. Fixes #554

iwahbe added a commit to pulumi/pulumi-cloudflare that referenced this pull request Apr 2, 2024

Include pulumi/pulumi-terraform-bridge#1812 (#708)

085504b

This resolves the immediate customer pain with #554. Fixes #554

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly handle MaxItems: 1 for extractSchemaInputs #1812

Correctly handle MaxItems: 1 for extractSchemaInputs #1812

iwahbe commented Mar 29, 2024 •

edited

codecov bot commented Mar 29, 2024 •

edited

iwahbe Mar 29, 2024

t0yv0 Mar 29, 2024

iwahbe Mar 29, 2024

t0yv0 Mar 29, 2024

t0yv0 commented Mar 29, 2024

t0yv0 Mar 29, 2024

Correctly handle MaxItems: 1 for extractSchemaInputs #1812

Correctly handle MaxItems: 1 for extractSchemaInputs #1812

Conversation

iwahbe commented Mar 29, 2024 • edited

codecov bot commented Mar 29, 2024 • edited

Codecov Report

iwahbe Mar 29, 2024

Choose a reason for hiding this comment

t0yv0 Mar 29, 2024

Choose a reason for hiding this comment

iwahbe Mar 29, 2024

Choose a reason for hiding this comment

t0yv0 Mar 29, 2024

Choose a reason for hiding this comment

t0yv0 commented Mar 29, 2024

t0yv0 Mar 29, 2024

Choose a reason for hiding this comment

iwahbe commented Mar 29, 2024 •

edited

codecov bot commented Mar 29, 2024 •

edited