Refactor makeTerraformInputs and friends

[VenelinMartinov]

This PR refactors the various functions related to converting pulumi inputs to TF inputs.

It also exposes a new option when making inputs: EnableMaxItemsOneDefaults which will allow us to fix pulumi/pulumi-aws#3427 once we can properly test the changes.

Opted not to land the behavioural changes for EnableMaxItemsOneDefaults yet because of the multiple rounds of varied downstream regressions - seems like we need to add a framework for testing against the TF behaviour in order to land this.

[codecov]

Codecov Report

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

Project coverage is 59.45%. Comparing base (54802d5) to head (09de622).
Report is 3 commits behind head on master.

Files	Patch %	Lines
pkg/tfbridge/provider.go	43.47%	12 Missing and 1 partial ⚠️
pkg/tfbridge/schema.go	91.66%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1725      +/-   ##
==========================================
- Coverage   59.79%   59.45%   -0.35%     
==========================================
  Files         300      309       +9     
  Lines       42025    42511     +486     
==========================================
+ Hits        25129    25274     +145     
- Misses      15474    15810     +336     
- Partials     1422     1427       +5     

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

[VenelinMartinov]

The downstream checks revealed at least 2 new issues, which weren't known before, perhaps due to #1730

https://github.com/pulumi/pulumi-vault/actions/runs/8159199780/job/22303439815?pr=429
https://github.com/pulumi/pulumi-azure/actions/runs/8158876277/job/22303367852?pr=1827

[t0yv0]

This is great, thank you very much. Conceptually this makes a lot of sense to me - the missing MaxItems=1 properties should be presenting as empty collections to the Terraform provider but not to the provider's validators. We have a slightly convoluted interaction with these validators already so the fix has to accommodate. This is really driving toward the correct behavior though! Let's do a pass to try to increase the longevity of the tests so we can still keep these tests around and understand what they're testing as the implementation itself evolves, this is an important property - left some style nits and suggestions, otherwise LGTM.

[iwahbe]

I agree with all of t0yv0's comments.

[VenelinMartinov]

From https://github.com/pulumi/terraform-plugin-sdk/blob/7ac578ce47fc07e0888beee6d4ab9db09369274f/helper/schema/schema.go#L78-L96:

If Elem is *schema.Resource then setting ConfigMode to SchemaConfigModeAttr will force it to be represented in configuration as an attribute, which means that the Computed flag can be used to provide default elements when the argument isn't set at all, while still allowing the user to force zero elements by explicitly assigning an empty list.

I have a feeling this is used to set a default for the rulesets and that's the reason TF does not error out here. The resource https://github.com/hashicorp/terraform-provider-azurerm/blob/61ac20e312db3e460ccb2324fdc04bc9eb2e503e/internal/services/eventhub/eventhub_namespace_resource.go#L128
has ConfigMode: pluginsdk.SchemaConfigModeAttr.

Note: I looked for the combination of attributes

	Computed:   true,
	MaxItems:   1,
	ConfigMode: pluginsdk.SchemaConfigModeAttr,

It looks like it's used in a few places in Azure but SchemaConfigModeAttr is virtually unused in AWS.

Will do a more in-depth comparison of the TF behaviour here with what the bridge does next week.

[VenelinMartinov]

Terraform docs on SchemaConfigModeAttr:
https://developer.hashicorp.com/terraform/plugin/sdkv2/guides/terraform-0.12-compatibility#computed-resource-attributes

Only activate this mode if your provider has existing functionality that is depending on the ability to distinguish unset vs. explicitly empty for a nested resource collection.

which sounds very very relevant.

[VenelinMartinov]

Given the multiple rounds of different downstream regressions caused by the stages of this PR, combined with the slow iteration loop, I don't see how we can safely land this right now. Instead, I'd like to make the refactoring changes which were part of this PR and then make the behavioural changes once we can properly test them.

Old description for posterity:
This reverts commit 6241673 and gets #1688 back in.

This should allow us to address pulumi/pulumi-aws#3427 and unblock further upstream updates there.

The issue was that TF seems to pass empty arrays for not-specified MaxItemsOne properties, while we were passing nil, resulting in differences in behaviour between the bridge and TF.

The original PR went a bit too far and made the change for Check too, which caused some issues: pulumi/pulumi-aws#3427 (comment).

I've reused the mechanism added #1583 and only apply the empty arrays for MaxItemsOne properties in makeTerraformInputs when the ApplyMaxItemsOneDefaults flag is set. This makes sure we don't mess with the parameters when doing Check but pass the correct parameters to other methods.

This PR introduces a new makeTerraformInputs variant: makeTerraformInputsWithoutMaxItemsOneDefaults - this is only used for CheckConfig, since CheckConfig depends on the null MaxItemsOne properties but needs the TF defaults.

[VenelinMartinov]

I've opened #1767 for the follow up work.

[t0yv0]

I was a bit uncomfortable at first but approving after our chat and another round of looking at it in detail. Behavior is not changing, some new code is added but it's flagged off, and new tests are added which is excellent.

A bit of a nit but I'd prefer in the future not to remove public members MakeTerraformState cold-turkey but mark them deprecated or call out they're for internal use and subject to change in a comment. It does not take a lot more time and is good on the off chance it's called into by some codebase out there. These sort of small PRs also can be quick approve.

Thanks for the substantial work on the foundations here!

[VenelinMartinov]

Thanks for the reviews! After discussing with @t0yv0, I've removed the breaking changes here - we really have no reason to do that. I've cleaned up other unnecessary changes from this PR and will merge once tests pass.