Introduce Azure Event Grid Notification Service #15

jonfuller · 2025-09-11T16:16:41Z

refers to issue #14 .

- added new service - added configuration - added tests

- created eventgrid wrapper - added additional payload tests

jonfuller · 2025-09-11T16:17:03Z

@arktronic-sep - I'm unsure the best way to test and validate the work that I did.

Would love to chat about it sometime! Let me know best way to move forward.

Copilot

Pull Request Overview

This PR introduces Azure Event Grid integration as a notification service for the sama monitoring application. The service sends structured events to Azure Event Grid when endpoint status changes occur or management operations are performed.

Adds EventGridNotificationService that implements INotificationService to send events to Azure Event Grid
Integrates the new service with dependency injection and configuration system
Includes comprehensive unit tests covering all notification scenarios

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
unit-tests/Services/EventGridNotificationServiceTests.cs	Comprehensive test suite covering all notification methods and event payload validation
sama/sama.csproj	Adds Azure.Messaging.EventGrid package dependency
sama/Startup.cs	Registers EventGridNotificationService and wrapper in DI container
sama/Services/SettingsService.cs	Adds configuration properties for Event Grid topic endpoint and access key
sama/Services/EventGridPublisherClientWrapper.cs	Testable wrapper around Azure EventGridPublisherClient
sama/Services/EventGridNotificationService.cs	Main service implementing INotificationService for Event Grid notifications

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

sama/Services/EventGridNotificationService.cs

unit-tests/Services/EventGridNotificationServiceTests.cs

Copilot · 2025-09-11T16:17:54Z

unit-tests/Services/EventGridNotificationServiceTests.cs

+            // Avoid async warning by not awaiting the result since we're testing the call was made
+            _ = result2;


The comment and discard pattern is unnecessary. The variable assignment can be removed entirely since the verification is already complete in the Received() call.

It's not entirely wrong. We should be awaiting rather than assigning to a result variable, though. (Why is it named result2 when there isn't a result1?)

unit-tests/Services/EventGridNotificationServiceTests.cs

Co-authored-by: Copilot <[email protected]>

sama/Services/EventGridPublisherClientWrapper.cs

sama/Services/EventGridNotificationService.cs

jonfuller · 2025-10-30T19:35:28Z

also, add place to configure settings for event grid.

Co-authored-by: Copilot <[email protected]>

use file scoped namespace make endpoint subjects consistent

jonfuller · 2025-10-31T12:37:22Z

@arktronic-sep - I think I'm all set.

I made the suggested changes, and added the settings fields.

Here is a screenshot showing that messages are flowing into the Azure Event Grid topic:

Please take a look when you can so we can get this sent over to IT.

Thanks!

arktronic-sep · 2025-11-04T17:50:49Z

sama/Services/EventGridNotificationService.cs

+
+namespace sama.Services;
+
+  public class EventGridNotificationService : INotificationService


Minor: spacing was not reformatted after switching namespace style

will def fix.

pushed an update.

arktronic-sep · 2025-11-04T17:52:42Z

sama/Services/EventGridNotificationService.cs

+          _eventGridWrapper = eventGridWrapper;
+      }
+
+      private static class EventTypes


Why a class here? It just contains const strings, so couldn't they exist in the parent class?

just to collect them all together. just how my brain wanted to organize them. no pref either way for me.

arktronic-sep · 2025-11-04T18:08:21Z

sama/Services/EventGridNotificationService.cs

+private static string Subject(string path) => $"Sama/{path}";
+private static string EndpointSubject(string path) => Subject($"endpoints/{path}");


Is Subject used anymore (aside from in EndpointSubject)?

Minor: spacing inconsistency

Minor: as these are methods, they should be verbified

Another minor question: why does the Subject capitalize the first letter in sama? Is this an Event Grid convention?

hehe - that's a good question! no, I don't think it's an EventGrid convention. I think I'll switch it to all lowercase sama.

Subject isn't used outside of EndpointSubject. That said, I still like the separation of concepts, if that makes sense.

arktronic-sep · 2025-11-05T13:46:43Z

unit-tests/Services/EventGridNotificationServiceTests.cs

+        [TestMethod]
+        public void NotifyMiscShouldExecuteInBackgroundForEndpointReconfigured()
+        {
+            var endpoint = CreateTestHttpEndpoint();
+
+            _service.NotifyMisc(endpoint, NotificationType.EndpointReconfigured);
+
+            _bgExec.Received(1).Execute(Arg.Any<Action>());
+        }
+
+        [TestMethod]
+        public void ShouldNotExecuteWhenTopicEndpointIsNotConfigured()
+        {
+            _settings.Notifications_EventGrid_TopicEndpoint.Returns((string)null);
+            var endpoint = CreateTestHttpEndpoint();
+            var result = new EndpointCheckResult { Success = true };
+
+            _service.NotifySingleResult(endpoint, result);
+
+            // Should still call Execute, but the async function inside should return early
+            _bgExec.Received(1).Execute(Arg.Any<Action>());
+        }


I'm a bit confused about these tests... they aren't validating the data received, which seems like it'd be important. And the tests named ShouldNotExecute* all expect that Execute is, in fact, called.

The first one - I would personally not validate the data, just verify that it executed. There are other tests that verify the payload.

Updated the rest of the ShouldNotExecute* tests. Good find!

arktronic-sep · 2025-11-05T13:49:44Z

unit-tests/Services/EventGridNotificationServiceTests.cs

+                )
+            );
+
+            // Avoid async warning by not awaiting the result since we're testing the call was made


I believe, verifying async calls is generally done with await. Not sure what warning this would be. For some examples, take a look at SlackNotificationServiceTests.

I think it was confused a bit. The warning shows up I think when you don't mark the method async. I marked them async and changed it back to await. All seems well!

- update sama subject name - remove result variables - update "shouldnotexecute..." tests

jonfuller · 2025-11-12T00:26:09Z

Alright - one more round if you want to take another look! Thanks!

jonfuller added 2 commits September 11, 2025 11:50

initial commit of Azure Event Grid

6319ae4

- added new service - added configuration - added tests

introduce service wrapper

22a5e3c

- created eventgrid wrapper - added additional payload tests

jonfuller requested a review from Copilot September 11, 2025 16:16

jonfuller assigned jonfuller and arktronic-sep Sep 11, 2025

jonfuller requested a review from arktronic-sep September 11, 2025 16:17

jonfuller unassigned arktronic-sep Sep 11, 2025

Copilot AI reviewed Sep 11, 2025

View reviewed changes

jonfuller and others added 3 commits September 11, 2025 12:27

Update unit-tests/Services/EventGridNotificationServiceTests.cs

fc3a5fd

Co-authored-by: Copilot <[email protected]>

Update unit-tests/Services/EventGridNotificationServiceTests.cs

4554ef8

Co-authored-by: Copilot <[email protected]>

Update unit-tests/Services/EventGridNotificationServiceTests.cs

5778769

Co-authored-by: Copilot <[email protected]>

jonfuller commented Oct 30, 2025

View reviewed changes

sama/Services/EventGridPublisherClientWrapper.cs Outdated Show resolved Hide resolved

sama/Services/EventGridNotificationService.cs Outdated Show resolved Hide resolved

sama/Services/EventGridNotificationService.cs Outdated Show resolved Hide resolved

jonfuller and others added 3 commits October 31, 2025 07:47

Update sama/Services/EventGridNotificationService.cs

c2c50c0

Co-authored-by: Copilot <[email protected]>

review recycle:

0d10321

use file scoped namespace make endpoint subjects consistent

add settings for Event Grid

8967200

jonfuller marked this pull request as ready for review November 4, 2025 14:55

arktronic-sep reviewed Nov 4, 2025

View reviewed changes

jonfuller added 2 commits November 4, 2025 12:59

fix whitespace

121c67b

fix compiler error

4c9cc1a

arktronic-sep reviewed Nov 4, 2025

View reviewed changes

rename subject methods

108857f

arktronic-sep reviewed Nov 5, 2025

View reviewed changes

code review:

6c7d9ee

- update sama subject name - remove result variables - update "shouldnotexecute..." tests

arktronic-sep approved these changes Nov 12, 2025

View reviewed changes

arktronic-sep merged commit 38b55bd into main Nov 12, 2025
1 check passed

arktronic-sep deleted the 14-azure-event-grid branch November 12, 2025 13:32

		// Avoid async warning by not awaiting the result since we're testing the call was made
		_ = result2;


		namespace sama.Services;

		public class EventGridNotificationService : INotificationService

		private static string Subject(string path) => $"Sama/{path}";
		private static string EndpointSubject(string path) => Subject($"endpoints/{path}");

Introduce Azure Event Grid Notification Service #15

Introduce Azure Event Grid Notification Service #15

Uh oh!

Conversation

jonfuller commented Sep 11, 2025

Uh oh!

jonfuller commented Sep 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonfuller commented Oct 30, 2025

Uh oh!

jonfuller commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonfuller commented Nov 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants