Skip to content
This repository has been archived by the owner on Feb 11, 2024. It is now read-only.

Uncaught custom exceptions during most operations should warn rather than crash #38

Open
ChanceNCounter opened this issue Jun 24, 2021 · 7 comments
Labels
bug Something isn't working

Comments

@ChanceNCounter
Copy link
Contributor

If, for instance, we get one of the GitHub parsing exceptions during osm sync, the malformation of a single skill entry should not crash osm, nor prevent the rest of the sync from finishing.

@ChanceNCounter ChanceNCounter added the bug Something isn't working label Jun 24, 2021
@JarbasAl
Copy link
Member

that should never happen, lets be sure to log it loud and clear if it happens, we need to fix root cause

@ChanceNCounter
Copy link
Contributor Author

That particular case shouldn't happen anymore, but some of these exceptions exist to break out of routines that would otherwise try to parse a bunch of Nones. It's good that they're raised, it's just bad that raising them crashes osm

@ChanceNCounter
Copy link
Contributor Author

I now see the confusion. Poorly titled, but I can't think of a better one.

I mean, several existing, uncaught cases of the GitHub-related custom exceptions should warn rather than crashing. I didn't include a list because I discovered the problem in passing. It will require a deeper dive to find the wheres.

@NeonDaniel
Copy link
Member

Related to this, I think the get_requirements_json logs should be downgraded from ERROR to WARNING since a skills manifest.yml is not required, and this has a tendency to spam the log when batch installing skills.

@NeonDaniel
Copy link
Member

Is this still valid? I thought everything in a loop got a try/except inside already..

@JarbasAl
Copy link
Member

JarbasAl commented Feb 4, 2022

the whole point of the dedicated exceptions is throwing/catching them when they make sense

if its uncaught, thats the point of adding it in the first place, go fix it!

imho this issue is wontfix unless i misunderstood

@ChanceNCounter
Copy link
Contributor Author

You misunderstood. Either this issue is resolved, or we just haven't had enough opportunities to use osm since the GitHub refactor. One way or another, we had a shitload of uncaught exceptions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants