-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
feat: CDN endpoint disable default value for compression #8610
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zekth, thanks so much for this PR... it's looking good so far, but I left a comment in the PR for a potential issue I noticed while reviewing the code. Get those two things fixed up and I think we would be good to go with merging these changes! 🚀
if is_compression_enabled := props.IsCompressionEnabled; is_compression_enabled != nil { | ||
d.Set("is_compression_enabled", *is_compression_enabled) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to get this to work as expected I suspect you are going to have to modify the resourceArmCdnEndpointCreate
function as well. The reason for this is since the data type of is_compression_enabled
is bool
and in the original create function has this line of code:
compressionEnabled := d.Get("is_compression_enabled").(bool)
So even if the d.Get
call does not find the is_compression_enabled
attribute in the configuration file the value of compressionEnabled
will be false
because that is the default zero value for bool
. Which means no matter what the is_compression_enabled
attribute is, or is not, it will always be set to false
. To fix this I think you will need to remove this line of code:
compressionEnabled := d.Get("is_compression_enabled").(bool)
and the this line of code from the cdn.Endpoint
struct definition:
IsCompressionEnabled = &compressionEnabled
Then switch the logic in the resourceArmCdnEndpointCreate
function to use the d.GetOk
instead of just d.Get
which will only set the value of compressionEnabled
if the attribute actually exists in the configuration file, something along the lines of:
location := azure.NormalizeLocation(d.Get("location").(string))
httpAllowed := d.Get("is_http_allowed").(bool)
httpsAllowed := d.Get("is_https_allowed").(bool)
cachingBehaviour := d.Get("querystring_caching_behaviour").(string)
originHostHeader := d.Get("origin_host_header").(string)
originPath := d.Get("origin_path").(string)
probePath := d.Get("probe_path").(string)
optimizationType := d.Get("optimization_type").(string)
contentTypes := expandArmCdnEndpointContentTypesToCompress(d)
geoFilters := expandCdnEndpointGeoFilters(d)
t := d.Get("tags").(map[string]interface{})
endpoint := cdn.Endpoint{
Location: &location,
EndpointProperties: &cdn.EndpointProperties{
ContentTypesToCompress: &contentTypes,
GeoFilters: geoFilters,
IsHTTPAllowed: &httpAllowed,
IsHTTPSAllowed: &httpsAllowed,
QueryStringCachingBehavior: cdn.QueryStringCachingBehavior(cachingBehaviour),
OriginHostHeader: utils.String(originHostHeader),
},
Tags: tags.Expand(t),
}
if v, ok := d.GetOk("is_compression_enabled"); ok {
endpoint.EndpointProperties.IsCompressionEnabled = &v.(bool)
}
Does that make sense? Also, can you please add a test case for this scenario where the is_compression_enabled
attribute is not passed in the configuration file?
@WodansSon slight changes compared to your review which seems to work. Also added a test that seems to fit the style. Please let me know your thoughts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zekth, thanks for pushing those changes, this LGTM now. If all the tests pass I will go ahead and merge the PR. Thanks again! 🚀
@zekth, unfortunately the test fails and I cannot merge the code until the test passes:
It appears even though you are attempting to suppress the |
My bad, thanks @WodansSon |
CI seems to fail on website-lint but doesn't seem to be related |
Yes, it appears a change in the |
@zekth, I have merged the PR that will fix your CI lint error, so feel free to do a force-push from main and I can get this one merged too! 🚀 |
This has been released in version 2.34.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.34.0"
}
# ... other configuration ... |
Sorry to bring the bad news but it seems there is another issue:
The terraform provider seems to add the QueryStringCachingBehavior and ContentTypesToCompress by default like IsCompressionEnabled was before the PR. eg: resource "azurerm_cdn_endpoint" "cdn_endpoint" {
name = "cdne${local.formatted_name}"
profile_name = var.cdn_profile_name
location = "global"
resource_group_name = var.resource_group_name
origin_path = "/foo"
origin_host_header = var.originHostname
is_https_allowed = true
is_http_allowed = false
origin {
name = "name"
host_name = var.originHostname
}
} Seems like the tests of the provider are not really in Sync with API. Especially with the verizon API. |
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! |
fix: #1109
Regarding this issue, this PR fixes it but it will forces people to set the attribute if they have not set
is_compression_allowed
attribute in their terraform template.Currently this is a blocking issue for people willing to use Premium Verizon