Skip to content

Improve build workflow BED-7214 #279

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.

Sign up for GitHub

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
lrfalslev wants to merge 8 commits into
base: v4
Choose a base branch
from
+42 −32
Open

Improve build workflow BED-7214 #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
98f2267
normalize SH pull request templates
lrfalslev Mar 10, 2026
baa16c5
temp change to test SHCE/SHE pipelines
lrfalslev Mar 10, 2026
d31ea2d
update actions, run tests
lrfalslev Mar 11, 2026
dccf8be
Merge branch 'v4' into lfalslev/bed-7214
lrfalslev Mar 11, 2026
9fa7b0b
test pipeline with failing test
lrfalslev Mar 11, 2026
71ee8c4
separate test job
lrfalslev Mar 12, 2026
c0d65dd
workflow failing test
lrfalslev Mar 12, 2026
17a0411
fix failing test
lrfalslev Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 27 additions & 22 deletions .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,40 @@
## Description

<!--- Describe your changes in detail -->
<!-- Describe your changes in detail -->

## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->

<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
This PR addresses: [GitHub issue or Jira ticket number]

## How Has This Been Tested?

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
<!--
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.*
-->

## Screenshots (if appropriate):

## Types of changes
<!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

- [ ] Chore (a change that does not modify the application functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Chore (a change that does not modify the application functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [ ] Documentation updates are needed, and have been made accordingly.
- [ ] I have added and/or updated tests to cover my changes.
- [ ] All new and existing tests passed.
- [ ] My changes include a database migration.
<!-- Please make sure you have completed all following checks. -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [ ] I have met the contributing prerequisites
- Assigned myself to this PR
- Added the appropriate labels
- Associated an issue: https://github.com/SpecterOps/BloodHound/issues/672
- Read the Contributing guide: https://github.com/SpecterOps/BloodHound/wiki/Contributing
- [ ] I have ensured that related documentation is up-to-date
Comment on lines +30 to +35
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Replace the hardcoded issue link with a placeholder.

Line 33 bakes one specific issue into every future PR description, which will create incorrect traceability unless authors remember to remove it. Keep this generic instead, e.g. #<issue-number> or a Jira placeholder.

Suggested change 
 - [ ] I have met the contributing prerequisites
   - Assigned myself to this PR
   - Added the appropriate labels
-  - Associated an issue: https://github.com/SpecterOps/BloodHound/issues/672
+  - Associated an issue: #<issue-number> / <Jira-ticket>
   - Read the Contributing guide: https://github.com/SpecterOps/BloodHound/wiki/Contributing
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- [ ] I have met the contributing prerequisites
- Assigned myself to this PR
- Added the appropriate labels
- Associated an issue: https://github.com/SpecterOps/BloodHound/issues/672
- Read the Contributing guide: https://github.com/SpecterOps/BloodHound/wiki/Contributing
- [ ] I have ensured that related documentation is up-to-date
- [ ] I have met the contributing prerequisites
- Assigned myself to this PR
- Added the appropriate labels
- Associated an issue: #<issue-number> / <Jira-ticket>
- Read the Contributing guide: https://github.com/SpecterOps/BloodHound/wiki/Contributing
- [ ] I have ensured that related documentation is up-to-date
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.github/pull_request_template.md around lines 30 - 35, Replace the hardcoded
issue URL in the PR template with a generic placeholder to avoid baking a
specific issue into every PR; update the checklist item that currently contains
"https://github.com/SpecterOps/BloodHound/issues/672" to use a placeholder such
as "#<issue-number>" or "{issue-id}" so authors can fill in the correct
reference when creating PRs (edit the checklist line in
.github/pull_request_template.md accordingly).

- Open API docs
- Code comments
- [ ] I have followed proper test practices
- Added/updated tests to cover my changes
- All new and existing tests passed
17 changes: 10 additions & 7 deletions .github/workflows/build.yml → .github/workflows/build-and-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
name: Build
name: Build and Test

on: pull_request
on:
pull_request:
branches: [ v4 ]

jobs:
build:
build-and-test:
runs-on: windows-latest

steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v6

- name: Setup .NET
uses: actions/setup-dotnet@v3
uses: actions/setup-dotnet@v5
with:
dotnet-version: |
7.0.x
Expand All @@ -19,7 +22,7 @@ jobs:
run: dotnet restore

- name: Build
run: dotnet build
run: dotnet build --no-restore

- name: Test
run: dotnet test
run: dotnet test --no-build
3 changes: 2 additions & 1 deletion src/CommonLib/Processors/CertAbuseProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ public class CertAbuseProcessor
public delegate Task ComputerStatusDelegate(CSVComputerStatus status);
public event ComputerStatusDelegate ComputerStatusEvent;

public CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor registryAccessor, ISAMServerAccessor samServerAccessor, ILogger log = null) {
//TODO: temp change to test github build pipeline
public CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor registryAccessor, ISAMServerAccessor samServerAccessor, String test, ILogger log = null) {
Comment on lines +27 to +28
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Remove the temporary required constructor parameter.

Line 28 introduces a breaking change to a public constructor, but test is unused in the class. That forces every caller to add a dummy string argument for no functional reason, and any existing 4-argument call that previously supplied ILogger now fails to compile.

🔧 Proposed fix 
-        //TODO: temp change to test github build pipeline
-        public CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor registryAccessor, ISAMServerAccessor samServerAccessor, String test, ILogger log = null) {
+        public CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor registryAccessor, ISAMServerAccessor samServerAccessor, ILogger log = null) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
//TODO: temp change to test github build pipeline
public CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor registryAccessor, ISAMServerAccessor samServerAccessor, String test, ILogger log = null) {
public CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor registryAccessor, ISAMServerAccessor samServerAccessor, ILogger log = null) {
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/CommonLib/Processors/CertAbuseProcessor.cs` around lines 27 - 28, The
public constructor CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor
registryAccessor, ISAMServerAccessor samServerAccessor, String test, ILogger log
= null) introduces an unused required parameter 'test' and breaks existing
callers; remove the temporary 'test' parameter and revert the signature to
CertAbuseProcessor(ILdapUtils utils, IRegistryAccessor registryAccessor,
ISAMServerAccessor samServerAccessor, ILogger log = null), updating any ctor
invocations to match and ensuring any constructor overloads or dependency
injection registrations reference the corrected constructor name/signature.

_utils = utils;
_registryAccessor = registryAccessor;
_samServerAccessor = samServerAccessor;
Expand Down
5 changes: 3 additions & 2 deletions test/unit/CertAbuseProcessorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ public CertAbuseProcessorTest() {
_mockLdapUtils = new Mock<ILdapUtils>();
_mockRegistryAccessor = new Mock<IRegistryAccessor>();
_mockSAMServerAccessor = new Mock<ISAMServerAccessor>();
_certAbuseProcessor = new CertAbuseProcessor(_mockLdapUtils.Object, _mockRegistryAccessor.Object, _mockSAMServerAccessor.Object);
//TODO: temp change to test github build pipeline
_certAbuseProcessor = new CertAbuseProcessor(_mockLdapUtils.Object, _mockRegistryAccessor.Object, _mockSAMServerAccessor.Object, "");

_certAbuseProcessor.ComputerStatusEvent += status => {
_receivedCompStatus = status;
Expand All @@ -53,7 +54,7 @@ public CertAbuseProcessorTest() {
public async Task CertAbuseProcessor_IsUserSpecifiesSanEnabled_ReturnsResult(int editFlags, bool expectedResult) {
const string subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{CAName}\\PolicyModules\\CertificateAuthority_MicrosoftDefault.Policy";
const string subValue = "EditFlags";

_mockRegistryAccessor
.Setup(ra => ra.GetRegistryKeyData(
TargetName,
Expand Down
Loading