Conversation
|
@scottaddie, @ellismg and @ahamad-MS Could you help review this PR? Thanks a lot. |
|
@scottaddie, @ellismg and @ahamad-MS Could you help review this PR? Thanks a lot. |
1 similar comment
|
@scottaddie, @ellismg and @ahamad-MS Could you help review this PR? Thanks a lot. |
| @@ -72,7 +72,7 @@ private List<EventGridEventModel> AdaptCloudEvent(string t) | |||
| private List<EventGridEventModel> AdaptEventGridEvent(string t) | |||
There was a problem hiding this comment.
Why do we need EventGridEventModel - can we just use EventGridEvent?
|
|
||
| [JsonProperty("time")] | ||
| public string Time { get; set; } | ||
| public DateTimeOffset Time { get; set; } |
There was a problem hiding this comment.
Could we just use the CloudEvent type from Azure.Core - https://docs.microsoft.com/en-us/dotnet/api/azure.messaging.cloudevent?view=azure-dotnet
| EventGridEvent eventGrid = new EventGridEvent(model.Subject, model.EventType, model.DataVersion, model.EventData); | ||
| // EventGrid validation message | ||
| if (model.EventType == EventTypes.EventGridSubscriptionValidationEvent) | ||
| if (eventGrid.TryGetSystemEventData(out object systemEvent)) |
There was a problem hiding this comment.
The systemEvent we get back would still need to be checked to make sure it is the correct type, i.e. SubscriptionValidationEventData. Or we can continue checking the event type, and then do eventGrid.Data.ToObjectFromJson().
| <ItemGroup> | ||
| <PackageReference Include="Azure.Messaging.EventGrid" Version="4.10.0" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="12.0.3" /> | ||
| <PackageReference Include="System.Memory.Data" Version="6.0.0" /> |
There was a problem hiding this comment.
This package will get pulled in by Azure.Messaging.EventGrid. Unless we need 6.0.0, I'd just remove this.
| EventData = eventGridEvent.Data, | ||
| EventTime = eventGridEvent.EventTime.ToString("o") // https://docs.microsoft.com/en-us/dotnet/api/system.datetime.tostring?view=netcore-3.1 | ||
| EventTime = eventGridEvent.EventTime // https://docs.microsoft.com/en-us/dotnet/api/system.datetime.tostring?view=netcore-3.1 | ||
| }; |
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Azure.EventGrid" Version="3.2.0" /> | ||
| <PackageReference Include="Azure.Messaging.EventGrid" Version="4.10.0" /> |
There was a problem hiding this comment.
| <PackageReference Include="Azure.Messaging.EventGrid" Version="4.10.0" /> | |
| <PackageReference Include="Azure.Messaging.EventGrid" Version="4.11.0" /> |
|
@JoshLove-msft - Updated according to your comments, could you please re-review it? Thanks! |
|
@JoshLove-msft - We have updated according to your comments, please help re-review it. Thanks a lot. |
1 similar comment
|
@JoshLove-msft - We have updated according to your comments, please help re-review it. Thanks a lot. |
4 participants
Update EventGrid SDK to T2
Update files:
@scottaddie, @ellismg and @ahamad-MS for notification.