-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
EventGrid Dataplane SDK: Re-generated SDK to include new event types + updated corresponding unit tests #4691
Conversation
[JsonConverter(typeof(JobStateConverter))] | ||
public struct JobState : System.IEquatable<JobState> | ||
[JsonConverter(typeof(StringEnumConverter))] | ||
public enum JobState |
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.
Question to the reviewer:
This model is based on the swagger definition in https://github.com/Azure/azure-rest-api-specs/blob/master/specification/eventgrid/data-plane/Microsoft.Media/stable/2018-01-01/MediaServices.json. There have been no changes in this swagger since May 18, hence I am looking to understand why this got re-generated (new autorest behavior?). Can you please help me understand this?
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.
@kalyanaj The behavior per se would be the same for both ends, i.e., the REST service sending/receiving this enum value vs the client.
Its a better/cleaner implementation of extensible enum that we released over the past few months. We do have some flags available to revert to the legacy implementation.
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.
Thanks @dsgouda for the info!
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.
Looks good for the most part
/// Hub. | ||
/// </summary> | ||
[JsonProperty(PropertyName = "opType")] | ||
public string OpType { get; set; } |
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.
Removing properties is a breaking change, please bump the major version number for nuget package
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.
OK, updating the version number in the csproj to 2.0.0. Also, updating the AssemblyFileVersion to 2.0.0.0 and AssemblyVersion to 2.0.0.0. Please let me know in case that's not what you meant...
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.
Yep, that is what I meant, here is an example
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.
Thanks, I have updated this in the latest commit.
[JsonConverter(typeof(JobStateConverter))] | ||
public struct JobState : System.IEquatable<JobState> | ||
[JsonConverter(typeof(StringEnumConverter))] | ||
public enum JobState |
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.
@kalyanaj The behavior per se would be the same for both ends, i.e., the REST service sending/receiving this enum value vs the client.
Its a better/cleaner implementation of extensible enum that we released over the past few months. We do have some flags available to revert to the legacy implementation.
Thanks @dsgouda for the review. If you have any additional comments, please let me know. If it looks good, can you please merge this? We are looking to release this tomorrow. |
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.
LGTM
Description
EventGrid Dataplane SDK: Re-generated SDK to include new event types + updated corresponding unit tests. Swagger updates that trigger this are:
Azure/azure-rest-api-specs#3504
Azure/azure-rest-api-specs#3448
Azure/azure-rest-api-specs#3174
This checklist is used to make sure that common guidelines for a pull request are followed.
Note: For the two IoT events (DeviceCreated/DeviceDeleted), the IoT team has removed two properties from the data schema. IoT events for EventGrid is currently in preview, hence this change has been done before they GA their EventGrid support.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csproj
andAssemblyInfo.cs
files have been updated with the new version of the SDK.