-
Notifications
You must be signed in to change notification settings - Fork 32
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
Automatically Create Database if Not Present #49
Conversation
async Task CreateDatabaseAsync(string databaseName, SqlConnection connection) | ||
{ | ||
using SqlCommand command = connection.CreateCommand(); | ||
command.CommandText = $"CREATE DATABASE {Identifier.Escape(databaseName)} COLLATE Latin1_General_100_BIN2_UTF8"; |
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 use Identifier.Escape(string)
to handle possibly non-standard names as well as protect against any malicious queries while we're setting up.
LogAssert.ExecutedSqlScript("permissions.sql"), | ||
LogAssert.SprocCompleted("dt._UpdateVersion")); | ||
LogAssert | ||
.For(this.logProvider) |
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 refactor may be a bit much.
I wanted a "simple" way to convey that some logs were expected in any order, some were expected in a certain order, and some where condition -- all for the same execution!
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 this design! I was thinking it would be necessary to have more flexible log validation at some point, and it looks like you've added that for me, which I appreciate. :)
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.
Thanks! This was a compromise in my head over creating some sort of LINQ-like decorators (e.g. the functions Contains
or Sequence
could return an interface ILogAssertion
).
LogAssert.ExecutedSqlScript("permissions.sql"), | ||
LogAssert.SprocCompleted("dt._UpdateVersion"), | ||
// 2nd | ||
LogAssert.AcquiredAppLock(), |
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 the other workers, I don't think we can guarantee their response codes other than success; there may or may not be contention for the lock.
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 an excellent PR and I really appreciate the work and care that went into it. I just have a few minor feedback items and then I'd love to get this merged.
LogAssert.ExecutedSqlScript("permissions.sql"), | ||
LogAssert.SprocCompleted("dt._UpdateVersion")); | ||
LogAssert | ||
.For(this.logProvider) |
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 this design! I was thinking it would be necessary to have more flexible log validation at some point, and it looks like you've added that for me, which I appreciate. :)
test/DurableTask.SqlServer.Tests/Logging/LogAssertExtensions.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.
LGTM! Thanks for this contribution! It will definitely reduce the setup burden for many users, and the new test functionality will be helpful as well.
No problem! Happy to help! |
As per #48, this change allows the worker to create the database automatically if it doesn't exist. Because the worker must gracefully handle the possibility the DB hasn't been created, a few changes needed to be made:
Initial Catalog
orDatabase
instead of the user-specified one. As such, the worker now initially connects to the system databasemaster
Database
, so now the lock is only obtained after the presence of the database has been verified.