-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add option to ignore source bundle upload failures #209
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @NINNiT, We should be able to finish the review of this PR on Friday |
lbloder
left a comment
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.
Thank you @NINNiT for your contribution.
Looks good code-wise. Added a few suggestions on how to improve the test so it tests a scenario that is closer to the real world.
src/test/java/io/sentry/integration/uploadSourceBundle/PomUtils.kt
Outdated
Show resolved
Hide resolved
src/test/java/io/sentry/integration/uploadSourceBundle/UploadSourceBundleTestIT.kt
Outdated
Show resolved
Hide resolved
src/test/java/io/sentry/integration/uploadSourceBundle/UploadSourceBundleTestIT.kt
Show resolved
Hide resolved
src/test/java/io/sentry/integration/uploadSourceBundle/UploadSourceBundleTestIT.kt
Show resolved
Hide resolved
|
|
||
| verifier.resetStreams() | ||
| } | ||
|
|
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.
Add a test to confirm that if the flag is set to false the build fails (i.e. old behaviour):
| @Test | |
| fun `fails build when upload fails and ignoreSourceBundleUploadFailure is false`() { | |
| val baseDir = setupProject() | |
| val path = getPOM(baseDir, sentryUrl = "http://unknown") | |
| val verifier = Verifier(path) | |
| verifier.isAutoclean = false | |
| assertThrows(VerificationException::class.java) { | |
| verifier.executeGoal("install") | |
| } | |
| verifier.verifyTextInLog("Could not resolve hostname (Could not resolve host: unknown)") | |
| } | |
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.
assertThrows wasn't available, but
assertFailsWith<VerificationException> {
verifier.executeGoal("install")
}
seems to do just fine.
uses sentryUrl property set to unknown instead of wonky custom sh script to force failure condition
|
@lbloder |
feat: add option to ignore source bundle upload failures
📜 Description
If a self-hosted sentry instance is unreachable, the build fails during the source bundle upload step, blocking the entire build process. This is an initial solution to optionally ignore failures while uploading source maps.
It adds the property
ignoreSourceBundleUploadFailure, which is set to false by default. (same behavior as before, should thus be a non-breaking change):💡 Motivation and Context
We're on a self-hosted sentry instance and it might have downtime at one point or another (upgrades, full backups, ...).
Source bundles are optional metadata for debugging - they shouldn't create a single point of failure for pipelines.
uploadSourceBundle#207💚 How did you test it?
UploadSourceBundleTestIT.ktmvn packageon the app finishes successfully, even though the source-bundle upload itself is failing.📝 Checklist
🔮 Next steps
Feel free to add suggestions / changes! This is just an initial draft, but should already be usable.
Docs would have to be updated
I'm not sure if the test is as clean as it could be, but we somehow have to induce a failure while uploading to the mocked python integration server...