Skip to content
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

Discussion: BREAKING CHANGE: Host and instance parameter is not consistent throughout the module #308

Closed
johlju opened this issue Jan 12, 2017 · 15 comments · Fixed by #917
Closed
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request.

Comments

@johlju
Copy link
Member

johlju commented Jan 12, 2017

Details of the scenario you try and problem that is occurring:
I'm a big fan of consistency. The host parameter and the instance parameter is not consistent throughout the module.

I suggest we try to come to an agreement to which names we are gonna use and add that to the contribution guidelines so we can be more consistent in the future.

I would also want to discuss what you think about trying to make all of these consistent. Is that unnecessary? (it would be a module size breaking change)

I compiled a list of each module. Some are missing both parameters and some are missing either parameter. For this discussion I assume they do need them. Those resource that do not need either parameter, those are removed from the total sum.

Below is a list for each module (includes changes that will merge eventually). But to summarize.

The unique host parameters (order by most to fewest):

Host parameter name Number of resources
SQLServer 17
(missing parameter) 6
NodeName 4

Question 1: The name SQLServer is used by most resources. Is that the best name it can have? From the top of my head there is also these that could be suitable; SQLServerName, NodeName, ServerName, ComputerName, HostName

The unique instance parameters (order by most to fewest):

Instance parameter name Number of resources
InstanceName 11
SQLInstanceName 11
SQLInstance 5
(missing parameter) 2

Question 2: The name InstanceName is used by most resources. Is that the best name it can have?

Compiled list for all resources

Resource Host parameter name Instance parameter name
xSQLDatabaseRecoveryModel SQLServer SQLInstanceName
xSQLServerAlias (not needed) (not needed)
xSQLServerAlwaysOnAvailabilityGroup SQLServer SQLInstanceName
xSQLServerAlwaysOnAvailabilityGroupReplica SQLServer SQLInstanceName
xSQLServerAlwaysOnService SQLServer SQLInstance
xSQLServerAvailabilityGroupListener NodeName InstanceName
xSQLServerConfiguration SQLServer SQLInstanceName
xSQLServerDatabase SQLServer SQLInstanceName
xSQLServerDatabaseOwner SQLServer SQLInstance
xSQLServerDatabasePermissions SQLServer SQLInstanceName
xSQLServerDatabaseRole SQLServer SQLInstanceName
xSQLServerEndpoint SQLServer SQLInstanceName
xSQLServerEndpointPermission NodeName InstanceName
xSQLServerEndpointState NodeName InstanceName
xSQLServerFirewall (missing) InstanceName
xSQLServerLogin SQLServer SQLInstanceName
xSQLServerMaxDop SQLServer SQLInstance
xSQLServerMemory SQLServer SQLInstance
xSQLServerNetwork SQLServer InstanceName
xSQLServerPermission NodeName InstanceName
xSQLServerReplication (missing) InstanceName
xSQLServerRole SQLServer SQLInstanceName
xSQLServerRSConfig (missing) InstanceName
xSQLServerRSSecureConnectionLevel (missing) InstanceName
xSQLServerScript (missing) (missing)
xSQLServerSetup (not needed) InstanceName
xWaitforAvailabilityGroup (missing) (missing)

The DSC configuration that is using the resource (as detailed as possible):
n/a

Version of the Operating System, SQL Server and PowerShell the DSC Target Node is running:
n/a

Version of the DSC module you're using, or 'dev' if you're using current dev branch:
Dev

@johlju johlju added the discussion The issue is a discussion. label Jan 12, 2017
@johlju johlju changed the title Host and instance parameter is not consistent throughout the module Discussion: Host and instance parameter is not consistent throughout the module Jan 12, 2017
@luigilink
Copy link
Contributor

luigilink commented Jan 25, 2017

xSQLAliasServer -> I think we can add SQLServer and SQLInstanceName and concatenate these parameters in resource.
For the others we have to choose the same parameter name, but which one 😄
I like SQLServer and SQLInstance or SQLServerName and SQLInstanceName.
Can we also discuss about default value ?

@johlju
Copy link
Member Author

johlju commented Mar 26, 2017

Removed deprecated resources from the list.

@johlju
Copy link
Member Author

johlju commented Apr 10, 2017

@luigilink What do you mean by concatenate the parameters in xSQLServerAlias? xSQLServerAlias does not connect to any SQL Server, it only changes the registry on the host it runs on. It do have a ServerName parameter, but it is what the remote server is that is set in the registry.

I really think we should make SQLServer (the Host) parameter non-mandatory (see issue #319). I think then we should have $env:COMPUTERNAME as default value for SQLServer (the Host) parameter.
InstanceName (the Instance) parameter should always be mandatory, so it can be used for more than one instance on the same server, so it cannot have a default value.

@johlju johlju added the breaking change When used on an issue, the issue has been determined to be a breaking change. label May 1, 2017
@johlju johlju changed the title Discussion: Host and instance parameter is not consistent throughout the module Discussion: BREAKING CHANGE: Host and instance parameter is not consistent throughout the module May 1, 2017
@johlju
Copy link
Member Author

johlju commented Oct 8, 2017

One thought with the instance parameter name SQLInstancename is that it feel strange if I would to install an Analysis Service or Reporting Service. The name InstanceName would be more natural then.

@johlju
Copy link
Member Author

johlju commented Oct 8, 2017

If we do a breaking change in issue #851 then it would be good if we also resolved this one?

@randomnote1
Copy link
Contributor

It would seem reasonable that if we have that large of a breaking change that this would be a relatively small portion of that change.

My vote is for InstanceName.

@johlju
Copy link
Member Author

johlju commented Oct 9, 2017

Any votes for the name of the parameter for the host name?

For the host name parameter, I personally don't think host name parameter name SQLServer is optimal for the same reason I stated above about the SQLInstanceName parameter name.

Options (please add more options if there are any):

  • SQLServer (most used by resource today)
  • SQLServerName
  • NodeName
  • ServerName
  • ComputerName (normally used by PowerShell Cmdlets)
  • HostName

I'm kind of divided between ServerName, ComputerName and HostName. But I personally think ServerName is best suited. But I go with whatever the majority of the community likes.

@randomnote1
Copy link
Contributor

I tend to like HostName because of connecting to Failover Cluster Instances. However, NodeName seems to line up better with the DSC configurations defining nodes. I'm with @johlju on this one, I could go with ServerName, NodeName, or HostName.

I'm wondering if the host name parameter is even needed since the node configuration already knows which host the instance is on. The only exception is when connecting to a Failover Cluster Instance, however, this could potentially be dynamically determined. I'm thinking out loud on this one, so be gentle 😜

@johlju
Copy link
Member Author

johlju commented Oct 10, 2017

I could go with NodeName. I started using that when I created resource when I started out with DSC because it was a term used by DSC. But I tend to like ServerName because I think that is how you think of it from an SQL perspective, I do like HostName also, because it's a term used with DNS.
I can go with either. Regardless which is chosen, any is better than 'SQLServer'. 😄

I think host names is needed when using AG groups, please see this issue #319.

@nabrond
Copy link
Contributor

nabrond commented Oct 15, 2017

+1 for ServerName

@johlju
Copy link
Member Author

johlju commented Nov 8, 2017

@randomnote1 @nabrond + all

Everybody okay with ServerName and InstanceName? If so, I suggest we make this change as soon as PR #902 is merged.

@johlju
Copy link
Member Author

johlju commented Nov 8, 2017

To all: Maybe I should have asked, is anyone not okay with ServerName and InstanceName as the new parameter names to replace the old (see issue description)? If so please tell us why. 😄

@randomnote1
Copy link
Contributor

@johlju, I'm good with the proposed change.

@nabrond
Copy link
Contributor

nabrond commented Nov 8, 2017

sounds good to me @johlju

@johlju johlju added enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community. high priority The issue or PR should be resolved first. It is of less priority than the label 'Blocking Release'. and removed discussion The issue is a discussion. labels Nov 10, 2017
@johlju
Copy link
Member Author

johlju commented Nov 12, 2017

Working on this now. Using PR #902 as base.

@johlju johlju added in progress The issue is being actively worked on by someone. and removed help wanted The issue is up for grabs for anyone in the community. labels Nov 12, 2017
johlju added a commit to johlju/SqlServerDsc that referenced this issue Nov 18, 2017
- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer has been renamed to ServerName
    (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue dsccommunity#308).
johlju added a commit that referenced this issue Dec 1, 2017
- Changes to SqlAG
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlAGDatabase
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlAGListener
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlAGReplica
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlAlwaysOnService
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlDatabase
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes SqlDatabaseDefaultLocation
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlDatabasePermission
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlDatabaseRecoveryModel
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlDatabaseRole
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerConfiguration
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerEndpoint
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerEndpointPermission
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerEndpointState
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerLogin
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerMaxDop
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerMemory
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerNetwork
  - BREAKING CHANGE: Parameters SQLServer has been renamed to ServerName
    ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerOwner
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerPermission
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerRole
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
- Changes to SqlServerServiceAccount
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly ([issue #308](https://github.com/PowerShell/SqlServerDsc/issues/)).
@vors vors removed the high priority The issue or PR should be resolved first. It is of less priority than the label 'Blocking Release'. label Dec 1, 2017
@johlju johlju removed the in progress The issue is being actively worked on by someone. label Dec 9, 2017
@johlju johlju removed their assignment Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants