aptly-dev / aptly

aptly - Debian repository management tool
https://www.aptly.info/
MIT License
2.55k stars 369 forks source link

Fix returncode when deleting a mirror with snapshot #1201

Closed acdn-ndostert closed 11 months ago

acdn-ndostert commented 12 months ago

When trying to delete a mirror that has snapshot and not providing the force option, the API should not return a 500 StatusInternalServerError. A 403 StatusForbidden is more appropriate when the condition is expected by the server.

Fixes #

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

I was able to run make modules install and make test with success but I'm not able to run make system-test because of missing dependencies (gpg1).

Description of the Change

When implementing a client for Aptly API the 500 HTTP status code returned does not make it clear that there is an issue with the request. From https://www.rfc-editor.org/rfc/rfc9110.html#name-500-internal-server-error

15.6.1. 500 Internal Server Error The 500 (Internal Server Error) status code indicates that the server encountered an unexpected condition that prevented it from fulfilling the request.

The condition preventing the request to be fulfill is not unexpected and is even documented in the response.

Checklist

- [ ] unit-test added (if change is algorithm) - [ ] functional test added/updated (if change is functional) - [ ] man page updated (if applicable) - [ ] bash completion updated (if applicable) - [ ] documentation updated

codecov[bot] commented 12 months ago

Codecov Report

Merging #1201 (1689ee5) into master (1df8cff) will decrease coverage by 0.02%. Report is 4 commits behind head on master. The diff coverage is 57.14%.

:exclamation: Current head 1689ee5 differs from pull request most recent head 5e83a20. Consider uploading reports for the commit 5e83a20 to get more accurate results

@@            Coverage Diff             @@
##           master    #1201      +/-   ##
==========================================
- Coverage   65.99%   65.97%   -0.02%     
==========================================
  Files         143      143              
  Lines       16196    16190       -6     
==========================================
- Hits        10688    10682       -6     
- Misses       4754     4756       +2     
+ Partials      754      752       -2     
Files Changed Coverage Δ
api/mirror.go 14.78% <0.00%> (ø)
deb/graph.go 0.00% <0.00%> (ø)
api/task.go 42.15% <100.00%> (ø)
deb/import.go 76.50% <100.00%> (+0.92%) :arrow_up:

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

reglim commented 11 months ago

It's HTTP 403. not 404 as you said in the description, but I agree that StatusForbidden makes more sense.

acdn-ndostert commented 11 months ago

Can you change 404 to 403 in the commit and PR description? Otherwise LGTM

Done Sorry for the mistake, thank you for finding it :)