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

Fix returncode when deleting a mirror with snapshot #1201

Merged

Conversation

acdn-ndostert
Copy link

@acdn-ndostert acdn-ndostert commented Jul 24, 2023

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

  • author name in AUTHORS

@codecov
Copy link

codecov bot commented Jul 24, 2023

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%.

❗ 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%) ⬆️

... and 1 file with indirect coverage changes

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

@reglim
Copy link
Contributor

reglim commented Aug 18, 2023

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

Copy link
Contributor

@reglim reglim left a comment

Choose a reason for hiding this comment

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

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

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.
@acdn-ndostert
Copy link
Author

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

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

@reglim reglim self-requested a review August 18, 2023 12:19
Copy link
Contributor

@reglim reglim left a comment

Choose a reason for hiding this comment

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

👍

@reglim reglim merged commit 214e907 into aptly-dev:master Aug 18, 2023
4 of 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.

3 participants