Skip to content

(MODULES-8438) Install 2019 #285

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 15, 2019
Merged

(MODULES-8438) Install 2019 #285

merged 1 commit into from
Jan 15, 2019

Conversation

RandomNoun7
Copy link
Contributor

This change allows the module to install and uninstall SQL 2019
idempotently. No further support is implemented.

@RandomNoun7 RandomNoun7 requested a review from Iristyle January 11, 2019 22:25
michaeltlombardi
michaeltlombardi reviewed Jan 14, 2019
View reviewed changes
@RandomNoun7 RandomNoun7 force-pushed the tickets/master/MODULES-8438-Install-2019 branch from 91b4840 to f42b60f Compare January 14, 2019 16:51
@michaeltlombardi
Copy link
Contributor

Happy to merge pending recording of functional review. :)

Iristyle
Iristyle reviewed Jan 14, 2019
View reviewed changes
metadata.json
@@ -38,6 +38,7 @@
"sql2014",
"sql2016",
"sql2017",
"sql2019",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add 2019 in summary on line 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@Iristyle
Copy link
Contributor

I think we're missing a couple changes to sql_testing_helpers.rb?

@Iristyle
Copy link
Contributor

Also missing a change to sqlserver/server_helper.rb?

@Iristyle
Copy link
Contributor

And maybe a small change to feature detection at

puppetlabs-sqlserver/spec/acceptance/z_last_sqlserver_features_spec.rb

Line 87 in 53d6f8a

if sql_version == '2016'
?

@Iristyle
Copy link
Contributor

Iristyle commented Jan 14, 2019 •
edited
Loading

And maybe a change at

puppetlabs-sqlserver/lib/puppet/provider/sqlserver_features/mssql.rb

Line 128 in 53d6f8a

installNet35(@resource[:windows_feature_source]) unless instance_version == SQL_2016

And a similar one at

puppetlabs-sqlserver/lib/puppet/provider/sqlserver_instance/mssql.rb

Line 104 in 53d6f8a

installNet35(@resource[:windows_feature_source]) unless instance_version == SQL_2016

@Iristyle
Copy link
Contributor

Some parameter description updates at

puppetlabs-sqlserver/lib/puppet/type/sqlserver_instance.rb

Lines 112 to 120 in 53d6f8a

newparam(:polybase_svc_account, :parent => Puppet::Property::SqlserverLogin) do
desc 'The account used by the Polybase Engine service. Only applicable for SQL Server 2016.'
end
newparam(:polybase_svc_password) do
desc 'The password for the Polybase Engine service account. Only applicable for SQL Server 2016.'
end

Iristyle
Iristyle reviewed Jan 15, 2019
View reviewed changes
README.md Outdated
@@ -1068,7 +1068,7 @@ Terminology differs somewhat between various database systems; please refer to t

## Limitations

SQL 2017 detection support has been added. This support is limited to functionality already present for other versions. No new SQL 2017 specific functionality has been added in this release.
SQL 2017 and 2019 detection support has been added. This support is limited to functionality already present for other versions. No new SQL 2017 specific functionality has been added in this release.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the notes about polybase for 2017 and 2019?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Iristyle
Iristyle reviewed Jan 15, 2019
View reviewed changes
README.md Outdated
@@ -1068,7 +1068,7 @@ Terminology differs somewhat between various database systems; please refer to t

## Limitations

SQL 2017 detection support has been added. This support is limited to functionality already present for other versions. No new SQL 2017 specific functionality has been added in this release.
SQL 2017 and 2019 detection support has been added. This support is limited to functionality already present for other versions. No new SQL 2017 specific functionality has been added in this release.

This module can manage only a single version of SQL Server on a given host (one and only one of SQL Server 2012, 2014 or 2016). The module is able to manage multiple SQL Server instances of the same version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should say 2016, 2017 or 2019 I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Iristyle
Iristyle reviewed Jan 15, 2019
View reviewed changes
README.md
@@ -20,7 +20,7 @@

## Overview

The sqlserver module installs and manages Microsoft SQL Server 2012, 2014, 2016 and 2017 on Windows systems.
The sqlserver module installs and manages Microsoft SQL Server 2012, 2014, 2016, 2017, 2019 on Windows systems.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make the corresponding changes in README_ja_JP.md or is that handled by translators @ehom ?

Iristyle
Iristyle suggested changes Jan 15, 2019
View reviewed changes
Copy link
Contributor

@Iristyle Iristyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A handful of minor things to follow up on...

@RandomNoun7 RandomNoun7 force-pushed the tickets/master/MODULES-8438-Install-2019 branch 2 times, most recently from 62aa228 to 05405de Compare January 15, 2019 18:23
@RandomNoun7
Copy link
Contributor Author

I'm reluctant to start fiddling with the spec helpers until we decide to actually put some ticketed effort into getting 2016-19 into the automated testing regime.

@RandomNoun7
Copy link
Contributor Author

The changes to the sqserverl_instance and sqlserver_features providers you requested addressed a bug where .net 3.5 was being installed in cases where it did not need to be. I have fixed that and tested that .net 3.5 not being installed works, and does not break the instance.

@RandomNoun7
(MODULES-8438) Install 2019
b8b5144 
This change allows the module to install and uninstall SQL 2019
idempotently. No further support is implemented.
@RandomNoun7 RandomNoun7 force-pushed the tickets/master/MODULES-8438-Install-2019 branch from 05405de to b8b5144 Compare January 15, 2019 19:56
@Iristyle
Copy link
Contributor

Excellent - so we ended up getting a bonus bugfix tagging along for the ride!

@RandomNoun7
Copy link
Contributor Author

@Iristyle Ok, these changes look good now. I've verified that it either installs or doesn't install .net 3.5 in the correct situations. Removing the do not merge label.

@Iristyle
Copy link
Contributor

Ok cool - so I'm going to merge this for now, noting that we still have a few changes to make in tests at:

Let's file a follow-on ticket to address those bits.

@Iristyle Iristyle merged commit d2a0f07 into puppetlabs:master Jan 15, 2019
@RandomNoun7 RandomNoun7 deleted the tickets/master/MODULES-8438-Install-2019 branch January 16, 2019 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Iristyle Iristyle Iristyle approved these changes

@michaeltlombardi michaeltlombardi michaeltlombardi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RandomNoun7 @michaeltlombardi @Iristyle