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

feat: add services restart to Set-EsxiCertificateMode #110

Conversation

garlicNova
Copy link
Contributor

@garlicNova garlicNova commented Feb 16, 2024

Summary

Added a restart for specific vCenter Server services to Set-EsxiCertificateMode function.

Type

  • Bugfix
  • Enhancement or Feature
  • Code Style or Formatting
  • Documentation
  • Refactoring
  • Chore
  • Other
    Please describe:

Breaking Changes?

  • Yes, there are breaking changes.
  • No, there are no breaking changes.

Test and Documentation

  • Tests have been completed.
  • Documentation has been added or updated.

Issue References

Closes #33

Additional Information

@garlicNova garlicNova requested a review from a team as a code owner February 16, 2024 20:29
@vmwclabot vmwclabot added the cla-not-required CLA Not Required label Feb 16, 2024
@github-actions github-actions bot added the needs-review Needs Review label Feb 16, 2024
@tenthirtyam tenthirtyam changed the title feat:add-restart-service-to-set-esxicertificatemode feat: add services restart to Set-EsxiCertificateMode Feb 16, 2024
@tenthirtyam tenthirtyam added the enhancement Enhancement label Feb 16, 2024
@tenthirtyam tenthirtyam added this to the v1.5.2 milestone Feb 16, 2024
Copy link
Contributor

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

Please amend with an update to the CHANGELOG.md and a bump to the build in the .psd1.

Example:

## v1.5.2

> Release Date: Unreleased

Enhancement:

- Added a restart for specific vCenter Server services to `Set-EsxiCertificateMode` function. [GH-110](https://github.com/vmware/powershell-module-for-vmware-cloud-foundation-certificate-management/pull/110)
# Version number of this module.
    ModuleVersion = '1.5.2.1000'

@tenthirtyam tenthirtyam force-pushed the feat-add-restart-service-to-Set-EsxiCertificateMode branch from a973c12 to 52bfa81 Compare February 16, 2024 21:09
Copy link
Contributor

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

What about something like this:

Helper function:

function Restart-VcenterService($serviceName) {
    Invoke-RestartService -Service $serviceName
    Start-Sleep -Seconds 30
    $serviceStatus = Invoke-GetService -Service $serviceName
    if ($serviceStatus.state -eq "STARTED") {
        Write-Output "$serviceName restarted successfully."
        return $null
    } else {
        Write-Error "$serviceName failed to restart."
        return $serviceName
    }
}

Then:

$services = @("certificateauthority", "certificatemanagement")
$failedServices = @()

foreach ($service in $services) {
    $failedService = Restart-VcenterService $service
    if ($null -ne $failedService) {
        $failedServices += $failedService
    }
}

if ($failedServices.Count -gt 0) {
    $failedServicesList = [string]::Join(", ", $failedServices)
    Write-Error "The following services failed to start: $failedServicesList. Operation Failed." -ErrorAction Stop
}

But perhaps add / modify the sleep with a counter?

And then add a check to verify that the mode is set as desired.

Copy link
Contributor

@bhumitra bhumitra left a comment

Choose a reason for hiding this comment

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

Just one more suggestion after what Ryan requested is to add an output message. Rest all good.

@github-actions github-actions bot added the documentation Documentation label Feb 17, 2024
@garlicNova
Copy link
Contributor Author

CHANGELOG.md and .psd1 file updated.

add a helper function. the suggested method would result in return value contains both the write-output and the return value so i modify it to return an array instead.

Copy link
Contributor

@bhumitra bhumitra left a comment

Choose a reason for hiding this comment

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

In summary, could you modify the function -
Set-EsxiCertificateMode to use the Restart-VcenterService - the new one you created. also add documentation for Restart-VcenterService function.

@tenthirtyam tenthirtyam marked this pull request as draft February 20, 2024 22:25
@tenthirtyam
Copy link
Contributor

I still have some open comments.

@garlicNova
Copy link
Contributor Author

I still have some open comments.

which ones? I have commented in #110 (comment) to resolve the first 2 comments. I do not see any other comments?

@tenthirtyam tenthirtyam force-pushed the feat-add-restart-service-to-Set-EsxiCertificateMode branch from eb03752 to 35da4b3 Compare February 26, 2024 20:23
bhumitra
bhumitra previously approved these changes Feb 26, 2024
Copy link
Contributor

@bhumitra bhumitra left a comment

Choose a reason for hiding this comment

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

LGTM!

@bhumitra bhumitra marked this pull request as ready for review February 26, 2024 21:55
@tenthirtyam
Copy link
Contributor

Kevin has a couple minor changes to make that we discussed yesterday and we'll delay until those are addressed- mainly around the service status of healthy.

@tenthirtyam tenthirtyam force-pushed the feat-add-restart-service-to-Set-EsxiCertificateMode branch 2 times, most recently from 89b7813 to 8ade530 Compare February 29, 2024 21:05
dded a restart for specific vCenter Server services to `Set-EsxiCertificateMode` function.

Signed-off-by: Kevin Teng <kevin.teng@broadcom.com>
@tenthirtyam tenthirtyam force-pushed the feat-add-restart-service-to-Set-EsxiCertificateMode branch from 8ade530 to 4944d17 Compare February 29, 2024 21:06
Copy link
Contributor

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

LGTM!

VMware.CloudFoundation.CertificateManagement.psm1 Outdated Show resolved Hide resolved
VMware.CloudFoundation.CertificateManagement.psm1 Outdated Show resolved Hide resolved
VMware.CloudFoundation.CertificateManagement.psm1 Outdated Show resolved Hide resolved
VMware.CloudFoundation.CertificateManagement.psm1 Outdated Show resolved Hide resolved
VMware.CloudFoundation.CertificateManagement.psm1 Outdated Show resolved Hide resolved
@tenthirtyam tenthirtyam merged commit 6bba674 into vmware:develop Feb 29, 2024
1 check passed
@tenthirtyam tenthirtyam removed the needs-review Needs Review label Feb 29, 2024
Copy link

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2024
@garlicNova garlicNova deleted the feat-add-restart-service-to-Set-EsxiCertificateMode branch May 20, 2024 20:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-not-required CLA Not Required documentation Documentation enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance Set-EsxiCertificateMode to restart vCenter Server services
4 participants