Use OpenTofu planner with Plugin Framework providers #2188

t0yv0 · 2024-07-16T18:30:11Z

OpenTofu planner code is now used with Plugin Framework providers to emulate Terraform CLI behavior for objchange.ProposedNew. Note that this code was already in use for SDKv2-based resources prior to the change. The vendored code replaces a custom implementation that is now removed.

Fixes #2172

Observable changes: Plugin Framework based resources may now reorder outputs for Set collections.

This change is known to fix bugs such as #2192 - to be confirmed with a regression test in a follow-up PR.

pf/internal/pfutils/proposed_new.go

t0yv0 · 2024-07-24T23:33:13Z

So looks like I was misled on this change https://github.com/pulumi/pulumi-terraform-bridge/pull/2060/files#diff-4f84cec74d4ce22a6ead8a99011c4a9d47bbed3b56a5e7382b12aeda28cf05e6R78 @VenelinMartinov - the branching is not present in OpenTOFU sources. So re-vendoring the code from OpenTOFU now leads to failing tests. That's not a good state to be in.

iwahbe

I'd feel more comfortable if the vendored code lived in a pkg/vendor directory, just so it's clearer which code is vendored.

Can you add to the README.mds why each package is vendored? In general, I'm not super comfortable dropping down to vendored code unless we absolutely need to. I'd like to make sure that if we do need to do so, we have the reasons clearly listed so we know what we need to do to go back to standard go modules.

t0yv0 · 2024-07-25T18:03:09Z

vendor has a special meaning in Go tooling and activates that handling which I couldn't quite figure out. I can look into renaming it to vendored or something similar.

iwahbe · 2024-07-25T18:27:18Z

vendor has a special meaning in Go tooling and activates that handling which I couldn't quite figure out. I can look into renaming it to vendored or something similar.

🤦 Of course it does. vendored works just as well for my purposes.

codecov · 2024-07-25T19:29:37Z

Codecov Report

Attention: Patch coverage is 15.71429% with 354 lines in your changes missing coverage. Please review.

Project coverage is 56.66%. Comparing base (59fe4bf) to head (3f907fb).
Report is 1 commits behind head on master.

Files	Patch %	Lines
pkg/vendored/opentofu/convert/schema.go	0.00%	212 Missing ⚠️
...ed/terraform-plugin-go/tfprotov6/toproto/schema.go	0.00%	83 Missing ⚠️
pf/internal/pfutils/convert_rschema_to_proto.go	57.77%	15 Missing and 4 partials ⚠️
...aform-plugin-go/tfprotov6/toproto/dynamic_value.go	0.00%	17 Missing ⚠️
pf/internal/pfutils/proposed_new.go	70.00%	7 Missing and 5 partials ⚠️
pf/internal/pfutils/schema.go	66.66%	6 Missing ⚠️
pf/proto/runtypes.go	0.00%	3 Missing ⚠️
...rraform-plugin-go/tfprotov6/toproto/string_kind.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2188      +/-   ##
==========================================
- Coverage   60.68%   56.66%   -4.03%     
==========================================
  Files         356      361       +5     
  Lines       46451    49443    +2992     
==========================================
- Hits        28189    28015     -174     
- Misses      16704    19891    +3187     
+ Partials     1558     1537      -21

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

t0yv0 · 2024-07-25T19:46:42Z

dynamic/testdata/TestPrimitiveTypes/diff(some).golden

@@ -2,6 +2,7 @@
  "changes": "DIFF_SOME",
  "diffs": [
    "attrNumberComputed",
+    "attrNumberRequired",


I think this is a correct result, this used to be a bug. Likely a degenerate case because a required property cannot be not specified in the program.

t0yv0 · 2024-07-25T19:47:56Z

pf/internal/pfutils/convert_rschema_to_proto.go

+)
+
+// Assist converting Plugin framework schemata to proto schemata for a resource.
+func convertResourceSchemaToProto(ctx context.Context, s *rschema.Schema) (*tfprotov6.Schema, error) {


This is a slightly convoluted way to convert rschema.Schema to protos. Perhaps could be avoided by vendoring internal code from the Plugin framework.

This is indeed quite roundabout - could we add a comment with a TODO here?

I think I don't intend revisiting though, I think alternatives are worse.

t0yv0 · 2024-07-25T19:48:34Z

pf/internal/pfutils/proposed_new_test.go

 						},
 						Optional: true,
 					},
 					"list": rschema.ListNestedAttribute{
 						NestedObject: rschema.NestedAttributeObject{
 							Attributes: map[string]rschema.Attribute{
-								"bar": rschema.StringAttribute{},
+								"bar": rschema.StringAttribute{Optional: true},


These cases started failing because the schema was not valid.

VenelinMartinov

Thanks for updating the dependencies here!

VenelinMartinov · 2024-07-26T08:18:11Z

pf/internal/pfutils/convert_rschema_to_proto.go

+)
+
+// Assist converting Plugin framework schemata to proto schemata for a resource.
+func convertResourceSchemaToProto(ctx context.Context, s *rschema.Schema) (*tfprotov6.Schema, error) {


This is indeed quite roundabout - could we add a comment with a TODO here?

VenelinMartinov · 2024-07-26T08:23:00Z

pf/internal/pfutils/proposed_new_test.go

+			// TODO[pulumi/pulumi-terraform-bridge#2247]: patching objchange.ProposedNew changed behavior
+			//
+			// Want:   object(map[string]valueBuilder{"foo": prim(nil)}),
+			Want: unk(),


These all look "more correct" here in the absence of a planmodifier. Good to see we don't have any regressions with the objchange patch in pf

Yeah well maybe, these tests are a bit too low level to call which one we want.

VenelinMartinov · 2024-07-26T08:25:35Z

pkg/vendored/opentofu/README.md

@@ -0,0 +1,3 @@
+# opentofu
+
+Code vendored from [opentofu](https://github.com/opentofu/opentofu). Do not edit, use `go generate` to update.


Should we add a CI check for vendored code to avoid any accidents here? Run go generate, check repo is clean. Fine as a follow-up.

Rebased and tidied up #2168 Fixes #2170 This change confirms that the bug discovered in pulumi/pulumi-meraki#97 is now fixed in the latest bridge via #2188 --------- Co-authored-by: Venelin <[email protected]>

t0yv0 requested review from iwahbe, VenelinMartinov and guineveresaenger July 16, 2024 19:53

t0yv0 commented Jul 16, 2024

View reviewed changes

pf/internal/pfutils/proposed_new.go Outdated Show resolved Hide resolved

This was referenced Jul 17, 2024

PF Computed Set does not trigger diff when an element is removed #2192

Closed

PF wrong diff for set of objects #2170

Closed

mjeffryes assigned t0yv0 Jul 19, 2024

mjeffryes modified the milestone: 0.107 Jul 24, 2024

t0yv0 force-pushed the t0yv0/remove-custom-objchange branch from bd2157d to 01e1cde Compare July 24, 2024 17:55

VenelinMartinov mentioned this pull request Jul 25, 2024

chore: make the patch to objchange.ProposedNew explicit #2246

Merged

t0yv0 mentioned this pull request Jul 25, 2024

Computed set no diff 2192 repro #2193

Closed

t0yv0 added 14 commits July 25, 2024 12:58

Toward removing custom objchange implementation

081d57b

Vendor files for parsing schema blocks from proto

485fd18

Tidy code-generation

4bdf139

Vendor tfprotov6/toproto code

34b9e26

Fill out convertBlock

df689b5

Fill in the missing tfprotov6.Schema conversions

758aa9a

Vendor and rebuild tfplugin6 protos

3828837

Clean up pkg/opentofu vendored code

c064bd4

Clean up tfprotov6 vendored code

f14431b

Simplify proposed_new conversions

351e4e8

Disambiguate the protos further

673a096

Actually fix the proto conflict

24042b6

Fix test cases that had invalid schemata

028127e

Lint corrections

15dbda0

t0yv0 force-pushed the t0yv0/remove-custom-objchange branch from 8a39d89 to 15dbda0 Compare July 25, 2024 16:58

t0yv0 added 2 commits July 25, 2024 13:02

Rerun opentofu go generate

f395b51

Lint auto-fixes

4fbd1d7

iwahbe reviewed Jul 25, 2024

View reviewed changes

Explain changes to test expectations.

4007cff

t0yv0 added 4 commits July 25, 2024 15:02

Accept set reordering

9d03fe8

Another instance of newly introduced set reordering

5bf30d0

Fix TestPrimitiveTypes/diff(some).golden

63ba4ac

Move tfplugin6 under vendored

7dea5df

t0yv0 added 7 commits July 25, 2024 15:30

Move to vendored/tfplugin6

89eac4c

Adjust references

b26ab85

Move more vendored code

ffbd78d

Fix compilation

c200e72

Fix PF compilation

457e48a

README.md

1574963

Adjust linter settings

3f907fb

t0yv0 commented Jul 25, 2024

View reviewed changes

t0yv0 changed the title ~~Toward removing custom objchange implementation~~ Use OpenTofu planner with Plugin Framework providers Jul 25, 2024

t0yv0 marked this pull request as ready for review July 25, 2024 20:10

t0yv0 requested review from iwahbe and corymhall July 25, 2024 20:10

VenelinMartinov approved these changes Jul 26, 2024

View reviewed changes

t0yv0 merged commit 7f2e4c0 into master Jul 26, 2024
13 checks passed

t0yv0 deleted the t0yv0/remove-custom-objchange branch July 26, 2024 13:16

guineveresaenger mentioned this pull request Jul 26, 2024

ci: add a check that go generate does not edit sources #2256

Open

t0yv0 mentioned this pull request Aug 9, 2024

Test a special set diff case #2304

Merged

mjeffryes added this to the 0.108 milestone Aug 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use OpenTofu planner with Plugin Framework providers #2188

Use OpenTofu planner with Plugin Framework providers #2188

t0yv0 commented Jul 16, 2024 •

edited

Loading

t0yv0 commented Jul 24, 2024

iwahbe left a comment

t0yv0 commented Jul 25, 2024

iwahbe commented Jul 25, 2024

codecov bot commented Jul 25, 2024 •

edited

Loading

t0yv0 Jul 25, 2024

t0yv0 Jul 25, 2024

VenelinMartinov Jul 26, 2024

t0yv0 Jul 26, 2024

t0yv0 Jul 25, 2024

VenelinMartinov left a comment

VenelinMartinov Jul 26, 2024

VenelinMartinov Jul 26, 2024

t0yv0 Jul 26, 2024

VenelinMartinov Jul 26, 2024

t0yv0 Jul 26, 2024

		@@ -0,0 +1,3 @@
		# opentofu

		Code vendored from [opentofu](https://github.com/opentofu/opentofu). Do not edit, use `go generate` to update.

Use OpenTofu planner with Plugin Framework providers #2188

Use OpenTofu planner with Plugin Framework providers #2188

Conversation

t0yv0 commented Jul 16, 2024 • edited Loading

t0yv0 commented Jul 24, 2024

iwahbe left a comment

Choose a reason for hiding this comment

t0yv0 commented Jul 25, 2024

iwahbe commented Jul 25, 2024

codecov bot commented Jul 25, 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

VenelinMartinov 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

t0yv0 commented Jul 16, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading