Skip to content
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

feat(cache): Mention when a download failure is due to insufficient permissions #518

Merged
merged 7 commits into from
Aug 20, 2021

Conversation

relaxolotl
Copy link
Contributor

If we know that the reason why we weren't able to download a file was due to permissions we now record that fact and return it in the symbolication response.

This bases itself off of #512 to avoid conflicts with the download error work that's currently open in PRs.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

lgtm, but please run some tests through sentry to make sure it can deal with these noperm errors. we ran into a couple of situations where we broke sentry because we changed the results that we return, and the validation for these is quite strict.

@relaxolotl
Copy link
Contributor Author

relaxolotl commented Aug 17, 2021

@Swatinem:

please run some tests through sentry to make sure it can deal with these noperm errors.

I quickly ran through some example errors in my local build of sentry to make sure nothing broke, but I believe armenzg very kindly added in integration tests to our pipeline to catch these sorts of errors, i.e. if this https://github.com/getsentry/symbolicator/runs/3349120741 job is passing we are good.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

I don’t think we have an integration test specifically for this. Might be a good idea to add one though. Just configure an obviously bogus s3 source, and see that surfaces the permission error. Though not sure if S3 gives you different errors if you provide a completely bogus bucket, or just if the keys are invalid.

crates/symbolicator/src/services/download/mod.rs Outdated Show resolved Hide resolved
Base automatically changed from feat/all-the-things to master August 19, 2021 14:21
@relaxolotl
Copy link
Contributor Author

also checked this locally to make sure sentry didn't reject the new-ish payload, and i can confirm that it does not (and it even mentions "permission")

Screenshot 2021-08-19 at 8 51 10 AM

@Swatinem
Copy link
Member

nice! so the UI was able to deal with these statuses after all ;-)

@relaxolotl relaxolotl enabled auto-merge (squash) August 20, 2021 13:17
@relaxolotl relaxolotl merged commit efaa4bc into master Aug 20, 2021
@relaxolotl relaxolotl deleted the feat/show-permissions-errors branch August 20, 2021 13:18
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.

3 participants