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

Added support for SQL 2016 (Fixes #26) #31

Merged
merged 6 commits into from Jan 24, 2018

Conversation

Projects
None yet
6 participants
@Sagnad
Contributor

Sagnad commented Mar 15, 2017

This change adds support for 2016 as a SQL Server version number and maps it to major version 130 in the get SQL Server major version functions. The value 2016 was added to the schema MOFs for xDatabase and xDBPackage. The unit tests for xDatabase have been moved to Tests\Unit\ as per the DSC Resource Testing Guidelines.

This is to resolve issue #26 where @kwirkykat asked for help.


This change is Reviewable

@msftclas msftclas added cla-required and removed cla-required labels Mar 15, 2017

@Sagnad

This comment has been minimized.

Show comment
Hide comment
@Sagnad

Sagnad May 7, 2017

Contributor

Is there an issue with the fix? We're going to have to use an internal fixed module which is an overhead I'd like to avoid.

Contributor

Sagnad commented May 7, 2017

Is there an issue with the fix? We're going to have to use an internal fixed module which is an overhead I'd like to avoid.

@PowerShell PowerShell deleted a comment from msftclas Sep 26, 2017

@PowerShell PowerShell deleted a comment from msftclas Sep 26, 2017

@msftgits msftgits removed the cla-signed label Sep 26, 2017

@AmarettoSlim

This comment has been minimized.

Show comment
Hide comment
@AmarettoSlim

AmarettoSlim Jan 17, 2018

What's the deal here, is this getting accepted? It's passed all checks and has no conflicts ... it'd be nice to have this merged.

At this point the resource already needs SQL Server 2017 support :)

AmarettoSlim commented Jan 17, 2018

What's the deal here, is this getting accepted? It's passed all checks and has no conflicts ... it'd be nice to have this merged.

At this point the resource already needs SQL Server 2017 support :)

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Jan 19, 2018

Contributor

Wouldn't it be better to change the parameter $SqlServerVersion to be a string value (without a validate set) that takes a string, and that could be 120,130,135,136,140 and then a change wouldn't be require for each version of new SQL Server.
Althought it would be a breaking change, and not quite as good.

Contributor

johlju commented Jan 19, 2018

Wouldn't it be better to change the parameter $SqlServerVersion to be a string value (without a validate set) that takes a string, and that could be 120,130,135,136,140 and then a change wouldn't be require for each version of new SQL Server.
Althought it would be a breaking change, and not quite as good.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Jan 19, 2018

Contributor

:lgtm:

@kwirkykat this looks good to me. Can fly over this and merge whenever you have time?


Reviewed 5 of 8 files at r1, 2 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Contributor

johlju commented Jan 19, 2018

:lgtm:

@kwirkykat this looks good to me. Can fly over this and merge whenever you have time?


Reviewed 5 of 8 files at r1, 2 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@kwirkykat kwirkykat merged commit 14bbd8a into PowerShell:dev Jan 24, 2018

3 checks passed

code-review/reviewable 8 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment