Add Stackdriver Uptime Check IPs data source (#674) #700
Conversation
|
Looks like I haven't added the new package into the |
markosamuli
force-pushed
the
is-674-stackdriver-ip-ranges
branch
from
166c653 to
b02c2f4
Compare
google/config.go
Outdated
| @@ -28,6 +28,7 @@ import ( | |||
| "google.golang.org/api/dns/v1" | |||
| "google.golang.org/api/iam/v1" | |||
| cloudlogging "google.golang.org/api/logging/v2" | |||
| monitoring "google.golang.org/api/monitoring/v3" | |||
There was a problem hiding this comment.
nit: no need to rename this import
| "time" | ||
|
|
||
| "github.com/hashicorp/terraform/helper/schema" | ||
| monitoring "google.golang.org/api/monitoring/v3" |
| monitoring "google.golang.org/api/monitoring/v3" | ||
| ) | ||
|
|
||
| func dataSourceStackdriverIpRanges() *schema.Resource { |
There was a problem hiding this comment.
Since the actual API is called uptimeCheckIps, would it make sense to call this that instead of IpRanges?
| Read: dataSourceStackdriverIpRangesRead, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "cidr_blocks": { |
There was a problem hiding this comment.
Likewise, the field actually returned from the API is also called uptimeCheckIps. I'm not 100% opposed to renaming them, but there has to be a good reason behind it since (at least in my opinion) it's easier to find things if they keep the same name everywhere (terraform, gcloud, rest api, etc.)
There was a problem hiding this comment.
cidr_blocks seems something I've seen on AWS resources.
| ) | ||
|
|
||
| func TestAccDataSourceStackdriverIpRanges_basic(t *testing.T) { | ||
| resource.Test(t, resource.TestCase{ |
There was a problem hiding this comment.
can you add t.Parallel() (and then a new line) to the top of this test?
| }) | ||
| } | ||
|
|
||
| const testAccStackdriverIpRangesConfig = ` |
There was a problem hiding this comment.
Does this test need to be seeded with data in any way? I don't have anything stackdriver related in my project and it didn't pass:
$ make testacc TEST=./google TESTARGS='-run=TestAccDataSourceStackdriverIpRanges'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccDataSourceStackdriverIpRanges -timeout 120m
=== RUN TestAccDataSourceStackdriverIpRanges_basic
--- FAIL: TestAccDataSourceStackdriverIpRanges_basic (0.86s)
testing.go:435: Step 0 error: Check failed: Check 1/2 error: data.google_stackdriver_ip_ranges.some: Attribute 'cidr_ranges.#' didn't match "^[1-9]+[0-9]*$", got ""
FAIL
exit status 1
FAIL github.com/terraform-providers/terraform-provider-google/google 1.030s
make: *** [testacc] Error 1
There was a problem hiding this comment.
Environment config:
$ gcloud auth login
$ gcloud auth application-default login
$ export GOOGLE_USE_DEFAULT_CREDENTIALS=1
$ export GOOGLE_PROJECT=terraform-tests-xxx
$ export GOOGLE_REGION=us-central1
$ export GOOGLE_XPN_HOST_PROJECT=terraform-tests-xxx
This seems to work:
$ make testacc TEST=./google TESTARGS='-run=TestAccDataSourceStackdriverUptimeCheckIps'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccDataSourceStackdriverUptimeCheckIps -timeout 120m
=== RUN TestAccDataSourceStackdriverUptimeCheckIps_basic
--- PASS: TestAccDataSourceStackdriverUptimeCheckIps_basic (0.82s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 0.831s
There was a problem hiding this comment.
This was a new GCP project and I didn't activate any additional services other than GCE.
I do have an existing Stackdriver account under my login, but this project was not linked to it yet.
|
Thanks for the PR, @markosamuli! I have a few small comments, and a few that might require a bit of discussion. Let me know what you think :) |
markosamuli
force-pushed
the
is-674-stackdriver-ip-ranges
branch
from
b02c2f4 to
8237fe3
Compare
Use naming semantics from the UptimeCheckIps API.
markosamuli
force-pushed
the
is-674-stackdriver-ip-ranges
branch
from
8237fe3 to
08dde79
Compare
|
@danawillow Updated the naming to match the API. I can run the test locally with my account that has an active Stackdriver subscription even if not linked to the specific project I'm running the tests in. |
| Read: dataSourceStackdriverUptimeCheckIpsRead, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "uptime_check_ips": { |
There was a problem hiding this comment.
Do you think other people that use this would want the region/location information as part of the data source too?
| for i, uptimeCheckIp := range uptimeCheckIps { | ||
| result[i] = uptimeCheckIp.IpAddress | ||
| } | ||
| sort.Strings(result) |
There was a problem hiding this comment.
Do you think it would make sense for the schema to have this attribute as a Set instead of a List? If so then you wouldn't need to sort it.
|
Hey there @markosamuli, if this is still on your radar please let us know. :) If not, I'll go ahead and close it. |
|
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
Data source for #674
My original commit was forked off from v1.1.1.
The attribute
cidr_blocksseems to come from the AWS resource type, doesn't match the API semantics.