Regenerate CDN SDK for Swagger bugfixes. #16481
Conversation
fore5fire
requested review from
allenjzhang,
bquantump,
m-nash,
markcowl,
nisha-bhatia and
YalinLi0312
as code owners
| /// <param name="transforms">List of transforms</param> | ||
| public CookiesMatchConditionParameters(string selector, string operatorProperty, IList<string> matchValues, bool? negateCondition = default(bool?), IList<string> transforms = default(IList<string>)) | ||
| public CookiesMatchConditionParameters(string operatorProperty, string selector = default(string), bool? negateCondition = default(bool?), IList<string> matchValues = default(IList<string>), IList<string> transforms = default(IList<string>)) | ||
| { |
There was a problem hiding this comment.
This appears to a be a breaking change. A caller passing in string, string, Ilist, bool into this will now fail since the 3rd parameter is now a bool and not an IList.
There was a problem hiding this comment.
Ahh, you're right. Unfortunately it's a bug that needs to be fixed, and this wasn't a breaking change for the REST API specs since REST parameters aren't positional, so it has already been accepted there. What's the procedure for getting a breaking change released?
There was a problem hiding this comment.
Swapping the params around in swagger ought to fix it, since you are just swapping around optional params.
Since swagger is merged, may be able to do some customization.
Customization options are here:
https://github.com/Azure/autorest.csharp#customizing-the-generated-code
If you do customization, follow the path structure as is done in DNS:
https://github.com/Azure/azure-sdk-for-net/tree/master/sdk/dns/Azure.ResourceManager.Dns/src
Basically can make your custom class, it won't build because naming conflicts, but then re-run the generator and should pick this customization file up and regenerate based on it.
There was a problem hiding this comment.
I've done some work trying to use a customization to correct this issue, but it seems that only data plane libraries are currently using customizations, and I'm having a difficult time referencing Azure.Core like in the example you provided as required by the autorest .Net code generator for removing the backwards-incompatible constructor.
Are customizations of generated code supported for management modules? Adding Azure.Core requires different .Net versions than the management modules are supposed to reference, as stated in CONTRIBUTING.md.
There was a problem hiding this comment.
The example I gave of DNS is management, although it is track 2.
I think all that may need done is to make a file call /Customization/Models/CookiesMatchConditionParameters.cs
And then in there, change CookiesMatchConditionParameters to:
public CookiesMatchConditionParameters(string selector, string selector = default(string), IList matchValues = default(IList), bool? negateCondition = default(bool?), IList transforms = default(IList))
There was a problem hiding this comment.
Ahh I see, the difference between track 2 management and data plane wasn't quite clear from the doc I read, makes sense though.
Your solution will work as long as you're ok with having redundant constructors - The issue with importing Azure.Core only applies for deleting the non backwards compatible constructor. I'll push the change with both constructors, just let me know if that isn't acceptable.
| /// or not</param> | ||
| /// <param name="transforms">List of transforms</param> | ||
| public CookiesMatchConditionParameters(string selector = null, string operatorProperty = null, IList<string> matchValues = null, bool? negateCondition = null, IList<string> transforms = null) | ||
| { |
There was a problem hiding this comment.
I would think we would do this here:
public CookiesMatchConditionParameters(string operatorProperty, string selector = default(string), IList<string> matchValues = default(IList<string>), bool? negateCondition = default(bool?), IList<string> transforms = default(IList<string>))
{
this.CookiesMatchConditionParameters(operatorProperty, selector, negateCondition, matchValues, transforms);
}
I the this.CookiesMatchConditionParameters above should also be defined here identical to the generated one but marked internal so it is not exposed outside of this.
There was a problem hiding this comment.
Does this work with Autorest C# v2 generator? I'm able to add the new constructor because that doesn't require any change for the generated code, but the generator isn't removing the existing constructor when I replace it with an internal one in my customizations partial class. I can't find any documentation about customization using v2 of the C# generator, only for v3.
|
@bquantump, does the Autorest C# v2 generator support customizations? I can't find any customization documentation for it, only for v3, and it doesn't appear to respond to any of the v3 customizations. |
hmm, yes it looks like it won't work in V2. So I guess let's try writing a few test cases here to test the new constructor you made the new generated one which utilized both optional and non-optional parameters. |
… OriginUpdateTest.
|
Hi @bquantump. There hasn't been recent engagement on this PR. Would you please be so kind as to let us know if this is still an active work stream or if the PR should be closed out? |
jsquire
added
the
no-recent-activity
|
Since there hasn't been recent engagement, I'm going to close this out. Please feel free to reopen if you'd like to continue working on these changes. |
Regenerates the CDN SDK to include the following fixes: Azure/azure-rest-api-specs#11046, Azure/azure-rest-api-specs#10950