Skip to content

Add pending reboot reason #873

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Gijsreyn
Copy link
Contributor

@Gijsreyn Gijsreyn commented Jun 8, 2025

PR Summary

Add a reason field property to indicate why there is a pending reboot. This is for #857

PR Context

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

This is a great change that seems obvious now :)

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Jun 11, 2025

Remarks resolved. Sometimes you've to see the obvious first.

@Gijsreyn Gijsreyn requested a review from SteveL-MSFT June 11, 2025 03:48
@@ -14,4 +14,21 @@ Describe 'reboot_pending resource tests' {
$LASTEXITCODE | Should -Be 0
$out.results.result.actualState.rebootPending | Should -Not -BeNullOrEmpty
}

It 'reboot_pending should have a reason' -Skip:(!$IsWindows) {
BeforeAll {
Copy link
Member

Choose a reason for hiding this comment

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

Is using a BeforeAll within an It the recommended way over try..finally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it were a single test, I would say that, based on its purpose, it should be added fully at the top. But we have multiple, that's why it's best practice to scope it for the particular test.

Copy link
Member

Choose a reason for hiding this comment

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

I think you misunderstand my question. Per https://pester.dev/docs/commands/beforeall/, the use of BeforeAll is intended for within a Context or Describe where you would have multiple tests (hence the All). SInce thi s is a single test, it seems better to use the existing pattern of try..finally.

@@ -14,4 +14,21 @@ Describe 'reboot_pending resource tests' {
$LASTEXITCODE | Should -Be 0
$out.results.result.actualState.rebootPending | Should -Not -BeNullOrEmpty
}

It 'reboot_pending should have a reason' -Skip:(!$IsWindows) {
BeforeAll {
Copy link
Member

Choose a reason for hiding this comment

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

I think you misunderstand my question. Per https://pester.dev/docs/commands/beforeall/, the use of BeforeAll is intended for within a Context or Describe where you would have multiple tests (hence the All). SInce thi s is a single test, it seems better to use the existing pattern of try..finally.

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