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

Fix 'edge_ip_connectivity' state persistence #1515

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

kireledan
Copy link

@kireledan kireledan commented Mar 17, 2022

Fixes #1500

This PR fixes the edge_ip_connectivity value from not being stored in the terraform state when being read.

Tested with the resource below and was able to get a No changes after applying and running another plan. As well as after importing the resource.

resource "cloudflare_spectrum_application" "myapp-east-sfe" {
  ip_firewall    = true
  origin_port    = 25
  protocol       = "tcp/25"
  proxy_protocol = "v1"
  tls            = "off"
  traffic_type   = "direct"
  edge_ip_connectivity = "ipv4"
  zone_id        = "redacted"
  dns {
    name = "myapp-test-spectrum.redacted.net"
    type = "CNAME"
  }
  origin_dns {
    name = "myapp-test-spectrum.redacted.net"
  }
}

@github-actions
Copy link
Contributor

Oops! It looks like no changelog entry is attached to this PR. Please include a release note as described in https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/docs/changelog-process.md.

Example:

```release-note:TYPE
Release note
```

If you do not require a release note to be included, please add the workflow/skip-changelog-entry label.

Copy link
Member

@jacobbednarz jacobbednarz left a comment

Choose a reason for hiding this comment

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

you’ll need to rename the CHANGELOG file to match the PR number.

can you please extend a test case to cover this field being saved to state? https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/cloudflare/resource_cloudflare_spectrum_application_test.go has examples to get you started.

@kireledan
Copy link
Author

Ah gotcha! Thought it was the Issue number.

Ill add that test in too 👍

@jacobbednarz jacobbednarz force-pushed the kireledan/1500-spectrum-fix branch from 9c0b496 to 0a7cf6a Compare March 21, 2022 22:56
@jacobbednarz
Copy link
Member

acceptance tests are passing locally

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareSpectrumApplication_" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareSpectrumApplication_Import
=== PAUSE TestAccCloudflareSpectrumApplication_Import
=== RUN   TestAccCloudflareSpectrumApplication_Basic
--- PASS: TestAccCloudflareSpectrumApplication_Basic (13.27s)
=== RUN   TestAccCloudflareSpectrumApplication_OriginDNS
--- PASS: TestAccCloudflareSpectrumApplication_OriginDNS (11.70s)
=== RUN   TestAccCloudflareSpectrumApplication_OriginPortRange
--- PASS: TestAccCloudflareSpectrumApplication_OriginPortRange (11.55s)
=== RUN   TestAccCloudflareSpectrumApplication_Update
--- PASS: TestAccCloudflareSpectrumApplication_Update (20.26s)
=== RUN   TestAccCloudflareSpectrumApplication_CreateAfterManualDestroy
--- PASS: TestAccCloudflareSpectrumApplication_CreateAfterManualDestroy (23.64s)
=== RUN   TestAccCloudflareSpectrumApplication_EdgeIPConnectivity
--- PASS: TestAccCloudflareSpectrumApplication_EdgeIPConnectivity (11.32s)
=== CONT  TestAccCloudflareSpectrumApplication_Import
--- PASS: TestAccCloudflareSpectrumApplication_Import (15.29s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	107.439s
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/changelog-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/maintainer-only-file-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/tf-log-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]

@jacobbednarz jacobbednarz merged commit 45e1431 into cloudflare:master Mar 21, 2022
@jacobbednarz
Copy link
Member

thank you @kireledan for this one!

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

Successfully merging this pull request may close these issues.

Spectrum App resource not reading "edge_ips" correctly from API
2 participants