Make Reason required for attribute CallerShouldAuditAttribute

[RodgeFu]

Make Reason required for attribute CallerShouldAuditAttribute as discussed in #40548

[azure-sdk]

API change check

API changes are not detected in this pull request.

[RodgeFu]

@tg-msft, could you help to suggest how these compat issues could be handled? thanks.

[RodgeFu]

double check the error and found the Attribute had already been released in some sdk which means removing the parameterless ctor will introduce compatibility issue for these SDKs, so I am going to close this PR considering we are holding a high bar for compatibility for .Net as I understand.

@heaths , please let me know if you still think we should make the change with the compat issue handled in some way. thanks.

[heaths]

double check the error and found the Attribute had already been released in some sdk which means removing the parameterless ctor will introduce compatibility issue for these SDKs, so I am going to close this PR considering we are holding a high bar for compatibility for .Net as I understand.

@heaths , please let me know if you still think we should make the change with the compat issue handled in some way. thanks.

It's shared source - the source file is included and compiled into the binary, so there's no way to break it. Changing it and shipping a new copy in other SDKs - and eventually a newer version of the older SDK - won't be breaking. We should still consider making this required if our goal is indeed to require it. /cc @tg-msft

[RodgeFu]

@tg-msft, since the compatibility errors are reported from Data Plain code of storage and table, could you help to review and confirm the change looks good and how we can ignore these errors so that they won't block the merge? thanks.

copied one compatibility error here for your convenience:

/mnt/vss/_work/1/.nuget/packages/microsoft.dotnet.apicompat/5.0.0-beta.20467.1/build/Microsoft.DotNet.ApiCompat.targets(82,5): Error : CannotRemoveAttribute : Attribute 'Azure.Core.CallerShouldAuditAttribute' exists on 'Azure.Data.Tables.TableClient.GenerateSasUri(Azure.Data.Tables.Sas.TableSasBuilder)' in the contract but not the implementation.

[RodgeFu]

Code updated according to the feedbacks. @heaths , @tg-msft , please help to review and confirm you are fine with the change in Data Plane and how I can ignore these compatible errors in pipeline so that they won't block the merge. thanks.

[RodgeFu]

@heaths , do you know how we can skip these compatible errors from the pipeline check? Or any expertise I can ask for suggestions? thanks.

[heaths]

@JoshLove-msft or @tg-msft any ideas about these validation errors? Seems we're including these internal attributes in the contract, but since they are internal, they don't appear in the actual public API so there's a discrepancy.

[JoshLove-msft]

@JoshLove-msft or @tg-msft any ideas about these validation errors? Seems we're including these internal attributes in the contract, but since they are internal, they don't appear in the actual public API so there's a discrepancy.

Were the attribute updates shipped already in GA versions of the KV libraries?

[heaths]

An older version was, but we changed to require a reason parameter; however, this attribute is internal and shouldn't be considered part of the contract.

[JoshLove-msft]

An older version was, but we changed to require a reason parameter; however, this attribute is internal and shouldn't be considered part of the contract.

That's strange that ApiCompat would complain about it then. The only way I'm aware of to bypass this would be to modify the csproj to comment out the ApiCompatVersion, ship the new version, and finally uncomment (if it wasn't re-added automatically by the azure-sdk bot).

[heaths]

You might also try https://learn.microsoft.com/dotnet/fundamentals/apicompat/diagnostic-ids#how-to-suppress

[JoshLove-msft]

You might also try https://learn.microsoft.com/dotnet/fundamentals/apicompat/diagnostic-ids#how-to-suppress

I would not do this as we have no way of ensuring it gets removed. I think with the ApiCompatVersion approach, after the release the Azure SDK bot will add it back (but not 100% sure). /cc @weshaggard

[weshaggard]

You can create a baseline file for these errors. See https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/textanalytics/Azure.AI.TextAnalytics/src/ApiCompatBaseline.txt as an example. You will need to create a file with that exact name living next to the project file you want it to apply to and then copy the error from the output into that text file one error per line.