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

Added catch-all for permission errors. #3202

Closed
wants to merge 1 commit into from

Conversation

mitchell-as
Copy link
Contributor

@mitchell-as mitchell-as commented Mar 28, 2024

TaskDX-2646 We don't report access denied errors to rollbar

We don't want to report these to rollbar.

Note: I chose not to centralize reporting inside the fileutils package because there are dozens of functions in there that would have to be changed, and there are also other files that use os-level functions that may return these types of errors (such as os.OpenFile() used by the projectfile package). I figured the best way to approach this was to have a catch-all at the bottom of the error funnel rather than having callers explicitly check for permission issues (similar to top-level rationalize.ErrNoProject handling) and crafting appropriate error messages.

Further note: based on the generality of this solution, it's impossible to provide a truly helpful message (such as which file/folder had a permission issue) without implementing full-blown user-facing errors and types/structs. I figured that was out of scope for this small-ish task at hand.

We don't want to report these to rollbar.
@mitchell-as
Copy link
Contributor Author

Test failures are not due to this PR.

@mitchell-as mitchell-as requested a review from Naatan March 28, 2024 20:37
@mitchell-as mitchell-as marked this pull request as ready for review March 28, 2024 20:37
Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

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

I'm worried that this is going to silence errors that aren't user input. There is no guarantee that the path in question was not writable due to the user giving us something invalid, or due to a bug we introduced.

Seeing as that's the case, I don't think we can solve this problem with a catch-all. We need to catch the individual cases, because only then can we correctly assert the cause to be user input.

I'm putting it on request changes, but really I would propose we abandon this story.

@mitchell-as
Copy link
Contributor Author

I agree with you. We should handle these errors on a case-by-case basis as they come in from Rollbar. After all, that's what it's for...

@mitchell-as mitchell-as deleted the mitchell/dx-2646 branch March 28, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants