-
Notifications
You must be signed in to change notification settings - Fork 589
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
Event Grid Explorer - Adding Support for Event Grid V2 #737
Conversation
… Explorer Implemented Event Grid V2 library and components to view and manage Event Grid V2 entities
…plorer into EventGridExplorer
Updated Event Grid Explorer README
Updated Event Grid Explorer README
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.
Excellent job, @t-wangamy.
I left a few comments, change requests, and questions.
src/EventGridExplorerLibrary/.pkgrefgen/EventGridExplorer.csproj.nuget.dgspec.json
Outdated
Show resolved
Hide resolved
src/EventGridExplorerLibrary/.pkgrefgen/EventGridExplorer.csproj.nuget.g.props
Outdated
Show resolved
Hide resolved
src/EventGridExplorerLibrary/Management/EventGridManagementClient.cs
Outdated
Show resolved
Hide resolved
src/EventGridExplorerLibrary/Management/EventGridManagementClient.cs
Outdated
Show resolved
Hide resolved
src/EventGridExplorerLibrary/Management/Models/DeliveryConfiguration.cs
Outdated
Show resolved
Hide resolved
Made various fixes, including updating the connect form
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.
Almost there. A few comments where missed out.
src/EventGridExplorerLibrary/Management/Models/DeliveryConfiguration.cs
Outdated
Show resolved
Hide resolved
Updated EventGridExplorer README and removed Service Bus root node menu strip from Event Grid root node
…/ServiceBusExplorer into EventGridExplorer
@SeanFeldman Today is actually the last day of my internship so I pushed the last changes for the Event Grid Explorer README. Thank you for reviewing my work and for your helpful feedback! I think from here my team will help with making any additional changes |
@t-wangamy, thank you so much for the contribution. I'll work on bringing it to the merge point. Please let me know who can continue from your team. Maybe tag them here. Thank you and good luck! |
Hi @SeanFeldman, I will continue. |
@hillaryc, great! would you be able to review the two remaining questions? Thank you. |
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.
A few clarifications, @hillaryc
@hillaryc, would you be able to assist with the questions/clarifications? I'd like to wrap up the PR. Thank you for your help. |
Yes, I will work on this today. 😊
Best Regards,
Hillary
From: Sean Feldman ***@***.***>
Sent: Friday, September 22, 2023 8:26 AM
To: paolosalvatori/ServiceBusExplorer ***@***.***>
Cc: Hillary Caituiro Monge ***@***.***>; Mention ***@***.***>
Subject: Re: [paolosalvatori/ServiceBusExplorer] Event Grid Explorer - Adding Support for Event Grid V2 (PR #737)
@hillaryc<https://github.com/hillaryc>, would you be able to assist with the questions/clarifications? I'd like to wrap up the PR. Thank you for your help.
—
Reply to this email directly, view it on GitHub<#737 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AT3VM5QA4PB43M5AUAFPOP3X3WUZZANCNFSM6AAAAAA3THR3GQ>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
Thank you, @hillaryc. |
I believe I replied all questions :) Let me know if anything. @SeanFeldman |
Hi @SeanFeldman, let me know if there is anything pending to be done to merge this. |
@hillaryc, code-wise, except for the lack of information on how to regenerate the auto-generated code, the rest is OK. These are the issues I've run into while manually testing the branch.
Error message (redacted)
The last issue is a blocker. Since there's no simple connection string and the only way to connect is to provide resource group, subscription ID, and the EG namespace (in addition to the API version), it's unclear how to troubleshoot the connectivity when an error such as this is encountered. |
Status update The PR is returning to a draft mode to address usability and maintainability. |
* Updated UI according to review. * Set default and cancel buttons. Changed window looks. * Changed the retry timout parsing
|
||
namespace ServiceBusExplorer.Helpers | ||
{ | ||
public class EventGridException : Exception |
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 - what does this custom exception type bring that is not handled by Exception
?
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.
I had to make this custom exception so I can throw them without having to hit the client and have the client throw the exception.
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.
I must have missed that use. Could you point to where it's used? Thanks.
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.
Found it here. That use is on the form. When an exception is thrown, the form logs it and swallows the exception. But with EventGrid, you're going to throw? Could you help me understand why it would need to be re-thrown?
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.
My idea for the user experience is that I can check the format of the filter key, operator and value and verify them without depending on the armclient to do the verification.
Yes, the form will log the error, but it will still go ahead and still hit the client.
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.
I might be missing something and would love to meet today to resolve it if possible.
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.
I might be missing something and would love to meet today to resolve it if possible.
Let's continue the discussion at the method level comments, no need for a meeting 🙂
src/EventGridExplorerLibrary/Management/EventGridFilterValues.cs
Outdated
Show resolved
Hide resolved
src/EventGridExplorerLibrary/Management/EventGridFilterValues.cs
Outdated
Show resolved
Hide resolved
src/EventGridExplorerLibrary/Management/Clients/EventGridControlPlaneClient.cs
Outdated
Show resolved
Hide resolved
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.
Please make sure it builds without errors and warnings.
src/ServiceBusExplorer/Forms/CreateEventGridSubscriptionForm.cs
Outdated
Show resolved
Hide resolved
src/ServiceBusExplorer/Forms/CreateEventGridSubscriptionForm.cs
Outdated
Show resolved
Hide resolved
src/EventGridExplorerLibrary/Management/Clients/EventGridControlPlaneClient.cs
Outdated
Show resolved
Hide resolved
src/EventGridExplorerLibrary/Management/Clients/EventGridControlPlaneClient.cs
Outdated
Show resolved
Hide resolved
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.
Much better, @harrieoriowo 🙂
Let me know when you're done-done (out of draft) and I'll give it a final review.
I believe I am done-done. I have opened it for final review! |
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.
Nullable reference type warnings - either don't use it or apply on the file level. Please don't turn it off at the line-by-line level.
@harrieoriowo, overall, looks good. Once you're done with the last review comments, I'm 👍 to merge. Thank you. |
For my Summer 2023 internship project under the Azure Messaging Event Grid team, I developed the Event Grid Explorer. This is a tool integrated in Service Bus Explorer for viewing and managing Event Grid V2 entities, including namespaces, topics and subscriptions, as well as provide create/delete operations for topics and subscriptions, publish/receive operations for events, and acknowledge/release/reject operations for events.
Currently, the Event Grid Explorer features the functionality that is available for the preview version of Event Grid V2.
This additional support for Event Grid V2 enables users to test event delivery with ease, contributing to the adoption of the new service.
The modifications include creating new Controls and Forms under ServiceBusExplorer as well as implementing an EventGridExplorerLibrary to interact with the Event Grid V2 SDK functions. This library also contains additional files under the Management folder that set up Event Grid related objects.