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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 8 commits into from
Closed

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

wants to merge 8 commits into from

Conversation

SarahFrench
Copy link
Collaborator

@SarahFrench SarahFrench commented Aug 29, 2024 •
edited
Loading

Replaced by #11903

provider: refactored how the provider configuration is handled internally

@SarahFrench
Copy link
Collaborator Author

There are lots of code deletions I can do as a result of this change, but I'll split that into a separate PR for clarity.

@SarahFrench SarahFrench changed the title Refactor muxing: Re-use same config to configure the SDK and PF providers, fix VCR testing Refactor muxing 1 : Re-use same config to configure the SDK and PF providers, fix VCR testing Aug 29, 2024
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@SarahFrench SarahFrench mentioned this pull request Aug 29, 2024
Draft
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@SarahFrench
Copy link
Collaborator Author

I'll fix the unused import after the test run finishes- I want to see the outcome and each push delays it!

@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
This was referenced Aug 31, 2024
Merged
Open
This was referenced Sep 5, 2024
Merged
Merged
Merged
@SarahFrench SarahFrench mentioned this pull request Sep 11, 2024
Merged
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@modular-magician

This comment was marked as outdated.

Sign in to view
@SarahFrench
Copy link
Collaborator Author

Rebased to include Go rewrtie

@SarahFrench SarahFrench mentioned this pull request Sep 26, 2024
Draft
@SarahFrench SarahFrench force-pushed the mux-refactor-4-reuse-sdk-config branch from 88ed904 to f230af6 Compare October 1, 2024 18:58
@SarahFrench
Copy link
Collaborator Author

Just rebased to pull in the latest changes from main, including new acc tests for how the provider is configured

@SarahFrench SarahFrench force-pushed the mux-refactor-4-reuse-sdk-config branch from 3a81730 to eeff5dd Compare October 2, 2024 22:05
@modular-magician

This comment has been minimized.

Sign in to view
1 similar comment
@modular-magician

This comment has been minimized.

Sign in to view
@modular-magician

This comment has been minimized.

Sign in to view
@modular-magician

This comment has been minimized.

Sign in to view
@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(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 4138
Passed tests: 3721
Skipped tests: 411
Affected tests: 6

Click here to see the affected service packages

All service packages are affected

Action taken

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

Get to know how VCR tests work

@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:
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

I'm going to re-make this PR, as it was a bit messy to start but rebases to pull in Go rewrite and new acc tests have made the PR history too long for good review. I'll re-open a new PR with a cleaner commit history

@SarahFrench
Add comments highlighting that parallel schemas are needed and must m…
a0ce16e 
…atch
@SarahFrench
When creating the PF provider, store a pointer to the SDK provider st…
729ab55 
…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.
@SarahFrench
Update the plugin-framework provider to pass the SDK's Config meta …
f274d7a 
…struct to all data sources and resources, instead of `FrameworkProviderConfig`
@SarahFrench
Update all PF-implemented things to use the Config struct, update the…
8acf458 
… 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
@SarahFrench
Update google_provider_config_plugin_framework to use Config struct…
130f9e6 
…, adapt to type system changes
@SarahFrench
Update TestFrameworkProvider_impl to use primary when creating PF p…
4ba69e0 
…rovider
@SarahFrench
Fix how the plugin-framework is used in VCR tests
1b0712f 
- 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.
@SarahFrench
Remove most instances of VCR skips referencing hashicorp/terraform-pr…
f64f805 
…ovider-google#14158

- Use of older TPG versions through ExternalProviders breaks VCR, so that is also removed from TestAccDataSourceGoogleFirebaseAppleAppConfig
@SarahFrench SarahFrench force-pushed the mux-refactor-4-reuse-sdk-config branch from 487e570 to f64f805 Compare October 3, 2024 09:05
@SarahFrench SarahFrench closed this Oct 3, 2024
@SarahFrench
Copy link
Collaborator Author

See PR description for replacement PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mraouffouad mraouffouad Awaiting requested review from mraouffouad

Assignees
No one assigned
Labels
service/firebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SarahFrench @modular-magician