Added default customDomainHttpsParameters value#11777
Conversation
ghost
added
the
customer-reported
|
Thank you for your contribution ChrisKlug! We will review the pull request and get back to you soon. |
|
|
|
@ramya-rao-a Is there a way to add this default configuration in code generator ? maybe change the swagger ? Otherwise it may get override when next time release it ? |
ramya-rao-a
added
Mgmt
I just confirmed with Service Team, the certicateType default value is actually related with the profile SKU. We can't approve this PR. It depends on the profile SKU - Standard_Microsoft uses dedicated, Standard_Akamai, Standard_Verizon, and Premium_Verizon use shared. Also the protocol type depends on the SKU, you can see in the example I sent earlier: https://github.com/Azure/azure-cli/blob/31584aaca4d5f02fe795f1d75172e38965ad67ab/src/azure-cli/azure/cli/command_modules/cdn/custom.py#L558-L585 @ramya-rao-a @joheredi Then in this case, Is there a way to put this kind of logic in the swagger ? |
|
Ok, that makes sense, and I could update the PR with logic similar to the Python example you provided. However, it doesn't feel very valuable if there is no way to get it into the code. I would say that the Swagger would be able to define the properties as mandatory, which would make it a breaking change, but it wouldn't be able to add custom logic. On the other hand, I assume that there has to be some way to get the code in there during generation as well. Do you want me to update the PR with the SKU code, or should I just abandon it? Unfortunately it causes problems for people, so I think solving it would be the best solution... |
It seems unlikely for user to add that complex default value logic in the swagger. @lsmith130 Is it possible to add the default value logic in the service backend side? If yes, will it take a long time to get it deployed to prod? @ChrisKlug I would suggest to change the default logic in the JS SDK to unblock customer first. @ramya-rao-a @joheredi How do you think of it? |
|
Well, for the sake of things, I added the code for creating different values per SKU. If nothing else, it might help someone implement it properly in the future... |
|
Adding this default logic in the swagger doesn't seem possible, and the logic seems very specific to this service to be added in the generator. If I'm understanding this correctly, it seems like this is a bug either in the Swagger where the parameter is marked as optional, or in the service where it is returning an error for an optional parameter not being set. If it is a Swagger error, I think the right thing to do would be to update it to have this property as required. Unfortunately it will be a breaking change If it is a Service bug it should be fixed in the service and since no Swagger fix would be needed, there won't be breaking changes @qiaozha who can help us confirm if this is a service bug? |
|
@lsmith130 is from the service team. Could you please confirm that? Thankx |
|
I agree with you that either the swagger has problem or the service side need to add that default logic to handle null. Just think both of the two processes might take a couple of days. To temporarily fix customer issue, maybe approve this PR is the best? @joheredi |
|
I did some digging in the service code, it looks like this was intended to be optional, but our request validation is setup in a way that makes the arguments required with no easy workaround. Fixing this in the service would require significant rearchitecture of this feature, unfortunately. If possible, please handle this on the client side. |
|
@ramya-rao-a @joheredi Then in this case. We should approve this manual fix right? |
|
Minor version has been bumped to 5.1 |
|
@ramya-rao-a @joheredi Could you please let me know if this works based on the comments above? If there's no objections from you, I will merge this PR. |
| }, | ||
| enableCustomHttpsOperationSpec, | ||
| callback) as Promise<Models.CustomDomainsEnableCustomHttpsResponse>; | ||
| let newOptions: Models.CustomDomainsEnableCustomHttpsOptionalParams = {}; |
There was a problem hiding this comment.
The next time we auto generate this package, the hand written code here will get over written. Please use the #region notation to mark this block of code as hand written. This way when we review the next auto generated code and see this block deleted, we can add it back knowing that this was added intentionally.
joheredi
left a comment
joheredi
left a comment
There was a problem hiding this comment.
Looks good! Please take @ramya-rao-a's suggestion to mark the manually added regions to make sure your changes are not lost the next time this package is re-generated.
|
Cool! I have added region comments to mark the code I have added. Hopefully this won't take up anymore of your time! Cheers! |
5 participants
As issue #11567 says, there is an issue with enabling HTTPS for custom domains in the CDN. This is caused by the API requiring a customDomainHttpsParameters parameter. However, in the current version, this property is optional, causing problems.
This PR adds a hopefully reasonable default value if the property has not been set. This should make it usable if left out, and make it backwards compatible to v4.2 where it worked without providing it.