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

SPSearchServiceApp: Added logic and parameter to provision default search topology #1401

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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