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: makes allowed_idps type to set #2094

Merged
merged 5 commits into from Dec 27, 2022

Conversation

keitap
Copy link
Contributor

@keitap keitap commented Dec 17, 2022

Because order of cloudflare_access_application.allowed_idps is not stable, each time terraform plan shows diff. To avoid this, allowed_idps should be set type rather than list.
It might be Cloudflare API doesn't care an order of IDPs.
For example, after I apply a plan below, rerun apply, it shows same diff.

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # cloudflare_access_application.xxx will be updated in-place
  ~ resource "cloudflare_access_application" "xxx" {
      ~ allowed_idps               = [
          - "327a35a4-d355-43f0-a90e-0ee915d3c7f9",
            "c6efb857-e6e4-483a-91c9-f0f0c63ea16e",
          + "327a35a4-d355-43f0-a90e-0ee915d3c7f9",
        ]
        id                         = "fcb66895-7006-482b-94a4-85a9eb3e51df"
        name                       = "XXX"
        # (12 unchanged attributes hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Signed-off-by: keita <keitap@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Dec 17, 2022

changelog detected ✅

Signed-off-by: keita <keitap@gmail.com>
@jacobbednarz
Copy link
Member

the change looks reasonable. can you please add test coverage for this change?

@keitap
Copy link
Contributor Author

keitap commented Dec 22, 2022

@jacobbednarz It was tested by TestAccCloudflareAccessApplication_WithADefinedIdps method. What kind of test do you want me add?

@jacobbednarz
Copy link
Member

the fact that the test was previously passing before you made your changes suggests it is not sufficient.

for sufficient test coverage, I should be able to run the test before your fix demonstrating the problem having the test fail, then apply your fix and have the test pass. in this case, you'll need to expertise the update with differently or ordered keys and have terraform not show a diff. if you check out other tests for TypeSet you'll see some prior art.

@keitap
Copy link
Contributor Author

keitap commented Dec 22, 2022

@jacobbednarz thank you! I will work on it😀

Copy link
Contributor Author

@keitap keitap left a comment

Choose a reason for hiding this comment

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

@jacobbednarz I have added a test but have not been able to run it on my end.
I understand that I need to set proper environment variables in order to run acceptance tests. Is there any easiest way to run acceptance tests on my local? Unfortunately, I don't have any unused domain that can register to Cloudflare ZT.

@jacobbednarz jacobbednarz merged commit df80200 into cloudflare:master Dec 27, 2022
@jacobbednarz
Copy link
Member

thanks @keitap ! 🏆

@github-actions github-actions bot added this to the v3.31.0 milestone Dec 27, 2022
github-actions bot pushed a commit that referenced this pull request Dec 27, 2022
@jacobbednarz
Copy link
Member

acceptance tests are all passing here

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareAccessApplication_" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareAccessApplication_BasicZone
--- PASS: TestAccCloudflareAccessApplication_BasicZone (5.39s)
=== RUN   TestAccCloudflareAccessApplication_BasicAccount
--- PASS: TestAccCloudflareAccessApplication_BasicAccount (4.42s)
=== RUN   TestAccCloudflareAccessApplication_WithCORS
--- PASS: TestAccCloudflareAccessApplication_WithCORS (7.10s)
=== RUN   TestAccCloudflareAccessApplication_WithSaas
--- PASS: TestAccCloudflareAccessApplication_WithSaas (3.97s)
=== RUN   TestAccCloudflareAccessApplication_WithAutoRedirectToIdentity
--- PASS: TestAccCloudflareAccessApplication_WithAutoRedirectToIdentity (2.43s)
=== RUN   TestAccCloudflareAccessApplication_WithEnableBindingCookie
--- PASS: TestAccCloudflareAccessApplication_WithEnableBindingCookie (5.33s)
=== RUN   TestAccCloudflareAccessApplication_WithCustomDenyFields
--- PASS: TestAccCloudflareAccessApplication_WithCustomDenyFields (3.74s)
=== RUN   TestAccCloudflareAccessApplication_WithADefinedIdps
--- PASS: TestAccCloudflareAccessApplication_WithADefinedIdps (5.21s)
=== RUN   TestAccCloudflareAccessApplication_WithMultipleIdpsReordered
--- PASS: TestAccCloudflareAccessApplication_WithMultipleIdpsReordered (10.36s)
=== RUN   TestAccCloudflareAccessApplication_WithHttpOnlyCookieAttribute
--- PASS: TestAccCloudflareAccessApplication_WithHttpOnlyCookieAttribute (3.50s)
=== RUN   TestAccCloudflareAccessApplication_WithHTTPOnlyCookieAttributeSetToFalse
--- PASS: TestAccCloudflareAccessApplication_WithHTTPOnlyCookieAttributeSetToFalse (3.54s)
=== RUN   TestAccCloudflareAccessApplication_WithSameSiteCookieAttribute
--- PASS: TestAccCloudflareAccessApplication_WithSameSiteCookieAttribute (3.52s)
=== RUN   TestAccCloudflareAccessApplication_WithLogoURL
--- PASS: TestAccCloudflareAccessApplication_WithLogoURL (3.70s)
=== RUN   TestAccCloudflareAccessApplication_WithSkipInterstitial
--- PASS: TestAccCloudflareAccessApplication_WithSkipInterstitial (3.66s)
=== RUN   TestAccCloudflareAccessApplication_WithAppLauncherVisible
--- PASS: TestAccCloudflareAccessApplication_WithAppLauncherVisible (3.58s)
PASS
PASS	github.com/cloudflare/terraform-provider-cloudflare/internal/provider	69.974s
PASS

@github-actions
Copy link
Contributor

This functionality has been released in v3.31.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@keitap keitap deleted the fix_allowed_idps_type branch December 28, 2022 05:08
@keitap
Copy link
Contributor Author

keitap commented Dec 28, 2022

@jacobbednarz Thank you for merging. Tests are passed but
if removing testAccCloudflareAccessApplicationConfigWithMultipleIdpsReordered and sharing testAccCloudflareAccessApplicationConfigWithMultipleIdps, the resource IDs (and name) will be swapped too. I think what we want is only need to swap allowed_idps order.
41fb703

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.

None yet

2 participants