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

Microsoft Defender status retrieval: Add 'not running' status #2448

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

sratz
Copy link
Member

@sratz sratz commented Oct 26, 2024

Fixes #2447

@sratz sratz requested a review from HannesWell October 26, 2024 11:54
Copy link
Contributor

github-actions bot commented Oct 26, 2024

Test Results

 1 821 files  ±0   1 821 suites  ±0   1h 50m 44s ⏱️ + 2m 46s
 7 714 tests ±0   7 486 ✅ ±0  228 💤 ±0  0 ❌ ±0 
24 303 runs  ±0  23 556 ✅ ±0  747 💤 ±0  0 ❌ ±0 

Results for commit 6048bfb. ± Comparison against base commit 4fbbe1b.

♻️ This comment has been updated with latest results.

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks.

I wonder: Maybe this method should be more graceful to unknown statuses, e.g. log them and return false instead of throwing an Exception.

Regarding that question from your issue: I considered that as well, but deliberately decided against it, because an unknown value could also mean the defender is active, e.g. if the defender is changed in a documented way in the future.
Then we would at least notice it even if none monitors the defender doc.
So while this is a bit more cumbersome in the beginning I think it's the saver way on the long run.

@sratz sratz requested a review from HannesWell October 27, 2024 12:47
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the update.

Ideally squash locally and force push as one commit, for context see the discussion at eclipse-pde/eclipse.pde#1455 (comment)

@sratz sratz force-pushed the sratz-defender-notrunning branch from 4b00b26 to 6048bfb Compare October 29, 2024 09:47
@sratz sratz merged commit 6af4673 into master Oct 30, 2024
11 of 13 checks passed
@sratz sratz deleted the sratz-defender-notrunning branch October 30, 2024 14:15
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.

Failed to retrieve Microsoft Defender status: Process terminated with unexpected result: Not running
2 participants