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

feat: Error if purge request made with dev mode disabled #3295

Merged
merged 19 commits into from
Dec 9, 2024

Conversation

ChrisBQu
Copy link
Collaborator

@ChrisBQu ChrisBQu commented Dec 6, 2024

Relevant issue(s)

Resolves #3140

Description

The issue was that if a purge request was made when the database was not in development mode, it would fail, and an error would be output on the node side of things. However, on the client side nothing indicated that the process had failed. Whether the purge was successful, or it failed, there would be no output.

I have created a new variable in the http package called IsDevMode, which is checked in the http/handler_extras.go/Purge function. If dev mode is enabled then htttp.StatusOK is written to the response header. If it is not enabled, then http.StatusBadRequest is written to the header instead, with a message indicating what happened.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

The platform(s) on which this was tested:

  • Windows

@ChrisBQu ChrisBQu added the area/cli Related to the CLI binary label Dec 6, 2024
@ChrisBQu ChrisBQu added this to the DefraDB v0.15 milestone Dec 6, 2024
@ChrisBQu ChrisBQu self-assigned this Dec 6, 2024
@ChrisBQu ChrisBQu closed this Dec 6, 2024
@ChrisBQu ChrisBQu reopened this Dec 6, 2024
@ChrisBQu ChrisBQu changed the title feat: Error message returned if purge request is made with dev mode disabled feat: Error returned if purge request is made with dev mode disabled Dec 6, 2024
@ChrisBQu ChrisBQu changed the title feat: Error returned if purge request is made with dev mode disabled feat: Error if purge request made with dev mode disabled Dec 6, 2024
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.09%. Comparing base (be3f381) to head (c8d5793).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
cli/start.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3295   +/-   ##
========================================
  Coverage    78.09%   78.09%           
========================================
  Files          382      382           
  Lines        35405    35412    +7     
========================================
+ Hits         27647    27654    +7     
+ Misses        6123     6122    -1     
- Partials      1635     1636    +1     
Flag Coverage Δ
all-tests 78.09% <87.50%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
http/errors.go 28.00% <ø> (ø)
http/handler.go 76.56% <ø> (ø)
http/handler_extras.go 100.00% <100.00%> (ø)
cli/start.go 25.73% <0.00%> (-0.11%) ⬇️

... and 19 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be3f381...c8d5793. Read the comment docs.

@ChrisBQu ChrisBQu requested a review from a team December 6, 2024 22:14
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

praise: The code looks good and is easy to follow :) Good job on implementing the chosen design.

todo: I do think the design is faulty here, this will improve things for the http and cli clients, but not the embedded clients. And it worsens an existing discrepancy between the embedded Go client and the http/cli clients.

I expect push back from some other team members on my todo, so maybe chat with them before making changes.

IMO this is adding more tech debt on top of existing tech debt, and it would be much more sensible/cost-effective to fix the existing purge problem first instead of continuing to build upon it.

@nasdf
Copy link
Member

nasdf commented Dec 9, 2024

todo: I do think the design is faulty here, this will improve things for the http and cli clients, but not the embedded clients. And it worsens an existing discrepancy between the embedded Go client and the http/cli clients.

The purge command doesn't need to exist in the embedded Go client. Its a development utility.

IMO this is adding more tech debt on top of existing tech debt, and it would be much more sensible/cost-effective to fix the existing purge problem first instead of continuing to build upon it.

I'm not sure what you mean by existing problem. Can you add more context?

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGMT. I don't see the tech debt issue raised by Andy as the purge command is a development utility that I don't see a need for on the embedded Go client. Let just wait to resolve the "purge problem" conversation before merging.

@AndrewSisley AndrewSisley dismissed their stale review December 9, 2024 20:58

Majority view is that Purge is not needed in the embedded Go client. I still think it would be architecturally nicer, cheaper maintenance-wise, basically free to have there, but I'm not going to try fight everyone on that, especially not in the middle of this PR :)

@ChrisBQu ChrisBQu merged commit d6900b7 into sourcenetwork:develop Dec 9, 2024
41 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to the CLI binary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

purge command should give feedback if can not be executed
4 participants