MODULES-9347 pdk convert or pdk update on the module #229
Conversation
| @@ -1,6 +1,6 @@ | |||
| def has_app_pool(pool_name) | |||
| def has_app_pool(_pool_name) | |||
There was a problem hiding this comment.
Strings in the spec directory do not need decoration
There was a problem hiding this comment.
I think this looks less like a decorator and maybe more like Rubocop complaining that a declared variable never gets used. On this point perhaps Rubocop has a point that this is confusing in this situation that the function takes a parameter, but then uses a global variable instead. This whole file needs to be re-evaluated and refactored to respect the params passed in I think and not use the globals. This test suite's use of globals has been a source of annoyance for some time, but maybe a separate ticket?
| inst_cmd << %{Set-WebConfigurationProperty -Filter "$($webApplication.ItemXPath)/virtualDirectory[@path='/']" -Name physicalPath -Value '#{@resource[:physicalpath]}' -ErrorAction Stop} | ||
| # XXX Under what conditions would we have other paths? | ||
| inst_cmd << %{Set-WebConfigurationProperty -Filter "$($webApplication.ItemXPath)/virtualDirectory[@path='/']" -Name physicalPath -Value '\ | ||
| #{@resource[:physicalpath]}' -ErrorAction Stop} |
There was a problem hiding this comment.
Please close the string at the end of the previous line and start a new one at the beginning here as per the good example in the Rubocop docs. If the string is not concatenated that way the result in the module's debug output will be a value passed to the -Value parameter that separated from the parameter name by 8 spaces for no discernable reason. Here we can see the reason is that the white space at the beginning of the line is added to the string because the string is not terminated, but end users won't see that. They will just see a very weird looking PowerShell call. The PowerShell parser will deal with this without issue, but it's the end users that will be confused if they do a puppet debug run, which is something support often requests of users when they have issues.
Since there are a number of instances of this issue, further instances will be commented simply saying "Concat Style".
| end | ||
|
|
||
| if @property_flush[:sslflags] | ||
| flags = @property_flush[:sslflags].join(',') | ||
| inst_cmd << "Set-WebConfigurationProperty -Location '#{self.class.find_sitename(resource)}/#{app_name}' -Filter 'system.webserver/security/access' -Name 'sslFlags' -Value '#{flags}' -ErrorAction Stop" | ||
| inst_cmd << "Set-WebConfigurationProperty -Location '#{self.class.find_sitename(resource)}/#{app_name}'\ | ||
| -Filter 'system.webserver/security/access' -Name 'sslFlags' -Value '#{flags}' -ErrorAction Stop" |
| inst_cmd << "Set-WebConfigurationProperty -Location '#{self.class.find_sitename(resource)}/#{app_name}' -Filter 'system.webserver/security/authentication/#{auth}Authentication' -Name enabled -Value #{@property_flush[:authenticationinfo][auth]} -ErrorAction Stop" | ||
| @property_flush[:authenticationinfo].each do |auth, _enable| | ||
| inst_cmd << "Set-WebConfigurationProperty -Location '#{self.class.find_sitename(resource)}/#{app_name}'\ | ||
| -Filter 'system.webserver/security/authentication/#{auth}Authentication' -Name enabled -Value #{@property_flush[:authenticationinfo][auth]} -ErrorAction Stop" |
| end | ||
| end | ||
|
|
||
| if @property_flush[:enabledprotocols] | ||
| inst_cmd << "Set-WebConfigurationProperty -Filter 'system.applicationHost/sites/site[@name=\"#{self.class.find_sitename(resource)}\"]/application[@path=\"/#{app_name}\"]' -Name enabledProtocols -Value '#{@property_flush[:enabledprotocols]}'" | ||
| inst_cmd << "Set-WebConfigurationProperty -Filter 'system.applicationHost/sites/site[@name=\"\ | ||
| #{self.class.find_sitename(resource)}\"]/application[@path=\"/#{app_name}\"]' -Name enabledProtocols -Value '#{@property_flush[:enabledprotocols]}'" |
There was a problem hiding this comment.
This instance is a little bit different. The string continuation is right in the middle of an XPath query. Since this is XML we're dealing with, any extra characters, including whitespace inside of a quoted string could be an issue.
| cmd << '-ErrorAction Stop;' | ||
| if @resource[:user_name] | ||
| cmd << "Set-ItemProperty -Path 'IIS:\\Sites\\#{@resource[:sitename]}\\#{@resource[:name]}'\ | ||
| -Name 'userName' -Value '#{@resource[:user_name]}' -ErrorAction Stop;" |
| end | ||
| if @resource[:password] | ||
| cmd << "Set-ItemProperty -Path 'IIS:\\Sites\\#{@resource[:sitename]}\\#{@resource[:name]}'\ | ||
| -Name 'password' -Value '#{escape_string(@resource[:password])}' -ErrorAction Stop;" |
| @@ -1,6 +1,6 @@ | |||
| def has_app_pool(pool_name) | |||
| def has_app_pool(_pool_name) | |||
There was a problem hiding this comment.
I think this looks less like a decorator and maybe more like Rubocop complaining that a declared variable never gets used. On this point perhaps Rubocop has a point that this is confusing in this situation that the function takes a parameter, but then uses a global variable instead. This whole file needs to be re-evaluated and refactored to respect the params passed in I think and not use the globals. This test suite's use of globals has been a source of annoyance for some time, but maybe a separate ticket?
|
Passed Adhoc. |
pdk version: 1.11.1