Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor muxing 1 : Re-use same config to configure the SDK and PF providers, fix VCR testing #11903

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

SarahFrench
Copy link
Collaborator

@SarahFrench SarahFrench commented Oct 3, 2024

Fixes hashicorp/terraform-provider-google#18774

Summary

This PR:

  • Changes how the plugin-framework implementation of the provider is configured
    • Before: Parallel implementation of configuration logic that parses user inputs and populates a fwtransport.FrameworkProviderConfig struct
    • After: The configure function obtains an already populated transport_tpg.Config struct from the SDK provider, with no duplication of logic to process user inputs.
  • Changes code defining the VCR system to accommodate the change above
  • Makes necessary changes to the data sources currently implemented with the plugin-framework
    • These now expect to receive a transport_tpg.Config struct
    • Any provider-level values are now in the old type system, and require thought about where we convert between those type systems.

Things to pay attention to during review/testing


provider: refactored how the provider configuration is handled internally

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 15 files changed, 157 insertions(+), 179 deletions(-))
google-beta provider: Diff ( 21 files changed, 201 insertions(+), 222 deletions(-))

@SarahFrench
Copy link
Collaborator Author

SarahFrench commented Oct 3, 2024

Initial acc tests for the SDK and PF provider code. These tests aren't final, as we're waiting for some more acc tests to be added to the code base and then pulled into this PR.

GA Provider:

Beta Provider:


Test failures:

  • In the Framework builds lots of tests currently fail with Attribute 'X' found when not expected
    • Explanation: this is because in the new PF type system if a string field's value is null the field isn't present at all, whereas in the SDK system the field would have a value of "".
    • User impact? : Values that were once null will become "" when passed to PF-implemented resources.
    • Code changes : This is an example of where we can just update the test
  • Changes cause "" values of request_reason to no longer be used in the PF code, instead any present ENVs will be used instead
  • Test flakiness is impacting:

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 4138
Passed tests: 3716
Skipped tests: 411
Affected tests: 11

Click here to see the affected service packages

All service packages are affected

Action taken

Found 11 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccDataSourceGoogleFirebaseAndroidAppConfig
  • TestAccDataSourceGoogleFirebaseAppleAppConfig
  • TestAccDataSourceGoogleQuotaInfos_basic
  • TestAccDataformRepositoryReleaseConfig_dataformRepositoryReleaseConfigExample
  • TestAccDataformRepositoryWorkflowConfig_dataformRepositoryWorkflowConfigExample
  • TestAccDataformRepository_dataformRepositoryWithCloudsourceRepoAndSshExample
  • TestAccDataformRepository_updated
  • TestAccDataprocCluster_withAutoscalingPolicy
  • TestAccFirebaseWebApp_firebaseWebAppFull
  • TestAccFrameworkProviderBasePath_setBasePath
  • TestAccFrameworkProviderMeta_setModuleName

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]
TestAccDataSourceGoogleFirebaseAppleAppConfig[Debug log]
TestAccFirebaseWebApp_firebaseWebAppFull[Debug log]
TestAccFrameworkProviderBasePath_setBasePath[Debug log]
TestAccFrameworkProviderMeta_setModuleName[Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🔴 Tests failed during RECORDING mode:
TestAccDataSourceGoogleQuotaInfos_basic[Error message] [Debug log]
TestAccDataformRepositoryReleaseConfig_dataformRepositoryReleaseConfigExample[Error message] [Debug log]
TestAccDataformRepositoryWorkflowConfig_dataformRepositoryWorkflowConfigExample[Error message] [Debug log]
TestAccDataformRepository_dataformRepositoryWithCloudsourceRepoAndSshExample[Error message] [Debug log]
TestAccDataformRepository_updated[Error message] [Debug log]
TestAccDataprocCluster_withAutoscalingPolicy[Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

@SarahFrench
Copy link
Collaborator Author

Latest commits address some of the test failures described in this comment.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccFrameworkProviderBasePath_setBasePath[Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🔴 Tests failed during RECORDING mode:
TestAccDataSourceGoogleQuotaInfos_basic[Error message] [Debug log]
TestAccDataformRepositoryReleaseConfig_dataformRepositoryReleaseConfigExample[Error message] [Debug log]
TestAccDataformRepositoryWorkflowConfig_dataformRepositoryWorkflowConfigExample[Error message] [Debug log]
TestAccDataformRepository_dataformRepositoryWithCloudsourceRepoAndSshExample[Error message] [Debug log]
TestAccDataformRepository_updated[Error message] [Debug log]
TestAccDataprocCluster_withAutoscalingPolicy[Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccFrameworkProviderBasePath_setBasePath[Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🔴 Tests failed during RECORDING mode:
TestAccDataSourceGoogleQuotaInfos_basic[Error message] [Debug log]
TestAccDataformRepositoryReleaseConfig_dataformRepositoryReleaseConfigExample[Error message] [Debug log]
TestAccDataformRepositoryWorkflowConfig_dataformRepositoryWorkflowConfigExample[Error message] [Debug log]
TestAccDataformRepository_dataformRepositoryWithCloudsourceRepoAndSshExample[Error message] [Debug log]
TestAccDataformRepository_updated[Error message] [Debug log]
TestAccDataprocCluster_withAutoscalingPolicy[Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

Copy link
Contributor

@rainshen49 rainshen49 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval for Firebase since the changes are mechanical in nature and Firebase tests are passing.

…ruct

This allows the PF provider to access any data on the SDK provider, including the meta/Config struct that will be created when the SDK provider is configured.
…struct to all data sources and resources, instead of `FrameworkProviderConfig`
… Config struct to be usable in those places.

- Replace use of FrameworkProviderConfig with Config
- Add Beta-only `NewFirebaseClient` method to Config struct for use with Firebase data sources
- Update LocationDescriber interface
- Misc places where the SDK and PF type systems meet and string needs to be converted to types.StringType
- This carries over the idea of configuring the PF provider using the SDK provider. The changes in the MuxedProviders func mimic changes already made in main.go.
- Remove unnecessary duplication of cached configs per test name; now one used regardless of PF/SDK
- Updates to some DestroyProducer functions so they access the cached SDK Config struct to get a client
- Remove GetFwTestProvider and the file containing it - this isn't needed.
…ovider-google#14158

- Use of older TPG versions through ExternalProviders breaks VCR, so that is also removed from TestAccDataSourceGoogleFirebaseAppleAppConfig
…e now ignored and ENVs can be used instead
…el_addition_strategy` acctest to reflect how the SDK configuration logic now affects the PF provider
…se of ExternalProviders to get other versions of TPG/TPGB
@SarahFrench
Copy link
Collaborator Author

SarahFrench commented Oct 25, 2024

Here I'm running acceptance tests to see which ones fail, similar to #11903 (comment) above.

TeamCity links:


SDK test failures

SDK test failures typically represent validation that was provided by the PF configuration logic being removed as part of the muxing fixes.

In the SDK tests above the only failure is TestAccSdkProvider_request_timeout /when_request_timeout_is_set_to_an_empty_string_in_the_config_the_value_fails_validation,_as_it_is_not_a_duration :

provider_request_timeout_test.go:103: Step 1/1, expected an error but got none

This shows that the muxing fixes have stopped some validation originating from the plugin-framework code that detects when the string it's parsable as a duration. The original validation observed for this field in the PF code doesn't originate from the schema as there are no Validators for that field. Instead I believe this code would raise an error when the PF provider's configuration logic was handling a non-parsable duration string. That code is no longer used for configuring the PF provider after this PR changes to muxing.

To solve this we could either:

  1. Update the test case and allow the provider to not provide feedback when an empty string is used for request_timeout. This means the provider will be becoming more permissive as a result of the muxing fixes.
  2. Add a validator to the PF provider implementation's schema that asserts all values passed in are parsable as a duration. This would result in the same behaviour being seen by users pre- and post-muxing fix. It would not be a breaking change as the new validation on the schema would return the same errors that were previously returned from here.

PF test failures

"found when not expected"

These test failures are due to how the muxing fixes will change the behaviour of unset field in the data source google_provider_config_plugin_framework used in the tests.

TestAccFwProvider_billing_project/when_billing_project_is_unset_in_the_config,_environment_variables_are_used_in_a_given_order fails due to this, as the field that was once explicitly unset becomes a zero valued string.

  • Check 1/1 error: data.google_provider_config_plugin_framework.default: Attribute 'billing_project' found when not expected

Similarly TestAccFwProvider_user_project_override/when_user_project_override_is_unset_in_the_config,_environment_variables_are_used fails with:

  • Check 1/1 error: data.google_provider_config_plugin_framework.default: Attribute 'user_project_override' found when not expected

Also, TestAccFwProvider_access_token/access_token_can_be_used_to_authenticate_the_provider:

  • Check 2/2 error: data.google_provider_config_plugin_framework.default: Attribute 'credentials' found when not expected

The solutions to these are to update how the test is defined. The test failures don't reflect changes that would impact users.

TestAccFwProvider_request_timeout

------- Stdout: -------
=== RUN   TestAccFwProvider_request_timeout
=== RUN   TestAccFwProvider_request_timeout/when_request_timeout_is_set_to_an_empty_string_in_the_config_the_value_fails_validation,_as_it_is_not_a_duration
    framework_provider_request_timeout_test.go:105: Step 1/1, expected an error but got none
=== RUN   TestAccFwProvider_request_timeout/a_default_value_of_120s_is_used_when_there_are_no_user_inputs
    framework_provider_request_timeout_test.go:47: Step 1/1 error: Check failed: Check 1/1 error: data.google_provider_config_plugin_framework.default: Attribute 'request_timeout' expected "120s", got "0s"
=== RUN   TestAccFwProvider_request_timeout/request_timeout_can_be_set_in_config_in_different_formats,_are_NOT_normalized_to_full-length_format
    framework_provider_request_timeout_test.go:78: Step 2/2 error: Check failed: Check 1/1 error: data.google_provider_config_plugin_framework.default: Attribute 'request_timeout' expected "3m", got "3m0s"
--- FAIL: TestAccFwProvider_request_timeout (32.45s)
    --- FAIL: TestAccFwProvider_request_timeout/when_request_timeout_is_set_to_an_empty_string_in_the_config_the_value_fails_validation,_as_it_is_not_a_duration (11.46s)
    --- FAIL: TestAccFwProvider_request_timeout/a_default_value_of_120s_is_used_when_there_are_no_user_inputs (5.88s)
    --- FAIL: TestAccFwProvider_request_timeout/request_timeout_can_be_set_in_config_in_different_formats,_are_NOT_normalized_to_full-length_format (15.09s)
FAIL
  1. "a default value of 120s is used when there are no user inputs"

Attribute 'request_timeout' expected "120s", got "0s" is due to variations in logic between SDK and PF implementations of configuration logic, described here : https://github.com/GoogleCloudPlatform/magic-modules/pull/8898/files#r1320174439

We should just update the test to reflect the SDK default being used here now, so the test matches the SDK equivalent: https://github.com/hashicorp/terraform-provider-google/blob/82d641bafd47d8863aaa6a07521a51207ad92732/google/provider/provider_request_timeout_test.go#L19

  1. "request_timeout can be set in config in different formats, are NOT normalized to full-length format"

In the PF code previously, if you supplied "3m" as a duration string it would remain that way. In contrast, the SDK code will convert that value to the longer-form "3m0s". This disparity is noted here in the test.

The solution here is to update the test to match the SDK behaviour.

  1. "when request_timeout is set to an empty string in the config the value fails validation, as it is not a duration"

This is failing for the same reason as explained in the SDK test failures section above.

@modular-magician

This comment was marked as off-topic.

@modular-magician

This comment was marked as off-topic.

@SarahFrench
Copy link
Collaborator Author

SarahFrench commented Oct 25, 2024

The last few commits are acting on my comments in #11903 (comment)

Screenshot 2024-10-25 at 18 54 37

Edit: 0f85c17 was a correction to one of these commits

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 25 files changed, 238 insertions(+), 226 deletions(-))
google-beta provider: Diff ( 31 files changed, 283 insertions(+), 270 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 4215
Passed tests: 3796
Skipped tests: 416
Affected tests: 3

Click here to see the affected service packages

All service packages are affected

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeInstanceConfidentialInstanceConfigMain
  • TestAccComputeInstanceFromMachineImage_confidentialInstanceConfigMain
  • TestAccContainerCluster_resourceManagerTags

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccContainerCluster_resourceManagerTags [Debug log]

🔴 Tests failed when rerunning REPLAYING mode:
TestAccContainerCluster_resourceManagerTags [Error message] [Debug log]

Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made.

Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer.


🔴 Tests failed during RECORDING mode:
TestAccComputeInstanceConfidentialInstanceConfigMain [Error message] [Debug log]
TestAccComputeInstanceFromMachineImage_confidentialInstanceConfigMain [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

@SarahFrench
Copy link
Collaborator Author

After the previous commits' changes to tests to address the discussion in #11903 (comment) I've re-run acceptance tests in TeamCity:

SDK tests

  • GA 18/18 pass
  • Beta 18/18 pass

Framework tests

  • GA 17/17 pass
  • Beta 16/17 pass
    • The failure in TestAccFwProvider_user_project_override looks like a timing issue when enabling a service API in the test, so is something that could be investigated separately to this PR.

@SarahFrench
Copy link
Collaborator Author

Also, today I did some manual testing to confirm that this PR fixes some authentication bugs that were introduced by muxing: hashicorp/terraform-provider-google#16832 (comment)

That comment has information about how to reproduce the bug and how to use my fork of the TPG repo to test how this PR's changes fix the issue.

@SarahFrench SarahFrench marked this pull request as ready for review October 28, 2024 19:21
@SarahFrench
Copy link
Collaborator Author

Opened for review - I've stopped doing self-review and manual testing, so I'm waiting on review comments either about the code, testing or to coordinate merging the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants