feat: CDN endpoint disable default value for compression #8610

zekth · 2020-09-24T11:46:59Z

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

WodansSon

@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! 🚀

WodansSon · 2020-10-22T01:03:49Z

azurerm/internal/services/cdn/cdn_endpoint_resource.go

+		if is_compression_enabled := props.IsCompressionEnabled; is_compression_enabled != nil {
+			d.Set("is_compression_enabled", *is_compression_enabled)
+		}
+


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?

zekth · 2020-10-22T12:24:51Z

@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.

WodansSon

@zekth, thanks for pushing those changes, this LGTM now. If all the tests pass I will go ahead and merge the PR. Thanks again! 🚀

WodansSon · 2020-10-22T23:31:05Z

@zekth, unfortunately the test fails and I cannot merge the code until the test passes:

go test -timeout 30000s -v ./azurerm/internal/services/cdn/tests -run TestAccAzureRMCdnEndpoint_withoutCompression

=== RUN   TestAccAzureRMCdnEndpoint_withoutCompression
=== PAUSE TestAccAzureRMCdnEndpoint_withoutCompression
=== CONT  TestAccAzureRMCdnEndpoint_withoutCompression
    TestAccAzureRMCdnEndpoint_withoutCompression: testing.go:684: Step 0 error: Check failed: Check 2/2 error: azurerm_cdn_endpoint.test: Attribute 'is_compression_enabled' found when not expected
--- FAIL: TestAccAzureRMCdnEndpoint_withoutCompression (212.51s)

It appears even though you are attempting to suppress the is_compression_enabled attribute from being written to the state file it is still getting persisted some how.

azurerm/internal/services/cdn/cdn_endpoint_resource.go

zekth · 2020-10-23T06:52:08Z

My bad, thanks @WodansSon

zekth · 2020-10-23T09:03:18Z

CI seems to fail on website-lint but doesn't seem to be related

WodansSon · 2020-10-23T20:45:44Z

CI seems to fail on website-lint but doesn't seem to be related

Yes, it appears a change in the Desktop Virtualization was accidentally merged into main which is causing the CI to flag the documentation. I am going to be fixing that issue in another PR, so once that is merged later today you should be able to do a force-push from main and the issue will fix itself. Sorry about that... so close! 🚀

WodansSon · 2020-10-24T01:38:11Z

@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! 🚀

Co-authored-by: WS <20408400+WodansSon@users.noreply.github.com>

WodansSon · 2020-10-25T07:23:00Z

ghost · 2020-10-29T19:07:24Z

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 ...

zekth · 2020-11-02T13:27:08Z

Sorry to bring the bad news but it seems there is another issue:

cdn.EndpointsClient#Create: Failure sending request: 
StatusCode=400 -- Original Error: Code="BadRequest" 
Message="We couldn’t configure QueryStringCachingBehavior, IsCompressionEnabled, ContentTypesToCompress, GeoFilters and DeliveryPolicy for premium profile endpoints. 
You’ll need to manage these endpoints in the supplemental portal."

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.

ghost · 2020-11-24T14:50:06Z

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!

ghost added size/XS documentation labels Sep 24, 2020

WodansSon requested changes Oct 22, 2020

View reviewed changes

WodansSon added the service/cdn label Oct 22, 2020

ghost added size/M and removed size/XS labels Oct 22, 2020

WodansSon approved these changes Oct 22, 2020

View reviewed changes

WodansSon reviewed Oct 23, 2020

View reviewed changes

azurerm/internal/services/cdn/cdn_endpoint_resource.go Outdated Show resolved Hide resolved

WodansSon added this to the v2.34.0 milestone Oct 24, 2020

zekth and others added 6 commits October 24, 2020 10:05

feat: disable default value

bf297f0

feat: disable is compression enabled default

efa8586

fix: fmt

8f4cb06

fix: review

67e2d14

feat: add test

05dea6c

Update azurerm/internal/services/cdn/cdn_endpoint_resource.go

ad5d5ad

Co-authored-by: WS <20408400+WodansSon@users.noreply.github.com>

WodansSon merged commit faccec8 into hashicorp:master Oct 25, 2020

WodansSon added a commit that referenced this pull request Oct 25, 2020

Updating to include #8610

3f632df

zekth deleted the feat_disabledefault branch October 25, 2020 08:00

ghost locked as resolved and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: CDN endpoint disable default value for compression #8610

feat: CDN endpoint disable default value for compression #8610

zekth commented Sep 24, 2020

WodansSon left a comment

WodansSon Oct 22, 2020

zekth commented Oct 22, 2020

WodansSon left a comment

WodansSon commented Oct 22, 2020

zekth commented Oct 23, 2020

zekth commented Oct 23, 2020

WodansSon commented Oct 23, 2020

WodansSon commented Oct 24, 2020

WodansSon commented Oct 25, 2020

ghost commented Oct 29, 2020

zekth commented Nov 2, 2020

ghost commented Nov 24, 2020

feat: CDN endpoint disable default value for compression #8610

feat: CDN endpoint disable default value for compression #8610

Conversation

zekth commented Sep 24, 2020

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon Oct 22, 2020

Choose a reason for hiding this comment

zekth commented Oct 22, 2020

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon commented Oct 22, 2020

zekth commented Oct 23, 2020

zekth commented Oct 23, 2020

WodansSon commented Oct 23, 2020

WodansSon commented Oct 24, 2020

WodansSon commented Oct 25, 2020

ghost commented Oct 29, 2020

zekth commented Nov 2, 2020

ghost commented Nov 24, 2020