-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add sample application #527
Add sample application #527
Conversation
Codecov Report
@@ Coverage Diff @@
## master #527 +/- ##
=======================================
Coverage 37.64% 37.64%
=======================================
Files 78 78
Lines 2614 2614
Branches 458 458
=======================================
Hits 984 984
Misses 1485 1485
Partials 145 145 Continue to review full report at Codecov.
|
JustSaying.Sample.Restaurant.KitchenConsole/OrderPlacedEventHandler.cs
Outdated
Show resolved
Hide resolved
JustSaying.Sample.Restaurant.OrderingApi/Handlers/OrderReadyEventHandler.cs
Outdated
Show resolved
Hide resolved
JustSaying.Sample.Restaurant.OrderingApi/Models/CustomerOrderModel.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.
This is a great start - thanks for picking up this issue 😃
JustSaying.Sample.Restaurant.KitchenConsole/OrderPlacedEventHandler.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.
Great start 👍
JustSaying.Sample.Restaurant.OrderingApi/Controllers/OrdersController.cs
Outdated
Show resolved
Hide resolved
|
else | ||
{ | ||
// The real AWS environment will require some means of authentication | ||
//x.WithBasicCredentials("###", "###"); |
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.
If you add UserSecrets to the config providers, you could load these from config without needing to have it commented-out with placeholders.
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.
Yeah im a bit torn - im trying to be careful to leave the sample app in the simplest possible state where people can easily see what they might need to do or change in order to get it running on their box. Some have keys, others env variables etc. Not sure whats best.
Can go with the secrets if you think that's best but I think its a bit of a distraction from demonstrating the bus.
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.
on a similar note i was thinking of leaving a line like this for those who configure the region via environment variables too..?
//x.WithRegion(new Amazon.Runtime.EnvironmentVariableAWSRegion().Region);
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 guess that's more of an advanced use case? We wouldn't want to balloon the samples too much with all the possible things you could do. Also if you were using environment variables, you'd probably be using the .NET provider for that and binding them to IConfiguration
somehow I'd have thought.
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.
Yeah and it feels a bit orthogonal to the actual demonstration of your library. Its more demonstrating aspnet core configuration and the aws sdk
JustSaying.Sample.Restaurant.OrderingApi/JustSaying.Sample.Restaurant.OrderingApi.csproj
Outdated
Show resolved
Hide resolved
else | ||
{ | ||
// The real AWS environment will require some means of authentication | ||
//x.WithBasicCredentials("###", "###"); |
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.
As above regarding user secrets.
JustSaying.Sample.Restaurant.OrderingApi/Controllers/OrdersController.cs
Outdated
Show resolved
Hide resolved
Further feedback welcome |
@PeterKneale Thanks for your work on this. When I get some time today or tomorrow, I'll pull your branch locally and have a play around with what's there so far. 👍 |
Sure thing. Thanks for a great library |
JustSaying.Sample.Restaurant.KitchenConsole/OrderPlacedEventHandler.cs
Outdated
Show resolved
Hide resolved
JustSaying.Sample.Restaurant.KitchenConsole/OrderPlacedEventHandler.cs
Outdated
Show resolved
Hide resolved
JustSaying.Sample.Restaurant.Models/JustSaying.Sample.Restaurant.Models.csproj
Outdated
Show resolved
Hide resolved
I've cloned the the sample locally and had a play around with it. Here's a few bits of feedback on specifics, but overall this is pretty good. I think rather than Swagger and an "API", we should have a simple one-page MVC app with Bootstrap added, then when you run the sample it can present a UI where the order can be picked from a few options (e.g. pizza, curry, burger, etc.) with a few simple JavaScript calls to post the order and poll for and update (simple meta refresh?) when the "kitchen" says it is ready. This is more natural to navigate that doing a manual HTTP POST in the Swagger UI and provides feedback that's a bit more visual that watching two consoles, though that's still there for a more "low level" view. I'd also add some additional handlers that simulates payment and store the order in an in-memory database in the web app to persist things. Then if messages fail the process can be potentially recovered by leveraging the reliability of building on top of a messaging system. This would also demonstrate things like have multiple subscriptions to the same queue for different purposes. Then we can flesh out the sample with a few extra handlers/processes, something like:
|
I've addressed your basic feedback. I like the idea about building the web frontend and demonstrating a more thorough workflow.. I'm wondering what delivers the most advantage because I've got limited time. An area that I think also really needs to be added is an example of how to build dockerised system tests.
|
I think it's fine to keep the scope more confined here, than my original suggestion/comments. That way this delivers value into master sooner, and then it can be iterated on over time to have the option of a more "interactive" sample as well as the lower-level one here. Otherwise what you've alluded to above sounds reasonable. Often the acceptance/integration/system tests for our own workers that use JustSaying involve observing the side effects of the implementation details for message publishing and consumption (e.g. a record appears in a database). |
JustSaying.Sample.Restaurant.KitchenConsole/ConfigurationExtensions.cs
Outdated
Show resolved
Hide resolved
JustSaying.Sample.Restaurant.KitchenConsole/JustSaying.Sample.Restaurant.KitchenConsole.csproj
Outdated
Show resolved
Hide resolved
JustSaying.Sample.Restaurant.KitchenConsole/OrderPlacedEventHandler.cs
Outdated
Show resolved
Hide resolved
await new HostBuilder() | ||
.ConfigureAppConfiguration((hostContext, config) => | ||
{ | ||
config.SetBasePath(AppContext.BaseDirectory); |
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.
Is there a method on HostBuilder
anywhere that does all this by default? WebHostBuilder
does similar (or comes preconfigured that way), so would shave off a few lines of code if it's not needed or there's a one-liner.
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.
Just removing it seems to work...
executing this from the projects folder works
dotnet run
as does this from the solution root
dotnet run --project samples/JustSaying.Sample.Restaurant.KitchenConsole/JustSaying.Sample.Restaurant.KitchenConsole.csproj
JustSaying.Sample.Restaurant.OrderingApi/ConfigurationExtensions.cs
Outdated
Show resolved
Hide resolved
JustSaying.Sample.Restaurant.OrderingApi/Properties/launchSettings.json
Outdated
Show resolved
Hide resolved
@PeterKneale Just some minor comments around naming and comments, otherwise this looks really good. We're planning on moving the code around soon to tidy up the mess of folders that's arisen over time into a Again, thanks for all your work on this! 👍 |
|
Added a sample application consisting of three projects - a web api that publishes from a controller and subscribes via a background service - a console application that subscribes via a background service and publishes within an event handler - a shared models assembly containing messages
"AWSRegion": "eu-west-1", | ||
"AWSServiceUrl": "http://localhost:4100", | ||
"Logging": { | ||
"LogLevel": { |
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.
Should I be able to add "EventLog": "Trace"
here and have the messages contents logged?
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 think so - at some point in v7 we need to rename that, because it's easy to confuse with the Windows Event Log.
"AWSRegion": "eu-west-1", | ||
"AWSServiceUrl": "http://localhost:4100", | ||
"Logging": { | ||
"LogLevel": { |
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 think so - at some point in v7 we need to rename that, because it's easy to confuse with the Windows Event Log.
I think I'm happy with this as it is as a starting point. If the rest of the maintainers are happy, I think we could get this merged next week and further changes can be done in follow-up PRs over time. |
👍 |
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'm more than happy with this, fantastic work!
@PeterKneale Is there any more you're planning to do as part of this PR to justify the WIP status still, or should we merge this? |
Ready to merge - I cant remove the WIP label. |
Excellent work @PeterKneale! |
Added a sample application consisting of three projects
a web api that publishes from a controller and subscribes via a background service
a console application that subscribes via a background service and publishes within an event handler
a shared models assembly containing event dto's
Further work required:
PublishMetaData
relates to issue #476