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

azurerm_cdn_endpoint - delivery_rule condition optional match_values #8850

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Oct 12, 2020

When the delivery_rule condition operator is set to Any, we should
not pass any match_values at all.

Fixes #8770

…lues`

When the `delivery_rule` condition operator is set to `Any`, we should
not pass any `match_values` at all.
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @magodo
Thanks for this PR. What does the API do if presented with a payload that doesn't use the Any operator and no match_rules are present? It feels remiss to trade one API error for another? If it does error, this should be addressed in code or documentation? WDYT?

@magodo
Copy link
Collaborator Author

magodo commented Oct 15, 2020

@jackofallops You're right, I should have mentioned it in the document to make it clear. Whilst I think it is not necessarily to check in the code as it will fail in the API anyway, there is no too much benefit to fail a bit earlier just before sending the request.

@ghost ghost removed the waiting-response label Oct 16, 2020
Copy link

@DeltaOffset DeltaOffset left a comment

Choose a reason for hiding this comment

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

Changing from Required to Optional setting for match_values when the operator is "Any" should be a valid fix for this scenario. IMHO

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @magodo

Taking a look through whilst this seems ok, can we add some tests for this to confirm that these changes are sufficient?

Thanks!

@ghost ghost added size/L and removed size/S labels Oct 22, 2020
@magodo
Copy link
Collaborator Author

magodo commented Oct 22, 2020

@tombuildsstuff Thank you for reviewing! I have added a test to cover all the conditions which has match set to Any.

Test Result

💤 make testacc TEST=./azurerm/internal/services/cdn/tests TESTARGS='-run TestAccAzureRMCdnEndpoint_deliveryRuleOptionalMatchValue'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/cdn/tests -v -run TestAccAzureRMCdnEndpoint_deliveryRuleOptionalMatchValue -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMCdnEndpoint_deliveryRuleOptionalMatchValue
=== PAUSE TestAccAzureRMCdnEndpoint_deliveryRuleOptionalMatchValue
=== CONT  TestAccAzureRMCdnEndpoint_deliveryRuleOptionalMatchValue
--- PASS: TestAccAzureRMCdnEndpoint_deliveryRuleOptionalMatchValue (445.78s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/cdn/tests   445.901s

@DeltaOffset
Copy link

Guys Is this going to be merged soon? I Will like to import my cdn rules

@katbyte
Copy link
Collaborator

katbyte commented Dec 30, 2020

@magodo - could we fix the merge conflicts? once thats done i think this is good to merge

@katbyte katbyte added this to the v2.42.0 milestone Dec 30, 2020
@magodo
Copy link
Collaborator Author

magodo commented Dec 31, 2020

@katbyte I've resolved the conflicts, please take another look!

💤 make testacc TEST=./azurerm/internal/services/cdn TESTARGS='-run TestAccCdnEndpoint_deliveryRuleOptionalMatchValue'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/cdn -v -run TestAccCdnEndpoint_deliveryRuleOptionalMatchValue -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccCdnEndpoint_deliveryRuleOptionalMatchValue
=== PAUSE TestAccCdnEndpoint_deliveryRuleOptionalMatchValue
=== CONT  TestAccCdnEndpoint_deliveryRuleOptionalMatchValue
--- PASS: TestAccCdnEndpoint_deliveryRuleOptionalMatchValue (504.39s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/cdn 504.426s

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @magodo - aside from the docs needed to be updated this LGTM 👍

@katbyte katbyte merged commit 5088ef4 into hashicorp:master Dec 31, 2020
katbyte added a commit that referenced this pull request Dec 31, 2020
@ghost
Copy link

ghost commented Jan 8, 2021

This has been released in version 2.42.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.42.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jan 30, 2021

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked as resolved and limited conversation to collaborators Jan 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_cdn_endpoint request_uri_condition if "any" match_values still required
5 participants