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

Skip instance migrate tests, they handle config directly #3554

Merged
merged 3 commits into from
May 27, 2020

Conversation

slevenick
Copy link
Contributor

@slevenick slevenick commented May 21, 2020

Because these tests don't use resource.Test and instead manually create *Config objects they never properly call the ConfigureFunc on the config, preventing VCR transport swapping

Skip Bigtable tests that use a client factory that takes the raw TokenSource to produce new clients on demand within the resource rather than using the shared HTTP client

Release Note Template for Downstream PRs (will be copied)


@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 2 insertions(+))
Terraform Beta: Diff ( 1 file changed, 2 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 32 insertions(+))
Terraform Beta: Diff ( 6 files changed, 32 insertions(+))

@slevenick slevenick requested a review from danawillow May 26, 2020 23:43
@@ -8,6 +8,8 @@ import (
)

func TestAccBigtableAppProfile_update(t *testing.T) {
// bigtable does not use the shared HTTP client
Copy link
Contributor

Choose a reason for hiding this comment

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

do you also need a skip on the app profile generated tests?
(also the comment is mostly true, but this resource and the IAM ones I think do use the shared HTTP client, it's just that their tests also create instances, which don't)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, app profile tests set up an instance. I'll change the note to reference the bigtable instance only

@slevenick slevenick force-pushed the instance-migrate-vcr branch from 6cef13d to 043fab0 Compare May 27, 2020 00:32
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 32 insertions(+))
Terraform Beta: Diff ( 6 files changed, 32 insertions(+))

1 similar comment
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 32 insertions(+))
Terraform Beta: Diff ( 6 files changed, 32 insertions(+))

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

Successfully merging this pull request may close these issues.

4 participants