-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[Terraform] remove auto_healing_policies from tests #979
Conversation
I am a robot that works on MagicModules PRs! I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
Why not delete it at Beta too? We test auto_healing_policies with a separate test, and I bet it just cropped up here by that one being copy pasted. |
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit dc80ddf) have been included in your existing downstream PRs. |
Looks like the tpgb changes are missing? |
Hmm I'm not sure whether the magician has the capability to add PRs after the initial set was generated, but I went ahead and opened it myself (since the branch was created successfully in the modular-magician fork). That'll probably be fine. Also for context in case you're curious, hashicorp/terraform-provider-google#522 was the PR that added auto_healing_policies in the first place, and it was in order to test the fact that IGM was using beta APIs but backend services weren't. So it did have a good reason to be there (at least for one test). Actually now I've convinced myself that I should add it back for one test, since people might use TPG/TPGB together. Don't approve this yet. |
Cool! I think we used https://github.com/GoogleCloudPlatform/magic-modules/blob/master/third_party/terraform/tests/resource_compute_instance_group_manager_test.go.erb#L234 as a test for versions in general initially. Is this test specific to BackendService or would it better be homed there? |
It's specific to backendservice since it tests the URL inside of a set. |
Ok I un-convinced myself that we need that test, since the presence of a beta feature in a config no longer influences which API we call, so that test isn't actually going to test interplay between beta/GA. |
Was it testing that we properly diffsuppress/guarded self links in a set maybe? I could see the value in keeping a test that does that & I thought that was what you meant. Let me know if you're still unconvinced & I should review as-is! |
auto_healing_policies is a beta feature, which means it only exists in the beta provider. however, it used to act as a toggle- if it was set, then we called the beta apis, otherwise we called the ga ones. since it doesn't act as a toggle anymore, it doesn't need to be in our tests. does that make sense? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM + downstreams
Talked offline - using a beta feature like this one used to be the only way to choose the API version, but now we can make anything beta by using the Beta provider. We don't need auto_healing_policies in these tests anymore.
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit f1781d0) have been included in your existing downstream PRs. |
This reverts commit da6b1b9.
Tracked submodules are build/terraform-beta build/terraform build/ansible build/inspec.
915e891
to
0ef303f
Compare
[all]
[terraform]
Remove auto_healing_policies from tests
[terraform-beta]
[ansible]
[inspec]