Skip to content

feat(server): add delete endpoint #136

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Sep 3, 2023
Merged

feat(server): add delete endpoint #136

merged 14 commits into from
Sep 3, 2023

Conversation

ajbdev
Copy link
Contributor

@ajbdev ajbdev commented Aug 31, 2023 •
edited
Loading

Description

Add a DELETE endpoint guarded by a separate auth token array.

Motivation and Context

Closes #127

How Has This Been Tested?

Wrote a test case and tested manually.

Changelog Entry

### Added

- Change `Config.get_tokens` to `Config.get_auth_tokens` and update invocations.
- Add delete_tokens array to config and `get_delete_tokens` method to Config for retrieving delete tokens.
- Add DELETE endpoint that will remove files if a delete token is set and authorized by the request.
- Test for new DELETE endpoint
- Add DELETE_TOKEN_ENV for setting the delete tokens array from an ENV var.

-->

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@ajbdev ajbdev requested a review from orhun as a code owner August 31, 2023 22:16
@ajbdev ajbdev mentioned this pull request Aug 31, 2023
Closed
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2023 •
edited
Loading

Codecov Report

Patch coverage is 87.17% of modified lines.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Files Changed Coverage
src/config.rs 80.00%
src/server.rs 91.66%

📢 Thoughts on this report? Let us know!.

tessus
tessus reviewed Sep 1, 2023
View reviewed changes
Copy link
Collaborator

@tessus tessus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

closes #127 (has to be set by OP in first comment: replace See #127 with closes #127)

@tessus
Copy link
Collaborator

tessus commented Sep 1, 2023

I was thinking a bit more. Maybe we should create an enum for token_type: auth, delete. And then use just one function get_tokens(&self, enum). I have no idea, whether this is better code or me just trying to optimize without any gain.

For me thia PR is perfecctly fine. Let's see what @orhun has to say.

orhun
orhun reviewed Sep 1, 2023
View reviewed changes
Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Just had a few comments 🐻

@tessus I don't know what would be the benefit of adding an enum but we can go with that for the sake of having a single function for retrieving the tokens. @ajbdev can you also make that change?

tessus and others added 2 commits September 1, 2023 11:58
@tessus @orhun
Update README.md
64bc21b 
Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>
@tessus @orhun
Update README.md
5db6d8b 
Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>
@tessus
Copy link
Collaborator

tessus commented Sep 1, 2023

@tessus I don't know what would be the benefit of adding an enum but we can go with that for the sake of having a single function for retrieving the tokens. @ajbdev can you also make that change?

I came up with this "brilliant" idea, thus I'll take care of it. ;-)

@tessus
refactor(server): use log::error! and return 500
98edc90 
Co-authored-by: Helmut K. C. Tessarek <tessarek@evermeet.cx>
@tessus
test(server): add test_delete_file_without_token_in_config
861a358
@tessus
refactor(server): use one function to retrieve auth/delete tokens
787591e 
Co-authored-by: Helmut K. C. Tessarek <tessarek@evermeet.cx>
@tessus
Copy link
Collaborator

tessus commented Sep 1, 2023

@orhun all done!

@tessus
Copy link
Collaborator

tessus commented Sep 1, 2023 •
edited
Loading

@orhun It seems that gh had a connection issue during 2 tests. Any chance you can rerun the action?

If I run the tests locally, everything is fine.

P.S.: Thanks for rerunning the action.

@tessus tessus mentioned this pull request Sep 1, 2023
Merged
@orhun orhun changed the title Delete endpoint implementation with delete_tokens configuration feat(server): add delete endpoint Sep 3, 2023
orhun
orhun approved these changes Sep 3, 2023
View reviewed changes
Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Tested and works well!

Thank you both @ajbdev @tessus 🚀

@orhun orhun merged commit 27e3be5 into orhun:master Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tessus tessus tessus left review comments

@orhun orhun orhun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DELETE end point
4 participants
@ajbdev @codecov-commenter @tessus @orhun