-
Notifications
You must be signed in to change notification settings - Fork 864
Use MySqlDataSource in AddMySql by default
#2096
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
Changes from 2 commits
e7917f8
e58c262
479d8f0
d62373e
f4af126
c58614c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,3 @@ | ||||||
| using Microsoft.Extensions.DependencyInjection; | ||||||
| using Microsoft.Extensions.Diagnostics.HealthChecks; | ||||||
| using MySqlConnector; | ||||||
|
|
||||||
|
|
@@ -10,14 +9,19 @@ namespace HealthChecks.MySql; | |||||
| public class MySqlHealthCheckOptions | ||||||
| { | ||||||
| /// <summary> | ||||||
| /// The MySQL connection string to be used. | ||||||
| /// The MySQL data source to be used. This is the preferred way to specify the MySQL server to be checked. | ||||||
| /// </summary> | ||||||
| public string ConnectionString { get; set; } = null!; | ||||||
| public MySqlDataSource? DataSource { get; set; } | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exposing a
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you worried that this could lead to API misuse because someone could create a new I'm more worried that without this property, someone would be forced to set the I'm happy to make this
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After thinking about it for a while and experimenting with Npgsql in #2116 I got to the following conclusions: Let's make the parameterless This allows us to ensure that every instance of this type is valid: it has either the connection string or the data source. Never both. This is the only change I would like to make before merging this PR. @bgrainger thoughts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me; I'll update this PR with that pattern soon.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When developing this, I found that I didn't need a parameterless constructor nor to set the properties after construction. Thus, the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Great! 👍 |
||||||
|
|
||||||
| /// <summary> | ||||||
| /// The MySQL connection string to be used, if <see cref="DataSource"/> isn't set. | ||||||
| /// </summary> | ||||||
| public string? ConnectionString { get; set; } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// The query to be executed. | ||||||
| /// </summary> | ||||||
| public string CommandText { get; set; } = MySqlHealthCheckBuilderExtensions.HEALTH_QUERY; | ||||||
| public string? CommandText { get; set; } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// An optional action executed before the connection is opened in the health check. | ||||||
|
|
||||||
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.
We don't need these checks anymore, as the public ctors ensure that the state is always valid.
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.
For some reason I can't apply this suggestion. I am going to merge it now since it's not blocking
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.
#2129