Skip to content

feat(go worker): implement the enrichers#5310

Open
michaelkedar wants to merge 15 commits intogoogle:masterfrom
michaelkedar:👷🙅🐍

Hidden character warning

The head ref may contain hidden characters: "\ud83d\udc77\ud83d\ude45\ud83d\udc0d"
Open

feat(go worker): implement the enrichers#5310
michaelkedar wants to merge 15 commits intogoogle:masterfrom
michaelkedar:👷🙅🐍

Conversation

@michaelkedar
Copy link
Copy Markdown
Member

@michaelkedar michaelkedar commented May 7, 2026

Added Enrichers to the pipeline to cover what the python worker is currently doing.
There's quite a bit, but I've tried making individual tests to cover each enricher, and copied one of the Python test yaml files to go to make sure it's similar

@michaelkedar michaelkedar marked this pull request as ready for review May 7, 2026 02:56
@michaelkedar michaelkedar requested review from Ly-Joey, another-rex and tobyhawker and removed request for Ly-Joey May 7, 2026 02:56
another-rex
another-rex previously approved these changes May 8, 2026
Copy link
Copy Markdown
Contributor

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

hmm for each enricher should it have a name, that way we can debug up to where each enrichment run actually got up to and which one errored.

Overall LGTM!

"github.com/ossf/osv-schema/bindings/go/osvschema"
)

type EventType int
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this just be a string type rather than an int type, and we can skip the switch statement in affected_versions.go

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I want this to be an int type to use for sorting, but I've added a String() method to the type.

Comment thread go/internal/worker/engine.go Outdated

// Get the current state of the vuln to check against
current, err := e.Stores.Vulnerability.Get(ctx, task.Vuln.GetId())
currentNotFound := errors.Is(err, models.ErrNotFound)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you invert this bool? The double negation is a bit confusing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

redid the logic here to avoid this variable altogether


var parsedEvents []parsedEvent
for _, evt := range events {
p, err := sys.Parse(evt.Version)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm do we have a linter or some sort of warning where we can get notified about these invalid versions? E.g. maybe this is showing a edge case in our version parsing where we are wrong.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've made the SortVersions and this log an error and skip. Hopefully we don't have bad versions OSV.dev (though IIRC, most semantic parsers cannot actually error.

@michaelkedar
Copy link
Copy Markdown
Member Author

hmm for each enricher should it have a name, that way we can debug up to where each enrichment run actually got up to and which one errored.

I actually use the %T formatting here which should log e.g. *namenormalize.Enricher

I can still add a Name() method if you'd prefer

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