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

blob: convert gocloud errors in Open() into appropriate fs errors #3443

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

milescrabill
Copy link
Contributor

@milescrabill milescrabill commented Jun 4, 2024

Motivation:

  • http.ServeFileFS() uses errors.Is() to match on fs.ErrNotExist and fs.ErrPermission errors and return corresponding HTTP status codes.
  • gocloud currently returns fs.PathError which do not wrap the underlying fs errors, and which are not handled by http.ServeFileFS()'s error handling.
  • gocloud returning fs.PathErrors results in http.ServeFileFS() serving Internal Server Errors instead of Not Found or Permission Denied errors.

Fix:

  • wraps gocloud NotFound and PermissionDenied errors encountered in Open() around their io/fs counterparts
  • does not perform wrapping for other gocloud errors as they can either not occur or are ambiguous in this context

- wraps gocloud NotFound and PermissionDenied errors around their
  `io/fs` counterparts
- does not perform wrapping for other gocloud errors as they can either
  not occur or are ambiguous in this context
Copy link

google-cla bot commented Jun 4, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@achew22
Copy link

achew22 commented Jun 12, 2024

@peczenyj it seems you've had some luck getting things merged into this repo. Do you have any tips or tricks on how to get this one reviewed?

@vangent
Copy link
Contributor

vangent commented Jun 12, 2024

Please be patient.

@peczenyj
Copy link
Contributor

I had no idea, to be honest. I send the first pull request without any expectations based on my previous attempts to contribute to other google projects. Somehow this project is different, maybe it is more close to the open source community.

Is there any mailing list where you can expose your problem and mention that you have an awesome pull request for that?

If the maintainers do not react with the necessary speed, perhaps you should consider fork the project - if the license allow - and if your fork get some attention maybe it change things (like archive this repo and point yours as the replacement - why not?)

Sometimes I just start pull requests because code can be more direct than discussions. But not all maintainers like this approach, since I can suggest something undesirable or using the wrong design. It can end in frustration. But I try not to be sentimental.

When issues or pull requests have no feedback for some time it give the wrong impression that they just don’t care. But in fact they can be very busy.

In fact I saw, on this project, many small opportunities to contribute and things to improve, etc. I would love continue to send PRs but first I’d to see what I just did being released.

maybe one day I can have a more substantial role, the I can do more than just write random thoughts

@peczenyj
Copy link
Contributor

Or we can just ping @vangent and try to bribe him and with some beers

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 73.37%. Comparing base (ba58ec7) to head (0e27588).

Files Patch % Lines
blob/blob_fs.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3443      +/-   ##
==========================================
- Coverage   73.40%   73.37%   -0.03%     
==========================================
  Files         113      113              
  Lines       14948    14954       +6     
==========================================
  Hits        10973    10973              
- Misses       3201     3207       +6     
  Partials      774      774              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vangent vangent merged commit afdaba3 into google:master Jun 13, 2024
5 checks passed
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.

None yet

4 participants