Skip to content

Conversation

@petepuu
Copy link
Contributor

@petepuu petepuu commented Mar 22, 2022

Pull Request (PR) description

Added optional ProvisionDefaultTopology (boolean) parameter to control should the SPSearchServiceApp provision deafult search topology. Use 'ProvisionDefaultTopology = $true' parameter to provision default search topology. When this parameter is defined and value is TRUE then topology is created as below:

  1. First we check are there any servers having Search or ApplicationWithSearch role. If there are then all search components are
    provisioned to all these servers

  2. If no Search or ApplicationWithSearch role servers exist then we check are there servers having Custom role. If yes then all
    search server components are provisioned to one (1) server having Custom role

  3. If no servers exist having roles defined in 1 and 2 then we check is this SingleServer or SingleServerFarm deployment and if yes the all search server components are provisioned to that single server

If you do not want to provision default topology then you need to define search topology using the SPSearchTopology resource!

This Pull Request (PR) fixes the following issues

None

Task list

  • Added an entry to the change log under the Unreleased section of the file CHANGELOG.md.
    Entry should say what was changed and how that affects users (if applicable), and
    reference the issue being resolved (if applicable).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof and comment-based
    help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

Pete added 2 commits March 21, 2022 09:01
ProvisionDefaultTopology when no server able to host
Search Service found
@petepuu petepuu requested a review from ykuijs as a code owner March 22, 2022 11:55
Copy link
Member

@ykuijs ykuijs left a comment

Choose a reason for hiding this comment

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

Great work! Just a few comments.

Also, I do not see an updated unit test for this resource, which means the updated code is not tested. Please also update the unit test (tests/Unit/SharePointDsc/SharePointDsc.SPSearchServiceApp.Tests.ps1), so your code is tested.

Reviewed 2 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @petepuu and @ykuijs)


SharePointDsc/DSCResources/MSFT_SPSearchServiceApp/MSFT_SPSearchServiceApp.psm1, line 483 at r2 (raw file):

                if ($params.ContainsKey("ProvisionDefaultTopology") -eq $true)
                {
                    if ($params.ProvisionDefaultTopology -eq $true)

You can combine these two lines into a single statement:
if (($params.ContainsKey("ProvisionDefaultTopology") -eq $true) -and ($params.ProvisionDefaultTopology -eq $true))
{
.....
}

If the first condition fails, it does not evaluate the second part. That way it will never throw an error.

Code quote:

                if ($params.ContainsKey("ProvisionDefaultTopology") -eq $true)
                {
                    if ($params.ProvisionDefaultTopology -eq $true)

SharePointDsc/DSCResources/MSFT_SPSearchServiceApp/MSFT_SPSearchServiceApp.psm1, line 509 at r2 (raw file):

                            if ($searchServers.Count -eq 0)
                            {
                                Write-Verbose -Message "Provisioning default search topology"

Why is this Verbose logging specified here? The message doesn't make much sense. I would update to something like "No servers found with Search or ApplicationWithSearch roles, checking Custom role"


SharePointDsc/DSCResources/MSFT_SPSearchServiceApp/MSFT_SPSearchServiceApp.psm1, line 515 at r2 (raw file):

                                if ($searchServers.Count -eq 0)
                                {
                                    $searchServers = $possibleSearchServers | Where-Object { $_.Role -in ("SingleServerFarm", "SingleServer") }

For troubleshooting purposes: Add Verbose message like "No servers found with Custom role. Server role must be SingleServer or SingleServerFarm"

Copy link
Member

@ykuijs ykuijs left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 4 files reviewed, 5 unresolved discussions (waiting on @petepuu and @ykuijs)


SharePointDsc/DSCResources/MSFT_SPSearchServiceApp/MSFT_SPSearchServiceApp.psm1, line 362 at r2 (raw file):

        [Parameter()]
        [System.Boolean]
        $ProvisionDefaultTopology

We should set the default value of this parameter to False, so:
$ProvisionDefaultTopology = $false

Do this for the Get/Test/Set methods


SharePointDsc/DSCResources/MSFT_SPSearchServiceApp/MSFT_SPSearchServiceApp.psm1, line 366 at r2 (raw file):

    Write-Verbose -Message "Setting Search service application '$Name'"

To make sure the Default value is also added to the PSBounParameters collection, add this line here:
$PSBoundParameters.ProvisionDefaultTopology = $ProvisionDefaultTopology

Do this for the Get/Test/Set methods

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants