feat: add scan failure onFinish topic and message type#97
Merged
Conversation
📝 WalkthroughWalkthroughAdds new scan-failure domain types and a Pulsar topic constant: a detailed ScanFailureReport and related enums/structs, an enriched ScanFailureOnFinishedMessage with on-finish metadata, and a new topic name for finished scan-failure messages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pulsar/common/systemhealth/datastructures.go`:
- Line 7: The import github.com/armosec/armosec-infra/scanfailure is referenced
in datastructures.go but the module is not declared in go.mod; add the
dependency to go.mod (for example by running a go command to add the module or
adding a require line for github.com/armosec/armosec-infra at an appropriate
version) so the package scanfailure can be resolved and the build succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6e86f87-6254-424c-8f6e-a43c88f9b1b5
📒 Files selected for processing (2)
pulsar/common/systemhealth/datastructures.gopulsar/common/topics.go
kooomix
force-pushed
the
feature/scan-failure-ingester
branch
from
9547665 to
ff6a943
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pulsar/common/systemhealth/datastructures.go (1)
7-7:⚠️ Potential issue | 🔴 CriticalBuild still failing: repository access issue for
armosec-infra.The dependency is now declared in
go.mod(line 8:v0.0.479), which addresses the original "no required module" error. However, CI is still failing with a different error:git ls-remote -q origin ... exit status 128. This typically indicates the repository is private and CI lacks credentials, or the specified version tag doesn't exist.Ensure:
- The
armosec-infrarepository grants access to the CI runner (via SSH keys orGOPRIVATE/GONOPROXYsettings).- Version
v0.0.479exists and is tagged in the upstream repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pulsar/common/systemhealth/datastructures.go` at line 7, CI cannot fetch the dependency github.com/armosec/armosec-infra (imported as scanfailure) at version v0.0.479; ensure the CI runner has access to that repository or change the dependency to a public/accessible tag: either grant CI repo access (SSH keys or token), set GOPRIVATE/GONOPROXY appropriately so go can fetch private modules, or update go.mod to a valid existing tag/version for armosec-infra if v0.0.479 doesn't exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 8: The go.mod entry for github.com/armosec/armosec-infra is failing
because the upstream repo is missing; inspect usages in
pulsar/common/systemhealth/datastructures.go to identify which types/functions
are imported from that module, then determine whether the upstream was moved or
renamed (search GitHub/organization, check go.sum commit hashes, or check
project docs/PRs). If the module was moved, update the import path in go.mod and
the imports in datastructures.go to the new module path and run go
get/<module>@<version> and go mod tidy; if it no longer exists or was internal,
replace the dependency by copying the required types into an internal package or
point go.mod to an alternative fork/local replace directive (using replace
github.com/armosec/armosec-infra => ../local/path or a maintained fork URL) and
update imports accordingly so the build succeeds.
---
Duplicate comments:
In `@pulsar/common/systemhealth/datastructures.go`:
- Line 7: CI cannot fetch the dependency github.com/armosec/armosec-infra
(imported as scanfailure) at version v0.0.479; ensure the CI runner has access
to that repository or change the dependency to a public/accessible tag: either
grant CI repo access (SSH keys or token), set GOPRIVATE/GONOPROXY appropriately
so go can fetch private modules, or update go.mod to a valid existing
tag/version for armosec-infra if v0.0.479 doesn't exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1539bd1f-7e7c-4102-8ec7-c080ed09b9b9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modpulsar/common/systemhealth/datastructures.gopulsar/common/topics.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pulsar/common/topics.go
kooomix
force-pushed
the
feature/scan-failure-ingester
branch
from
ff6a943 to
243e99e
Compare
|
Summary:
|
kooomix
force-pushed
the
feature/scan-failure-ingester
branch
from
243e99e to
6962b0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pulsar/common/systemhealth/datastructures.go`:
- Around line 49-58: The FailureCase field on ScanFailureOnFinishedMessage is an
untyped int mirroring armosec-infra; define a local typed enum (e.g., type
ScanFailureCase int) and a set of constants (e.g., const (ScanFailureCaseUnknown
ScanFailureCase = iota; ScanFailureCaseX ...)) to represent the valid cases,
change ScanFailureOnFinishedMessage.FailureCase from int to that new
ScanFailureCase type, and add conversion helpers at integration boundaries
(marshal/unmarshal or explicit ConvertToInfraCase/ConvertFromInfraCase
functions) so external integration still maps to the private repo values without
exposing raw ints in this public module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9d2d3d5-433c-4aa8-a39c-b120ff903ce3
📒 Files selected for processing (2)
pulsar/common/systemhealth/datastructures.gopulsar/common/topics.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pulsar/common/topics.go
|
Summary:
|
Add ScanFailureOnFinishPulsarTopic constant and ScanFailureOnFinishedMessage struct with inlined report fields (avoiding private armosec-infra dependency). Signed-off-by: Eran Madar <eranm@armosec.io> Signed-off-by: kooomix <eranm@armosec.io>
kooomix
force-pushed
the
feature/scan-failure-ingester
branch
from
6962b0e to
ef7f4ec
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pulsar/common/systemhealth/datastructures.go (1)
42-50: Consider documenting the embedding vs explicit-fields pattern choice.The struct uses embedding for
ScanFailureReportwhich differs from the established pattern inSecurityRisksIngesterOnFinishedMessage(which uses explicit fields). Both approaches are valid—embedding is more DRY and automatically inherits new fields fromScanFailureReport. However, embedding means JSON field names are controlled by the embedded struct's tags, which could be a consideration for API contracts.If intentional, this is fine. Just noting the pattern difference for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pulsar/common/systemhealth/datastructures.go` around lines 42 - 50, The struct ScanFailureOnFinishedMessage currently embeds scanfailure.ScanFailureReport which differs from the explicit-field pattern used in SecurityRisksIngesterOnFinishedMessage; update the comment above ScanFailureOnFinishedMessage to document that embedding is intentional, note that JSON field names are inherited from scanfailure.ScanFailureReport (and thus part of the API contract), and mention tradeoffs (DRY vs explicit control) so future maintainers know why embedding was chosen and to be cautious about changing fields in scanfailure.ScanFailureReport.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pulsar/common/systemhealth/datastructures.go`:
- Around line 42-50: The struct ScanFailureOnFinishedMessage currently embeds
scanfailure.ScanFailureReport which differs from the explicit-field pattern used
in SecurityRisksIngesterOnFinishedMessage; update the comment above
ScanFailureOnFinishedMessage to document that embedding is intentional, note
that JSON field names are inherited from scanfailure.ScanFailureReport (and thus
part of the API contract), and mention tradeoffs (DRY vs explicit control) so
future maintainers know why embedding was chosen and to be cautious about
changing fields in scanfailure.ScanFailureReport.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a55069de-1bdc-49b6-b781-4e393adb3dca
📒 Files selected for processing (3)
pulsar/common/scanfailure/types.gopulsar/common/systemhealth/datastructures.gopulsar/common/topics.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pulsar/common/topics.go
|
Summary:
|
bvolovat
approved these changes
Mar 8, 2026
matthyx
approved these changes
Mar 8, 2026
This was referenced Mar 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ScanFailureOnFinishPulsarTopicconstant (scan-failure-finished-v1) for the scan failure onFinish fan-outScanFailureOnFinishedMessagestruct insystemhealthpackage, embeddingscanfailure.ScanFailureReportwith rate-limit metadata fieldsPart of SUB-7106 (Scan Failure Ingester fan-out pipeline).
Dependencies
armosec-infrawithscanfailurepackage (from SUB-7113 B.1)Test plan
go build ./...passes after armosec-infra dependency is pinnedSummary by CodeRabbit