Remove all instances of GOOGLE_XPN_HOST_PROJECT environment variable. #815
Conversation
Instead of GOOGLE_XPN_HOST_PROJECT being required to run some tests, I added the ability to create and tear down the necessary project structure. This allows us to remove one environment variable, and use two others which are already widely-required: org and billing ID.
google/resource_compute_disk_test.go
Outdated
| @@ -200,6 +200,35 @@ func testAccCheckComputeDiskDestroy(s *terraform.State) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func testAccCheckComputeDiskExistsInProject(n, p string, disk *compute.Disk) resource.TestCheckFunc { | |||
There was a problem hiding this comment.
I don't like that I had to do this (this is a copy of testAccCheckComputeDiskExists with an extra project parameter, which I hate) but I wasn't sure how I could avoid that. The other method (here and in all three files) uses the default project. I'm a little too new to Go to know what the right move here was - I wanted to make the project an argument in testAccCheckComputeDiskExists, but don't know how to get the name of the default project at the call site. I assume this cannot be right - please let me know what would be right.
There was a problem hiding this comment.
getTestProjectFromEnv() is the method you are looking for.
There was a problem hiding this comment.
Ahh, thank you! That seems better. I'll make that change and ping you again.
|
I think @rosbo is the right person to look at this. |
|
Okay, I made that change - I'm struggling to run all the tests I've changed, now - running into quota issues on my personal account, and don't have an Organization that I can use in the google.com org. Do we have CI that will run these tests in a canonical environment? |
…vider-google into remove_xpn_var
google/resource_compute_disk_test.go
Outdated
| @@ -345,29 +345,43 @@ resource "google_compute_disk" "foobar" { | |||
| }`, diskName) | |||
| } | |||
|
|
|||
| func testAccComputeDisk_fromSnapshot(firstDiskName, snapshotName, diskName, xpn_host string, ref_selector string) string { | |||
| func testAccComputeDisk_fromSnapshot(projectName, firstDiskName, snapshotName, diskName, org, billingId, ref_selector string) string { | |||
There was a problem hiding this comment.
Why was this test using the XPN project in the first place instead of the regular test project?
There was a problem hiding this comment.
Not sure - I can't see any reason that it ought to be.
There was a problem hiding this comment.
I think we should re-write it using the default test project to avoid having to create extra test project.
There was a problem hiding this comment.
Agreed, and done.
| @@ -350,7 +351,7 @@ func testAccCheckComputeInstanceTemplateDestroy(s *terraform.State) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func testAccCheckComputeInstanceTemplateExists(n string, instanceTemplate *compute.InstanceTemplate) resource.TestCheckFunc { | |||
| func testAccCheckComputeInstanceTemplateExists(n, p string, instanceTemplate *compute.InstanceTemplate) resource.TestCheckFunc { | |||
There was a problem hiding this comment.
Consider adding a second method: testAccCheckComputeInstanceTemplateXPNExists
func testAccCheckComputeInstanceTemplateExists(n string, instanceTemplate *compute.InstanceTemplate) resource.TestCheckFunc {
return testAccCheckComputeInstanceTemplateXPNExists(n, getTestProjectFromEnv(), instanceTemplate)
} There was a problem hiding this comment.
Good idea, done.
| @@ -873,7 +874,7 @@ func testAccCheckComputeInstanceDestroy(s *terraform.State) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func testAccCheckComputeInstanceExists(n string, instance *compute.Instance) resource.TestCheckFunc { | |||
| func testAccCheckComputeInstanceExists(n, p string, instance *compute.Instance) resource.TestCheckFunc { | |||
There was a problem hiding this comment.
Same comment than for instance template.
|
Cool, I'm not planning to add to the changelog for this since it's 100% internal. what should I do to remove the now-unnecessary environment variable from our test environment? @rosbo or @paddycarver might know? |
|
I'll take care of it! Thanks for making this happen. 😁 |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
Instead of GOOGLE_XPN_HOST_PROJECT being required to run some tests, I added the ability to create and tear down the necessary project structure. This allows us to remove one environment variable, and use two others which are already widely-required: org and billing ID.
After this PR, no instances of GOOGLE_XPN_HOST_PROJECT appear in the repository. This closes #715.