Port Legacy SQL Server to FAKE 5 Module #2074

gfritz · 2018-08-30T21:00:41Z

Description

Ports SqlServer from FAKE 4 to a FAKE 5 module.

TODO

New (API-)documentation for new features exist (Note: API-docs are enough, additional docs are in help/markdown)
unit or integration test exists (or short reasoning why it doesn't make sense)
(if new module) the module has been linked from the "Modules" menu, edit help/templates/template.cshtml, linking to the API-reference is fine.
(if new module) the module is in the correct namespace
(if new module) the module is added to Fake.sln (dotnet sln Fake.sln add src/app/Fake.*/Fake.*.fsproj)
Fake 5 API guideline is honored

New (API-)documentation for new features exist

I could use some advice on how to get the new module to appear in the apidocs. I think I followed what Sql.DacPac did, but I cannot get Sql.SqlServer to appear.

Unit or integration test exist

If there is a SQL Server edition installed on the build server, I would be interested in trying to get some integration tests working. I don't think testing this module makes much sense without an instance of SQL Server to use.

matthid

Thanks a lot for your contribution.

I tried to pinpoint some places which might help to get this ready.
The only thing I'm a bit worried is getting the signatures right (as this introduces a lot of public API).

Regarding the tests: On circle-ci we can use any docker-container we might need, for other CI systems I'd assume there are other solutions. We just need to make sure we keep the build time low or move long running tests to other run configurations.

matthid · 2018-09-01T13:27:32Z

help/redirects/sqlserver.md

@@ -0,0 +1,6 @@
+# Packaging and Deploying SQL Databases


I don't think we need a redirection page as it didn't exist previously. We mainly have those to not break old links.

matthid · 2018-09-01T13:29:21Z

src/app/Fake.Sql.SqlServer/Fake.Sql.SqlServer.fsproj

@@ -0,0 +1,24 @@
+<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <PropertyGroup>
+    <TargetFramework>netstandard2.0</TargetFramework>


I think this is why the api docs are missing, we need <TargetFrameworks>netstandard2.0;net46</TargetFrameworks> (ie one target for the full framework) in order to generate docs. This is because FSharp.Formatting is not yet compatible with netstandard assemblies.

matthid · 2018-09-01T13:33:34Z

src/app/Fake.Sql.SqlServer/Sql.SqlServer.fs

+    let dropAndCreateDatabase connectionString =
+        connectionString
+        |> getServerInfo
+        |> dropDb


I'm not sure if this are the signatures we want (ie returning ServerInfo instead of unit). This makes those methods weird to use on their own and you need |> ignore everywhere?

Not sure just my thinking around that

I could not decide which way to go either, so I stuck with the signatures the old module had.

I like returning the ServerInfo record from attach, detach, killAllProcesses because it indicates that something on the serverInfo record changed. The underlying calls to serverInfo.Server.<Attach/Detach/KillAllProcesses> read like they modify the serverInfo.Server object.

I see that those functions could return unit instead because serverInfo is already changed by serverInfo.Server mutating, and usually unit indicates some side effect happened.

Now that I write this all out, it sounds like returning unit is a clearer signature with the bonus of less |> ignore. I will get going on that and hopefully push it today.

gfritz · 2018-09-09T23:38:42Z

Current status - making some integration tests for this on appveyor.

matthid · 2018-09-15T16:37:52Z

src/app/Fake.Sql.SqlServer/Sql.SqlServer.fs

+
+    type ServerInfo = {
+        Server: Server
+        ConnBuilder: SqlConnectionStringBuilder }


Somehow I don't like this mutable design, but if you can persuade this that this improves the API/design or is needed for some reason I'm happy to merge it (we might consider adding that as comment)

I ported what FAKE 4 had, and I also do not like it. I will try some things out in a branch to not hold up this PR if that is ok.

matthid · 2018-09-15T16:39:23Z

Current status - making some integration tests for this on appveyor.

Yes that would be nice indeed, however if that turns out to be too complicated feel free to tell so. Maybe we can add some small docs on how to test this manually.

matthid · 2018-09-22T10:09:35Z

Should we release this as is? Can you resolve the conflicts?

gfritz · 2018-09-25T16:34:52Z

My build fails for the same reason as the build for Release 5.7.2 did. Is that something I should fix?

I agree about merging this. I won't hold this up with integration tests either. Turns out they were tough to get working on the CI.

matthid · 2018-09-25T16:48:00Z

Ah yes sorry I'm just in the process of getting it green again :(
If you want to get this green you could rebase on an order commit (like aa803f5) or wait until I managed to fix it and pull again

matthid · 2018-09-25T16:53:08Z

Wait, somehow your commits disappeared?

gfritz · 2018-09-25T17:00:40Z

Yikes, I am not doing well here. Let me get them back up.

- rebase on older good commit aa803f5

gfritz · 2018-09-25T20:12:31Z

Here is the rundown:

Thanks for following up! I did not mean this to drag on for so long.
I agree we should release as-is;
super simple integration tests live on another branch, but I have trouble getting the Target using DotNet.exec ... that succeeds locally to succeed on appveyor - some kind of path error;
Would you like short manual testing docs in another PR? They could be "run dotnet path-to-sqlserver-integrationtests-dll --summary".

matthid · 2018-09-27T16:33:50Z

super simple integration tests live on another branch, but I have trouble getting the Target using DotNet.exec ... that succeeds locally to succeed on appveyor - some kind of path error;

Feel free to open a PR to improve visibility.

Would you like short manual testing docs in another PR? They could be "run dotnet path-to-sqlserver-integrationtests-dll --summary".

Feel free to add a Readme to the test project and a note linking to the readme in the contributors page.

matthid · 2018-09-27T16:34:21Z

I re-started the PR as it looked like some temporary thing, will merge when green

matthid · 2018-09-27T18:28:19Z

Thanks for taking care of this :)

matthid · 2018-10-04T08:40:21Z

@gfritz Just to let you know. @BlythMeister informed me that apparently we cannot release this because of #2007 ...
I don't know what to do besides disabling the project file for now (leaving the code in the repository for now)

gfritz · 2018-10-06T01:15:15Z

Well crud, I did not run into this. Wish I could have caught it earlier.

matthid reviewed Sep 1, 2018

View reviewed changes

matthid reviewed Sep 15, 2018

View reviewed changes

gfritz changed the title ~~Port Legacy SQL Server to FAKE 5 Module~~ WIP - Port Legacy SQL Server to FAKE 5 Module Sep 25, 2018

gfritz force-pushed the port-sqlserver-to-fake5 branch from 7c11ec7 to 2ee1519 Compare September 25, 2018 15:10

matthid changed the title ~~WIP - Port Legacy SQL Server to FAKE 5 Module~~ Port Legacy SQL Server to FAKE 5 Module Sep 25, 2018

port sql server fake4 module to fake5

336abd7

- rebase on older good commit aa803f5

gfritz force-pushed the port-sqlserver-to-fake5 branch from 2ee1519 to 336abd7 Compare September 25, 2018 19:19

matthid approved these changes Sep 27, 2018

View reviewed changes

matthid merged commit 0ccb7d0 into fsprojects:release/next Sep 27, 2018

This was referenced Sep 27, 2018

Release 5.8 #2104

Merged

Release 5.8.4 #2116

Merged

Numpsy mentioned this pull request Sep 1, 2024

Remove Microsoft.SqlServer.SqlManagementObjects from the fakemodule group on paket.dependencies #2813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port Legacy SQL Server to FAKE 5 Module #2074

Port Legacy SQL Server to FAKE 5 Module #2074

gfritz commented Aug 30, 2018 •

edited

Loading

matthid left a comment

matthid Sep 1, 2018

matthid Sep 1, 2018

matthid Sep 1, 2018

gfritz Sep 5, 2018

gfritz commented Sep 9, 2018

matthid Sep 15, 2018

gfritz Sep 25, 2018

matthid commented Sep 15, 2018

matthid commented Sep 22, 2018

gfritz commented Sep 25, 2018

matthid commented Sep 25, 2018

matthid commented Sep 25, 2018

gfritz commented Sep 25, 2018

gfritz commented Sep 25, 2018 •

edited

Loading

matthid commented Sep 27, 2018

matthid commented Sep 27, 2018

matthid commented Sep 27, 2018

matthid commented Oct 4, 2018

gfritz commented Oct 6, 2018

Port Legacy SQL Server to FAKE 5 Module #2074

Port Legacy SQL Server to FAKE 5 Module #2074

Conversation

gfritz commented Aug 30, 2018 • edited Loading

Description

TODO

New (API-)documentation for new features exist

Unit or integration test exist

matthid left a comment

Choose a reason for hiding this comment

matthid Sep 1, 2018

Choose a reason for hiding this comment

matthid Sep 1, 2018

Choose a reason for hiding this comment

matthid Sep 1, 2018

Choose a reason for hiding this comment

gfritz Sep 5, 2018

Choose a reason for hiding this comment

gfritz commented Sep 9, 2018

matthid Sep 15, 2018

Choose a reason for hiding this comment

gfritz Sep 25, 2018

Choose a reason for hiding this comment

matthid commented Sep 15, 2018

matthid commented Sep 22, 2018

gfritz commented Sep 25, 2018

matthid commented Sep 25, 2018

matthid commented Sep 25, 2018

gfritz commented Sep 25, 2018

gfritz commented Sep 25, 2018 • edited Loading

matthid commented Sep 27, 2018

matthid commented Sep 27, 2018

matthid commented Sep 27, 2018

matthid commented Oct 4, 2018

gfritz commented Oct 6, 2018

gfritz commented Aug 30, 2018 •

edited

Loading

gfritz commented Sep 25, 2018 •

edited

Loading