-
Notifications
You must be signed in to change notification settings - Fork 8
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
CORGI-552: Fix duplicate purl error due to small Syft / Brew NEVRA differences #560
Conversation
976fa9d
to
e2b888f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, its hard to know how to handle all this.
FWIW
shows that case sensitivity heuristics will probably have to be derived from each package ecosystem ... I agree we should try it out in stage (and cross our fingers).
11cb89b
to
007c82e
Compare
007c82e
to
3842e04
Compare
Re: above, case sensitivity isn't one of the issues I'm worried about. That's a farily simple fix, and only seems to affect Github and PyPI components anyway (based on testing I did when we first saw this issue). The bigger problem is purl creation in general. I know that Github and PyPI purls are always lowercased, and I know that PyPI purls have _ underscores converted to - dashes, but I don't know what I don't know. There might be other special rules that cause two different components to end up with the same purl. Most of the errors in our daily monitoring email are for PyPI components, so I deployed this to stage for testing to see if it would prevent errors for at least some of those. But it actually caused a deadlock in the DB, so I'll need to look more at this next week after I finish other tickets. Putting back into draft for now. |
3e46abf
to
aa04e7f
Compare
I tweaked this slightly and deployed again, then didn't see any more deadlock issues. Those might be a rare problem, or a one-off issue when deploying this change while other tasks were running, or a real bug in the code that's now fixed. I'm going to merge this as-is since it does seem to help reduce the IntegrityErrors we're seeing. Will open follow-up PRs or back this out as needed. |
aa04e7f
to
d7891a9
Compare
@RedHatProductSecurity/corgi-devs This is honestly pretty terrible code, but there's no point in me staring at it any longer. I hope this will fix the errors we're seeing in our monitoring email, but I'm not confident the fix is right.
Maybe we should just deploy this and see if it helps, then clean it up if the errors go away. Or if there are still other edge cases, I can handle those in a follow-up PR.