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

xSQLServer: Resource Naming Conventions #851

Closed
randomnote1 opened this Issue Sep 28, 2017 · 35 comments

Comments

Projects
None yet
6 participants
@randomnote1
Contributor

randomnote1 commented Sep 28, 2017

A common theme I've been reading in the issues is that the names of the resources in the xSQLServer module are too long. Even if they worked with Azure Automation, they are considered to be too long. With that said, some of the names are extremely long due to the naming conventions that have been adopted (eg. xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership).

The basic format of the naming convention appears to be:
<Module Identifier>[<Component>][<Action>]<Scope>{<Feature> | <Property>}

If we could publish guidance on what our naming convention is for each item, I think it could result in shorter name lengths. This published guidance would be part of the contributing guidelines. For example:

  • Module Identifier
    • Sql - I think xSQLServer for the module identifier is redundant since the module is named xSQLServer. This is also seven characters shorter. A good reference for this is the SharePointDSC module. They use "SP" as the prefix for all their resources.
  • Component
    • - Database Engine has no component abbreviation
    • AS - Analysis Services
    • IS - Integration Services
    • RS - Reporting Services
  • Action (not required)
    • Setup
    • WaitFor
  • Scope - Where the action, feature, or property is being applied. (List is incomplete)
    • Server
    • Database
    • AG (AvailabilityGroup)
  • Feature (List is incomplete)
    • AlwaysOn - This is for the overall AlwaysOn feature
  • Feature Property (can be omitted)
    • Alias
    • Configuration
    • DatabaseMembership
    • RecoveryModel
    • Service

Here's a table with the current resource names and proposed resource names:

Current Name Proposed Name Notes
xSQLServerAlias SqlAlias
xSQLServerAlwaysOnAvailabilityGroup SqlAG
xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership SqlAGDatabase
xSQLServerAlwaysOnAvailabilityGroupReplica SqlAGReplica
xSQLServerAlwaysOnService SqlAlwaysOnService
xSQLServerAvailabilityGroupListener SqlAGListener
xSQLServerConfiguration SqlServerConfiguration Can't think of a better descriptor for this
xSQLServerDatabase SqlDatabase
xSQLServerDatabaseDefaultLocation SqlDatabaseDefaultLocation
xSQLServerDatabaseOwner SqlDatabaseOwner This should be rolled into the SqlDatabase resource
xSQLServerDatabasePermission SqlDatabasePermission
xSQLServerDatabaseRecoveryModel SqlDatabaseRecoveryModel This should be rolled into the SqlDatabase resource
xSQLServerDatabaseRole SqlDatabaseRole
xSQLServerEndpoint SqlServerEndpoint
xSQLServerEndpointPermission SqlServerEndpointPermission
xSQLServerEndpointState SqlServerEndpointState This should be rolled into the SqlServerEndpoint resource
xSQLServerFirewall SqlWindowsFirewall
xSQLServerLogin SqlServerLogin
xSQLServerMaxDop SqlServerMaxDop
xSQLServerMemory SqlServerMemory
xSQLServerNetwork SqlServerNetwork
xSQLServerPermission SqlServerPermission
xSQLServerRole SqlServerRole This naming specifies that this is for a "Server Role" as opposed to a "Database Role"
xSQLServerReplication SqlServerReplication
xSQLServerRSConfig SqlRS
xSQLServerRSSecureConnectionLevel SqlRSSecureConnectionLevel
xSQLServerScript SqlScript What would be the scope for this?
xSQLServerServiceAccount SqlServiceAccount
xSQLServerSetup SqlSetup What would be the scope for this?
xWaitForAvailabilityGroup SqlWaitForAG

I'm looking forward to hearing your thoughts on this.

@johlju johlju changed the title from xSQLServer Resource Naming Conventions to xSQLServer: Resource Naming Conventions Oct 5, 2017

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 8, 2017

Contributor

If someone else has an opinion on this, then please share.

Me personally think @randomnote1's suggestion is all positive. The only minor negative is that 'SQL' does not accurately describe the SQL Server product, but could be any other SQL product, potentially colliding/conflicting with that products name. But most likely both those products are not installed on the same target node, so conflict with two, for example, SQLSetup resource are really low. So, a minor negative 😄

To enforce this guideline on present resources will be a major breaking change.

The specific contribution guidelines in this repository would be a good place for describing this naming convention.

I think we also should consider resolving issue #308 when we do such a breaking change. And add that to the naming convention as well.

Contributor

johlju commented Oct 8, 2017

If someone else has an opinion on this, then please share.

Me personally think @randomnote1's suggestion is all positive. The only minor negative is that 'SQL' does not accurately describe the SQL Server product, but could be any other SQL product, potentially colliding/conflicting with that products name. But most likely both those products are not installed on the same target node, so conflict with two, for example, SQLSetup resource are really low. So, a minor negative 😄

To enforce this guideline on present resources will be a major breaking change.

The specific contribution guidelines in this repository would be a good place for describing this naming convention.

I think we also should consider resolving issue #308 when we do such a breaking change. And add that to the naming convention as well.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 8, 2017

Contributor

@randomnote1 I think the example 'SQLAlwaysOnDatabaseMembership' is wrong? Shouldn't be 'SQLAvailabilityGroupDatabaseMembership'? Maybe it could even be shortened to 'SQLAvailabilityGroupDatabase'?

Would you mind adding a list of current name and new name to your issue description (or new comment)?

Contributor

johlju commented Oct 8, 2017

@randomnote1 I think the example 'SQLAlwaysOnDatabaseMembership' is wrong? Shouldn't be 'SQLAvailabilityGroupDatabaseMembership'? Maybe it could even be shortened to 'SQLAvailabilityGroupDatabase'?

Would you mind adding a list of current name and new name to your issue description (or new comment)?

@randomnote1

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Oct 8, 2017

Contributor

@johlju, you are completely correct! Fixed my example above. I'll work on building out a table translating current resource names to proposed resource names.

I thought about that with "competing" SQL products, however others tend not to have "SQL" in the name except for MySQL. Though, in that example, I would expect the DSC module would start with "MySQL".

Contributor

randomnote1 commented Oct 8, 2017

@johlju, you are completely correct! Fixed my example above. I'll work on building out a table translating current resource names to proposed resource names.

I thought about that with "competing" SQL products, however others tend not to have "SQL" in the name except for MySQL. Though, in that example, I would expect the DSC module would start with "MySQL".

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 9, 2017

Contributor

Thanks for adding the list of resource and also making notes on them! Awesome!

In regards of your suggestion to skip 'x' from the resource names. I did talk with @mgreenegit at MSIgnite and he suggested to skip the 'x' when I mentioned a new resource was in the pipe (xSQLServerDefaultLocation). He also had a thought, emphasis on thought (might not happen in this way), that maybe there could be a new branch which would be a "SqlServerDsc"-branch and published to the gallery.
Essential we will then have three branches, "dev"-, "experimental"- and "supported"-branches (names just picked top of my head). Where "experimental" would still be published as xSQLServer to the gallery for "yet-not-ready-to-be-(Microsoft|Community)-supported" (my word).
If the resource names are equal in all branches (no 'x'), then there wouldn't need to be any conversion of resource names, which is a must I my opinion. And if the user would install both xSQLServer and SqlServerDsc they would be conflicting with each other.
@mgreenegit was to write something up around this subject.

But today, in the the CONTRIBUTING.md in DscResources it says

If you are adding a new resource to an experimental/preview module (module name starts with 'x'), the resource name must also start with 'x' (e.g. MSFT_xResource.psm1 or xResource.psm1).

So for this issue I think we must assume we can't rename the resources to not use 'x' as long as the module is still named 'xSQLServer'.
Discussing this in PR PowerShell/DscResources#317.

Contributor

johlju commented Oct 9, 2017

Thanks for adding the list of resource and also making notes on them! Awesome!

In regards of your suggestion to skip 'x' from the resource names. I did talk with @mgreenegit at MSIgnite and he suggested to skip the 'x' when I mentioned a new resource was in the pipe (xSQLServerDefaultLocation). He also had a thought, emphasis on thought (might not happen in this way), that maybe there could be a new branch which would be a "SqlServerDsc"-branch and published to the gallery.
Essential we will then have three branches, "dev"-, "experimental"- and "supported"-branches (names just picked top of my head). Where "experimental" would still be published as xSQLServer to the gallery for "yet-not-ready-to-be-(Microsoft|Community)-supported" (my word).
If the resource names are equal in all branches (no 'x'), then there wouldn't need to be any conversion of resource names, which is a must I my opinion. And if the user would install both xSQLServer and SqlServerDsc they would be conflicting with each other.
@mgreenegit was to write something up around this subject.

But today, in the the CONTRIBUTING.md in DscResources it says

If you are adding a new resource to an experimental/preview module (module name starts with 'x'), the resource name must also start with 'x' (e.g. MSFT_xResource.psm1 or xResource.psm1).

So for this issue I think we must assume we can't rename the resources to not use 'x' as long as the module is still named 'xSQLServer'.
Discussing this in PR PowerShell/DscResources#317.

@randomnote1

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Oct 9, 2017

Contributor

This is good stuff! I read through the PR for updating the naming guidance. I think dropping the "x" on the resource name is an excellent idea so we can seamlessly move resources from dev to experimental and then eventually supported.

I think the guidance for working around the conflicts of using same resource names in all of the branches would look something like this:

Import-DscResource -ModuleName xSQLServer SQLDefaultLocation -Version 8.3.0.0
Import-DscResource -ModuleName SQLServerDSC SQLMemory -Version 1.0.0.0

Once the conversation in the contributing guidance is completed, then we should be able to finalize the decision here.

Contributor

randomnote1 commented Oct 9, 2017

This is good stuff! I read through the PR for updating the naming guidance. I think dropping the "x" on the resource name is an excellent idea so we can seamlessly move resources from dev to experimental and then eventually supported.

I think the guidance for working around the conflicts of using same resource names in all of the branches would look something like this:

Import-DscResource -ModuleName xSQLServer SQLDefaultLocation -Version 8.3.0.0
Import-DscResource -ModuleName SQLServerDSC SQLMemory -Version 1.0.0.0

Once the conversation in the contributing guidance is completed, then we should be able to finalize the decision here.

@nabrond

This comment has been minimized.

Show comment
Hide comment
@nabrond

nabrond Oct 15, 2017

Contributor

Good stuff here @randomnote1! Couple of notes/comments:

  1. For readability, I suggest using Sql over SQL. I forget where I picked this up, but the guidance was any acronym over two characters should only have the first character capitalized.
  2. The usage of the "Feature" block seems to be inconsistent. For example SqlDatabasePermission vs SqlPermission. Shouldn't the latter be SqlServerPermission since it targets the server level permission assignment? Perhaps the scheme should be defined as: <Module Identifier>[<Action>]<Scope>{<Feature> | <Property>}
    The values for <Scope> could be, but not limited to:
    • Server
    • Database
    • AG (AvailabilityGroup)
Contributor

nabrond commented Oct 15, 2017

Good stuff here @randomnote1! Couple of notes/comments:

  1. For readability, I suggest using Sql over SQL. I forget where I picked this up, but the guidance was any acronym over two characters should only have the first character capitalized.
  2. The usage of the "Feature" block seems to be inconsistent. For example SqlDatabasePermission vs SqlPermission. Shouldn't the latter be SqlServerPermission since it targets the server level permission assignment? Perhaps the scheme should be defined as: <Module Identifier>[<Action>]<Scope>{<Feature> | <Property>}
    The values for <Scope> could be, but not limited to:
    • Server
    • Database
    • AG (AvailabilityGroup)
@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 15, 2017

Contributor

Agree with @nabrond on that 'Sql' should be used.

Scope is an interesting idea. For example setting permission for Analysis Services, how would that resource be named?

  1. AnalysisServicesPermission
  2. ASPermission
  3. SqlAnalysisServicesPermission
  4. SqlAnalysisPermission
  5. SqlASPermission

The last would align with the proposed name for 'xSQLServerRSConfig'; 'SqlRS'.

Contributor

johlju commented Oct 15, 2017

Agree with @nabrond on that 'Sql' should be used.

Scope is an interesting idea. For example setting permission for Analysis Services, how would that resource be named?

  1. AnalysisServicesPermission
  2. ASPermission
  3. SqlAnalysisServicesPermission
  4. SqlAnalysisPermission
  5. SqlASPermission

The last would align with the proposed name for 'xSQLServerRSConfig'; 'SqlRS'.

@randomnote1

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Oct 16, 2017

Contributor

I like the idea of scope. How would we handle items that are outside of the SQL instance? I'm think of things like the AlwaysOn service, aliases, network configuration, etc. Would they fall under the server scope?

Instead of Server, how about Instance? This would look like SqlInstancePermission. Though I suppose if we're matching how Management Studio labels everything, it still wold be Server.

For Analysis Services (and other components), I am leaning towards using the acronym with the module identifier (SqlASPermission). I think spelling out the full name (SqlAnalysisServicesPermission) puts us right back where we started with really long names.

I updated the table with the scope concept. There are several items that need refined.

Contributor

randomnote1 commented Oct 16, 2017

I like the idea of scope. How would we handle items that are outside of the SQL instance? I'm think of things like the AlwaysOn service, aliases, network configuration, etc. Would they fall under the server scope?

Instead of Server, how about Instance? This would look like SqlInstancePermission. Though I suppose if we're matching how Management Studio labels everything, it still wold be Server.

For Analysis Services (and other components), I am leaning towards using the acronym with the module identifier (SqlASPermission). I think spelling out the full name (SqlAnalysisServicesPermission) puts us right back where we started with really long names.

I updated the table with the scope concept. There are several items that need refined.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 16, 2017

Contributor

I think they need to fall under server scope.

I do like 'Instance', but I think it would be confusing.

Yes I think 'SqlASPermission' is the best option. We should not use long names again. :)

Contributor

johlju commented Oct 16, 2017

I think they need to fall under server scope.

I do like 'Instance', but I think it would be confusing.

Yes I think 'SqlASPermission' is the best option. We should not use long names again. :)

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Oct 16, 2017

Contributor

In issue description there is "Feature Property" in the list. Maybe that should be renamed to "Scope property" instead?

I thought of yet another idea. It builds on the discussion but changes the formula a bit.
Personally I think it generates non-PowerShelly names since it is not using PascalCase. Meaning, I not fond about the idea 😄

<Module Identifier>[<Component abbrev. | Action>]<Scope | Feature abbrev.>{<Scope | Property>,...}
  • Module Identifier
    • Sql
  • Action (not required)
    • Setup
    • WaitFor
    • ...
  • Component abbreviation
    • DBE
    • AS
    • RS
    • IS
    • ...
  • Scope - Where the action, feature, or property is being applied. (List is incomplete)
    • Server
    • Database
    • ...
  • Feature abbreviation
    • AG (AvailabilityGroup)
    • AlwaysOn - This is for the overall AlwaysOn feature
    • ...
  • Scope Property (can be omitted)
    • Alias
    • Configuration
    • State
    • RecoveryModel
    • Service
    • Role
    • ...

That would make resource name like this.

SqlSetup
SqlWaitForAG
SqlDBEConfiguration
SqlDBEAG
SqlDBEDatabasePermission
SqlDBEAGDatabases
SqlDBEEndpoint
SqlASPermission
SqlRS
SqlRSSecureConnectionLevel
SqlISPermission

Contributor

johlju commented Oct 16, 2017

In issue description there is "Feature Property" in the list. Maybe that should be renamed to "Scope property" instead?

I thought of yet another idea. It builds on the discussion but changes the formula a bit.
Personally I think it generates non-PowerShelly names since it is not using PascalCase. Meaning, I not fond about the idea 😄

<Module Identifier>[<Component abbrev. | Action>]<Scope | Feature abbrev.>{<Scope | Property>,...}
  • Module Identifier
    • Sql
  • Action (not required)
    • Setup
    • WaitFor
    • ...
  • Component abbreviation
    • DBE
    • AS
    • RS
    • IS
    • ...
  • Scope - Where the action, feature, or property is being applied. (List is incomplete)
    • Server
    • Database
    • ...
  • Feature abbreviation
    • AG (AvailabilityGroup)
    • AlwaysOn - This is for the overall AlwaysOn feature
    • ...
  • Scope Property (can be omitted)
    • Alias
    • Configuration
    • State
    • RecoveryModel
    • Service
    • Role
    • ...

That would make resource name like this.

SqlSetup
SqlWaitForAG
SqlDBEConfiguration
SqlDBEAG
SqlDBEDatabasePermission
SqlDBEAGDatabases
SqlDBEEndpoint
SqlASPermission
SqlRS
SqlRSSecureConnectionLevel
SqlISPermission

@mgreenegit

This comment has been minimized.

Show comment
Hide comment
@mgreenegit

mgreenegit Nov 2, 2017

Member

I would like to submit a PR asap to resolve Issue 713. The root cause of that issue is actually the length of the AlwaysOnAvailabilityGroupDatabaseMembership resource. If I am going to request a breaking change, I would like to minimize churn and risk of a rename happening twice.

I will get a PR rolling with the naming conventions from the top of this page. If there are any objections, please feel welcome to make comments in the PR and I can update accordingly.

cc @kwirkykat

Member

mgreenegit commented Nov 2, 2017

I would like to submit a PR asap to resolve Issue 713. The root cause of that issue is actually the length of the AlwaysOnAvailabilityGroupDatabaseMembership resource. If I am going to request a breaking change, I would like to minimize churn and risk of a rename happening twice.

I will get a PR rolling with the naming conventions from the top of this page. If there are any objections, please feel welcome to make comments in the PR and I can update accordingly.

cc @kwirkykat

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 2, 2017

Contributor

@mgreenegit go ahead with the PR.

@randomnote1 (and others) my last comment with the idea of using the component abbreviation in the naming convention, what is your thought? Should we discard the idea?
I’m all for going with the convention (and names) from the issue description. Just want to make sure that there are no more ideas or opinions before @mgreenegit’s PR is merged and new version is released.

Contributor

johlju commented Nov 2, 2017

@mgreenegit go ahead with the PR.

@randomnote1 (and others) my last comment with the idea of using the component abbreviation in the naming convention, what is your thought? Should we discard the idea?
I’m all for going with the convention (and names) from the issue description. Just want to make sure that there are no more ideas or opinions before @mgreenegit’s PR is merged and new version is released.

@randomnote1

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Nov 2, 2017

Contributor

@johlju, I'm struggling with the same thing about the pascal case of the component suggestion.

Can we drop DBE (database engine) from the list? I think it's safe to assume that resources without a component name are database engine.

The other option is to change DBE to Dbe, thus making it work with pascal case. I think SqlDbeAGDatabases definitely looks better.

@nabrond, can you weigh in?

Contributor

randomnote1 commented Nov 2, 2017

@johlju, I'm struggling with the same thing about the pascal case of the component suggestion.

Can we drop DBE (database engine) from the list? I think it's safe to assume that resources without a component name are database engine.

The other option is to change DBE to Dbe, thus making it work with pascal case. I think SqlDbeAGDatabases definitely looks better.

@nabrond, can you weigh in?

@randomnote1

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Nov 2, 2017

Contributor

@mgreenegit, @johlju
What is the max length for the resource name? We should probably add that to our unit tests.

Contributor

randomnote1 commented Nov 2, 2017

@mgreenegit, @johlju
What is the max length for the resource name? We should probably add that to our unit tests.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 2, 2017

Contributor

I’m okay with dropping the ‘DBE’ from the list, I agree that resources without an” component abbreviation would be assumed to belong to Database Engine.
I’m also okay writing it as ‘Dbe’ if we would keep it. Agree that it looks better. But I think dropping it from he list is best.

If we are going with component abbreviation should we write Analysis Services with the abbreviation ‘As’ (pascal case)? making a resource name look like ‘SqlAsPermisson’, and for Reporting Services use ‘Rs’, making a resource band look like ‘SqlRs’
I don’t mind using these as ‘AS’ and ‘RS’ either.

@randomnote1 in the end I’m good either way, so I suggest you decide, and I will agree with whatever combination you choose of these options. Because I don’t think we can get it absolutely perfect 😄

If we get more opinions or ideas we take it up for discussion again.

Contributor

johlju commented Nov 2, 2017

I’m okay with dropping the ‘DBE’ from the list, I agree that resources without an” component abbreviation would be assumed to belong to Database Engine.
I’m also okay writing it as ‘Dbe’ if we would keep it. Agree that it looks better. But I think dropping it from he list is best.

If we are going with component abbreviation should we write Analysis Services with the abbreviation ‘As’ (pascal case)? making a resource name look like ‘SqlAsPermisson’, and for Reporting Services use ‘Rs’, making a resource band look like ‘SqlRs’
I don’t mind using these as ‘AS’ and ‘RS’ either.

@randomnote1 in the end I’m good either way, so I suggest you decide, and I will agree with whatever combination you choose of these options. Because I don’t think we can get it absolutely perfect 😄

If we get more opinions or ideas we take it up for discussion again.

@nabrond

This comment has been minimized.

Show comment
Hide comment
@nabrond

nabrond Nov 2, 2017

Contributor

I agree with @randomnote1, not a huge fan of DBE, I agree on baking the assumption into the scheme. As for the other components, I vote to use uppercase for them!

Contributor

nabrond commented Nov 2, 2017

I agree with @randomnote1, not a huge fan of DBE, I agree on baking the assumption into the scheme. As for the other components, I vote to use uppercase for them!

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 2, 2017

Contributor

@randomnote1 there is an issue in either DscResources or DscResources.Tests that we need to get a Tests for the hard limit for path’s (not sure it’s needed though). But if there is a hard limit for resource names we absolutely need a common test for that.

Contributor

johlju commented Nov 2, 2017

@randomnote1 there is an issue in either DscResources or DscResources.Tests that we need to get a Tests for the hard limit for path’s (not sure it’s needed though). But if there is a hard limit for resource names we absolutely need a common test for that.

@randomnote1

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Nov 2, 2017

Contributor

@johlju, @nabrond, @mgreenegit

I will update the template in the first comment with the suggested naming scheme. For two character abbreviations, I'll stick with uppercase letters per @nabrond's earlier suggestion.

Contributor

randomnote1 commented Nov 2, 2017

@johlju, @nabrond, @mgreenegit

I will update the template in the first comment with the suggested naming scheme. For two character abbreviations, I'll stick with uppercase letters per @nabrond's earlier suggestion.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 2, 2017

Contributor

Sounds great! Thanks @randomnote!

Contributor

johlju commented Nov 2, 2017

Sounds great! Thanks @randomnote!

@randomnote1

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Nov 2, 2017

Contributor

@johlju, @nabrond, @mgreenegit

The suggestion table at the top is updated. I ran through it pretty quickly, so please let me know if I made any mistakes.

Thanks!

Contributor

randomnote1 commented Nov 2, 2017

@johlju, @nabrond, @mgreenegit

The suggestion table at the top is updated. I ran through it pretty quickly, so please let me know if I made any mistakes.

Thanks!

@mgreenegit

This comment has been minimized.

Show comment
Hide comment
@mgreenegit

mgreenegit Nov 3, 2017

Member

Could I ask someone to please take a look at this so far? Pester is returning the same error for every resource and I'm having trouble figuring out where the error is coming from.
https://github.com/mgreenegit/xSQLServer/tree/mgreenegit-patch-2

This assumes the goal would be to rename the module to SqlServerDSC, and that since it is a breaking change it would become version 9.0.0. I wasn't sure if that was in scope or not.

@randomnote1 for now, 129 chars

Member

mgreenegit commented Nov 3, 2017

Could I ask someone to please take a look at this so far? Pester is returning the same error for every resource and I'm having trouble figuring out where the error is coming from.
https://github.com/mgreenegit/xSQLServer/tree/mgreenegit-patch-2

This assumes the goal would be to rename the module to SqlServerDSC, and that since it is a breaking change it would become version 9.0.0. I wasn't sure if that was in scope or not.

@randomnote1 for now, 129 chars

@nabrond

This comment has been minimized.

Show comment
Hide comment
@nabrond

nabrond Nov 3, 2017

Contributor

@mgreenegit Poked around a little bit with your branch tonight. I found some issues where MSFT_SqlAGListener.Tests.ps1 and MSFT_SqlServerConfiguration.Tests.ps1 were mistakenly trying to load the module prefixed with SqlServerDSC. (Side note, I think with the new naming standard, the resource would be SqlServerDsc (lowercase sc).)

There also seems to be an oddity in SqlAGListener.Tests.ps1\Test-TargetResource (at least on my machine [Pester 4.0.8]). It seems as if the BeforeEach is not firing before the first assertion is evaluated. I get prompted for $InstanceName, $NodeName, $Name, and $AvailabilityGroup. Problem is, if I throw some breakpoints in and step through the tests, I cannot duplicate that issue.

Contributor

nabrond commented Nov 3, 2017

@mgreenegit Poked around a little bit with your branch tonight. I found some issues where MSFT_SqlAGListener.Tests.ps1 and MSFT_SqlServerConfiguration.Tests.ps1 were mistakenly trying to load the module prefixed with SqlServerDSC. (Side note, I think with the new naming standard, the resource would be SqlServerDsc (lowercase sc).)

There also seems to be an oddity in SqlAGListener.Tests.ps1\Test-TargetResource (at least on my machine [Pester 4.0.8]). It seems as if the BeforeEach is not firing before the first assertion is evaluated. I get prompted for $InstanceName, $NodeName, $Name, and $AvailabilityGroup. Problem is, if I throw some breakpoints in and step through the tests, I cannot duplicate that issue.

@randomnote1

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Nov 3, 2017

Contributor

Dumb question here. When @mgreenegit's PR is merged and the resource is renamed, will we rename the repository? Or will we create a new repo?

Not particularly sure how this works with such a major change.

Contributor

randomnote1 commented Nov 3, 2017

Dumb question here. When @mgreenegit's PR is merged and the resource is renamed, will we rename the repository? Or will we create a new repo?

Not particularly sure how this works with such a major change.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 3, 2017

Contributor

A lot of things to comment on here. One at the time below...

@mgreenegit Running your code both locally and in AppVeyor. Will see if I can see whats going on.

@mgreenegit renaming the resource to 'SqlServerDsc' was in scope, but we was waiting for the discussion about the naming convention to be concluded and the new guidelines merged i DscResources. 😄
But since the work is already done now we could do that, but before release there are a few (or at least one) bigger change that we would like to address in such major change, and that is naming of parameters (issue #308).

@mgreenegit I think we should name the resource 'SqlServerDsc' (pascal case) to align with all other resource modules.

@mgreenegit 129 chars limit, is that for the full path length? Because it cannot be limit of resource name, then we didn't need to rename the resources. :)

@randomnote1 and @mgreenegit For the common test to work (without reworking one or two common tests in DscResource.Tests), either the repository need to be renamed, or create a new repository, with the same name as the .psd1 file has.

Contributor

johlju commented Nov 3, 2017

A lot of things to comment on here. One at the time below...

@mgreenegit Running your code both locally and in AppVeyor. Will see if I can see whats going on.

@mgreenegit renaming the resource to 'SqlServerDsc' was in scope, but we was waiting for the discussion about the naming convention to be concluded and the new guidelines merged i DscResources. 😄
But since the work is already done now we could do that, but before release there are a few (or at least one) bigger change that we would like to address in such major change, and that is naming of parameters (issue #308).

@mgreenegit I think we should name the resource 'SqlServerDsc' (pascal case) to align with all other resource modules.

@mgreenegit 129 chars limit, is that for the full path length? Because it cannot be limit of resource name, then we didn't need to rename the resources. :)

@randomnote1 and @mgreenegit For the common test to work (without reworking one or two common tests in DscResource.Tests), either the repository need to be renamed, or create a new repository, with the same name as the .psd1 file has.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 3, 2017

Contributor

@nabrond I have seen that issue... Think it had something to do with hash tables being used wrongly (not cloned?) which is caught in PowerShell prompt, but not in debugger because of different scopes. I have fixed it in some resources, but a long time ago, so I don't remember exactly what caused it.

Update 2: I have sent in a PR fixing this.

Update:

This block is the problem in the below code. Before this block $testParameters contains 4 keys (InstanceName, etc). Instead of adding two more keys, this block of code changes the $testParameters to only contain these two keys (Ensure, Permission). But why it do work in the debugger and not in the PowerShell prompt is a mystery. 😄

$testParameters += @{
    Ensure = 'Present'
    Permission = 'CONNECT'
}

Changing the block to this resolves the problem.

$testParameters['Ensure'] = 'Present'
$testParameters['Permission'] = 'CONNECT'

https://github.com/PowerShell/xSQLServer/blob/61862e73f3bc4067e1217f42e04d761bf96a19b7/Tests/Unit/MSFT_xSQLServerEndpointPermission.Tests.ps1#L171-L205

Contributor

johlju commented Nov 3, 2017

@nabrond I have seen that issue... Think it had something to do with hash tables being used wrongly (not cloned?) which is caught in PowerShell prompt, but not in debugger because of different scopes. I have fixed it in some resources, but a long time ago, so I don't remember exactly what caused it.

Update 2: I have sent in a PR fixing this.

Update:

This block is the problem in the below code. Before this block $testParameters contains 4 keys (InstanceName, etc). Instead of adding two more keys, this block of code changes the $testParameters to only contain these two keys (Ensure, Permission). But why it do work in the debugger and not in the PowerShell prompt is a mystery. 😄

$testParameters += @{
    Ensure = 'Present'
    Permission = 'CONNECT'
}

Changing the block to this resolves the problem.

$testParameters['Ensure'] = 'Present'
$testParameters['Permission'] = 'CONNECT'

https://github.com/PowerShell/xSQLServer/blob/61862e73f3bc4067e1217f42e04d761bf96a19b7/Tests/Unit/MSFT_xSQLServerEndpointPermission.Tests.ps1#L171-L205

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 3, 2017

Contributor

@mgreenegit I ran the code in AppVeyor. There are errors but can't see those you were mentioning.

See all errors here:
https://ci.appveyor.com/project/johlju/xsqlserver/build/9.0.1035.0

  • Line 54: These errors are due to repository is not named as the module manifest. Since it does not recognize the repository name it tries to download the module 'SqlServerDsc' as an dependency for integration tests and example configurations.
  • Line 788: This is due to repository name is not the same as module manifest as well.
  • Line 899: The strings file 'MSFT_xSQLServerServiceAccount.strings.ps1' needs to be renamed as well. Still 'x' in this one.
  • Line 1544: This is due to repository name is not the same as module manifest as well (I think, unsure).
  • Line 1558: Lint errors in CONTRIBUTING and README.md.
  • Line 1580: still using 'xSQLServer' in these example files.
  • Line 3151: This is due to that $script:DSCResourceName have not yet renamed to the correct resource name. This line in the unit test MSFT_SqlAGListener.Tests.ps1;
    `$script:DSCResourceName    = 'MSFT_SqlServerDSCAvailabilityGroupListener'`
    
  • Line 4029: Same as previous. Test MSFT_SqlServerConfiguration.Tests.ps1 has the wrong name for $script:DSCResourceName.
  • Line 5292: Will be fixed with Line 899.
  • Line 7146: This is due to repository name is not the same as module manifest as well.
Contributor

johlju commented Nov 3, 2017

@mgreenegit I ran the code in AppVeyor. There are errors but can't see those you were mentioning.

See all errors here:
https://ci.appveyor.com/project/johlju/xsqlserver/build/9.0.1035.0

  • Line 54: These errors are due to repository is not named as the module manifest. Since it does not recognize the repository name it tries to download the module 'SqlServerDsc' as an dependency for integration tests and example configurations.
  • Line 788: This is due to repository name is not the same as module manifest as well.
  • Line 899: The strings file 'MSFT_xSQLServerServiceAccount.strings.ps1' needs to be renamed as well. Still 'x' in this one.
  • Line 1544: This is due to repository name is not the same as module manifest as well (I think, unsure).
  • Line 1558: Lint errors in CONTRIBUTING and README.md.
  • Line 1580: still using 'xSQLServer' in these example files.
  • Line 3151: This is due to that $script:DSCResourceName have not yet renamed to the correct resource name. This line in the unit test MSFT_SqlAGListener.Tests.ps1;
    `$script:DSCResourceName    = 'MSFT_SqlServerDSCAvailabilityGroupListener'`
    
  • Line 4029: Same as previous. Test MSFT_SqlServerConfiguration.Tests.ps1 has the wrong name for $script:DSCResourceName.
  • Line 5292: Will be fixed with Line 899.
  • Line 7146: This is due to repository name is not the same as module manifest as well.
@mgreenegit

This comment has been minimized.

Show comment
Hide comment
@mgreenegit

mgreenegit Nov 3, 2017

Member

This is awesome collaboration. I will make these changes ASAP.

The naming is in line with what I am proposing for the new guidelines (also hoping to get done ASAP). I will present my proposal next week on the community call for feedback and/or consensus.

The 129 char limit is fullname (path) per file. We hope that limit is going to be temporary but can be used as a guide in the short term.

Member

mgreenegit commented Nov 3, 2017

This is awesome collaboration. I will make these changes ASAP.

The naming is in line with what I am proposing for the new guidelines (also hoping to get done ASAP). I will present my proposal next week on the community call for feedback and/or consensus.

The 129 char limit is fullname (path) per file. We hope that limit is going to be temporary but can be used as a guide in the short term.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 7, 2017

Contributor

@mgreenegit I updated the issue PowerShell/DscResource.Tests#188 with a link to the above comment regarding the limit.
Let me know if you need any further help with the PR.

Contributor

johlju commented Nov 7, 2017

@mgreenegit I updated the issue PowerShell/DscResource.Tests#188 with a link to the above comment regarding the limit.
Let me know if you need any further help with the PR.

@mgreenegit

This comment has been minimized.

Show comment
Hide comment
@mgreenegit

mgreenegit Nov 8, 2017

Member

Looks like we have a good build. I will submit a PR for review and discussion. Note I temporarily renamed my fork during testing.
https://ci.appveyor.com/project/mgreenegit/sqlserverdsc

Member

mgreenegit commented Nov 8, 2017

Looks like we have a good build. I will submit a PR for review and discussion. Note I temporarily renamed my fork during testing.
https://ci.appveyor.com/project/mgreenegit/sqlserverdsc

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 8, 2017

Contributor

@mgreenegit @randomnote1 I saw a thing in the above list. SqlAGDatabases is written as plural. Should we change that to SqlAGDatabase so it is singular as the rest? I suggest we do that in PR #902.

Contributor

johlju commented Nov 8, 2017

@mgreenegit @randomnote1 I saw a thing in the above list. SqlAGDatabases is written as plural. Should we change that to SqlAGDatabase so it is singular as the rest? I suggest we do that in PR #902.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 10, 2017

Contributor

@randomnote1 and @nabrond can I please get your opinion of the above comment - renaming 'SqlAGDatabases' to 'SqlAGDatabase' (singular). Did we have a reason for naming it using plural?

Contributor

johlju commented Nov 10, 2017

@randomnote1 and @nabrond can I please get your opinion of the above comment - renaming 'SqlAGDatabases' to 'SqlAGDatabase' (singular). Did we have a reason for naming it using plural?

@randomnote1

This comment has been minimized.

Show comment
Hide comment
@randomnote1

randomnote1 Nov 10, 2017

Contributor

Sorry...I totally forgot to respond to your initial question!

I'm ok with using the singular Database. I'm not sure that I had a good reason for using Databases other than it indicated that multiple databases could be added to the availability group using one resource instance.

Contributor

randomnote1 commented Nov 10, 2017

Sorry...I totally forgot to respond to your initial question!

I'm ok with using the singular Database. I'm not sure that I had a good reason for using Databases other than it indicated that multiple databases could be added to the availability group using one resource instance.

@nabrond

This comment has been minimized.

Show comment
Hide comment
@nabrond

nabrond Nov 10, 2017

Contributor

+1 for singular. Although I understand the point @randomnote1 is making about it potentially managing multiple databases. Maybe we need to take a look at the SqlAG resources as a whole and redesign? That's a discussion for another issue though.

Contributor

nabrond commented Nov 10, 2017

+1 for singular. Although I understand the point @randomnote1 is making about it potentially managing multiple databases. Maybe we need to take a look at the SqlAG resources as a whole and redesign? That's a discussion for another issue though.

@johlju

This comment has been minimized.

Show comment
Hide comment
@johlju

johlju Nov 10, 2017

Contributor

Updated the table above and will add a review comment asking if we can do this change in PR #902.

Contributor

johlju commented Nov 10, 2017

Updated the table above and will add a review comment asking if we can do this change in PR #902.

@kwirkykat kwirkykat self-assigned this Nov 28, 2017

@johlju johlju closed this in #902 Nov 28, 2017

johlju added a commit that referenced this issue Nov 28, 2017

BREAKING CHANGE: SqlServerDsc: Implement all suggested fixes from iss…
…ue 851 (#902)

- BREAKING CHANGE: Significant rename to reduce length of resource names. 
  See issue #851 for a complete table mapping rename changes.
  Impact to all resources.

@vors vors removed the high priority label Nov 28, 2017

@mgreenegit

This comment has been minimized.

Show comment
Hide comment
@mgreenegit

mgreenegit Nov 29, 2017

Member

Thank you everyone for assisting me with this effort.

Member

mgreenegit commented Nov 29, 2017

Thank you everyone for assisting me with this effort.

jpogran added a commit to jpogran/puppetlabs-dsc that referenced this issue Feb 7, 2018

(MODULES-6592) Fix building SqlServerDsc
This commit handles the rename of the  DSC Resource to by moving
the contents of the xSQLServer folder to a folder named SqlServerDSC.

In PR PowerShell/SqlServerDsc#851 the
xSQLServer DSC Resource was renamed from xSQLServer to SqlServerDsc.
This normally wouldn't be a problem, however when they changed the name
they updated the xSQLServer submodule to point to the commit in the
newly renamed SqlServerDsc repo. Since we do a direct git submodule
update, it uses the folder name of the submodule reference (xSQLServer)
and not the name of the target repo (SqlServerDSC), resulting in a
folder called xSQLServer with all the contents of the SqlServerDsc repo.

The simple short term fix is to move in place the folder to the correct
name. The longer term fix is stop using the powershell/dscresources git
repo as a source of truth, which is part of a larger project that
is planned.

This is hardcoded and done in a single line right now because premature
optimization is the root of all evil. While we can predict there will be
more renamed cases
(https://blogs.msdn.microsoft.com/powershell/2017/12/08/dsc-resource-naming-and-support-guidelines/)
there is no telling where things will land until we see how people
migrate their DSC Resources forward. This is also largely a moot point
when we move to the PowerShell parser approach.

jpogran added a commit to jpogran/puppetlabs-dsc that referenced this issue Feb 7, 2018

(MODULES-6592) Fix building SqlServerDsc
This commit handles the rename of the  DSC Resource to by moving
the contents of the xSQLServer folder to a folder named SqlServerDSC.

In PR PowerShell/SqlServerDsc#851 the
xSQLServer DSC Resource was renamed from xSQLServer to SqlServerDsc.
This normally wouldn't be a problem, however when they changed the name
they updated the xSQLServer submodule to point to the commit in the
newly renamed SqlServerDsc repo. Since we do a direct git submodule
update, it uses the folder name of the submodule reference (xSQLServer)
and not the name of the target repo (SqlServerDSC), resulting in a
folder called xSQLServer with all the contents of the SqlServerDsc repo.

The simple short term fix is to move in place the folder to the correct
name. The longer term fix is stop using the powershell/dscresources git
repo as a source of truth, which is part of a larger project that
is planned.

This is hardcoded and done in a single line right now because premature
optimization is the root of all evil. While we can predict there will be
more renamed cases
(https://blogs.msdn.microsoft.com/powershell/2017/12/08/dsc-resource-naming-and-support-guidelines/)
there is no telling where things will land until we see how people
migrate their DSC Resources forward. This is also largely a moot point
when we move to the PowerShell parser approach.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment