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

Comments

Projects
None yet
5 participants
@johlju
Contributor

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 label Jan 12, 2017

@johlju johlju changed the title from Host and instance parameter is not consistent throughout the module to Discussion: Host and instance parameter is not consistent throughout the module Jan 12, 2017

@luigilink

This comment has been minimized.

Show comment
Hide comment
@luigilink

luigilink Jan 25, 2017

Contributor

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 ?

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

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Mar 26, 2017

Contributor

Removed deprecated resources from the list.

Contributor

johlju commented Mar 26, 2017

Removed deprecated resources from the list.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Apr 10, 2017

Contributor

@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.

Contributor

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 changed the title from Discussion: Host and instance parameter is not consistent throughout the module to Discussion: BREAKING CHANGE: Host and instance parameter is not consistent throughout the module May 1, 2017

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 8, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 8, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Oct 8, 2017

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.

Contributor

randomnote1 commented Oct 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 9, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Oct 9, 2017

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 😜

Contributor

randomnote1 commented Oct 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 10, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@nabrond

nabrond Oct 15, 2017

Contributor

+1 for ServerName

Contributor

nabrond commented Oct 15, 2017

+1 for ServerName

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 8, 2017

Contributor

@randomnote1 @nabrond + all

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 8, 2017

Contributor

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. 😄

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Nov 8, 2017

Contributor

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

Contributor

randomnote1 commented Nov 8, 2017

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

@nabrond

This comment has been minimized.

Show comment
Hide comment
@nabrond

nabrond Nov 8, 2017

Contributor

sounds good to me @johlju

Contributor

nabrond commented Nov 8, 2017

sounds good to me @johlju

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 12, 2017

Contributor

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

Contributor

johlju commented Nov 12, 2017

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

@johlju johlju added in progress and removed help wanted labels Nov 12, 2017

johlju added a commit to johlju/SqlServerDsc that referenced this issue Nov 18, 2017

Changes to SqlAG
- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc 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 PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerLogin
- BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerPermission
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerEndpoint
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlAGDatabases
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlAGReplica
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerOwner
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlAGListener
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlAlwaysOnService
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlDatabase
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlDatabasePermission
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlDatabaseRecoveryModel
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlDatabaseRole
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerConfiguration
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerEndpointPermission
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerEndpointState
  - BREAKING CHANGE: Parameter NodeName has been renamed to ServerName
    (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerMaxDop
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerMemory
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerNetwork
  - BREAKING CHANGE: Parameters SQLServer has been renamed to ServerName
    (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerRole
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

johlju added a commit to johlju/SqlServerDsc that referenced this issue Dec 1, 2017

Changes to SqlServerServiceAccount
  - BREAKING CHANGE: Parameters SQLServer and SQLInstanceName has been renamed
    to ServerName and InstanceName respectivly (issue PowerShell#308).

@johlju johlju closed this in #917 Dec 1, 2017

johlju added a commit that referenced this issue Dec 1, 2017

SqlServerDsc: Fix inconsistent parameter names in all resources (#917)
- 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 label Dec 1, 2017

@johlju johlju removed the in progress label Dec 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment