-
Notifications
You must be signed in to change notification settings - Fork 11
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 Akka.Hosting extension methods #199
Add Akka.Hosting extension methods #199
Conversation
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.
Left some feedback on the Akka.Hosting API surface area
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Akka.Persistence.Hosting" /> |
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, version range for the MSFT.EXT packages is controlled by the base Akka.Hosting library.
{ | ||
public class JournalSettingsSpec | ||
{ | ||
[Fact(DisplayName = "Default options should not override default hocon config")] |
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
{ | ||
public sealed class JournalDatabaseOptions | ||
{ | ||
public static JournalDatabaseOptions Default => new(DatabaseMapping.Default) |
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 like the convenience of having ready-made "defaults" for each of the major supported databases here.
We need to add an example of using the hosting bits as well |
I think doing some integration tests is probably the best way to go - let's use the Akka.Hosting.TestKit to do that. |
protected override StringBuilder Build(StringBuilder sb) | ||
{ | ||
if (string.IsNullOrWhiteSpace(ConnectionString)) | ||
throw new ArgumentNullException(nameof(ConnectionString), $"{nameof(ConnectionString)} can not be null or empty."); |
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 where connection string value is being checked, we make sure that the user doesn't shoot themselves on the foot here.
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
No description provided.