fix(github): Guard against missing release #567
Conversation
src/targets/github.ts
Outdated
| repo: this.githubConfig.repo, | ||
| }) | ||
| ).data; | ||
| } catch { |
There was a problem hiding this comment.
The blanket catch here may mask other potential issues such as authentication or temporary server unavailability. I feel like we should filter these.
There was a problem hiding this comment.
what would you filter here? To me it sounds reasonable to fall back to assume it is the latest release if something goes wrong here 🤔 But if you want to exclude something specific, we can of course do that, open for concrete suggestions (not really easy to test/try this...)
There was a problem hiding this comment.
We should not assume lack of a release on a 401 or 500 response. I'd even say unless we get a 200 with an explicit null in it, we should not assume anything?
There was a problem hiding this comment.
So you mean we should fail the whole release if we do not get a 200 here?
There was a problem hiding this comment.
Yes as otherwise you create an incomplete release or worse, mark a hotfix release as latest accidentally.
If you think this behavior should be optional, we should revise the flow and make this an explicit user choice or make a breaking release.
There was a problem hiding this comment.
finally got around to give this another go. I adjusted it to only "swallow" 404 errors, which occur when no release exists. all other errors are rethrown!
There was a problem hiding this comment.
See https://github.com/getsentry/craft/pull/567/files#r1899608188
In it's current form this patch creates inconsistent behavior on server related issues.
mydea
force-pushed
the
fn/fix-github-no-releases
branch
from
35eef2a to
ac2964b
Compare
Co-authored-by: Burak Yigit Kaya <[email protected]>
Fixes #503 (comment)
Note: We already have a test covering that
getLatestRelease()accepts undefined.