feat: CLI purge command #2998

nasdf · 2024-09-12T00:56:21Z

Relevant issue(s)

Resolves #2953

Description

~~This PR adds a purge command that clears the data directory of the defradb instance on the local machine.~~

This PR adds a purge command that clears the configured datastore and restarts the defradb instance.

The purge command requires a --force flag to be run, and the defradb instance must have the development flag or config value enabled.

Add dev flag and config parameter that is required for destructive operations

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?

Added unit tests and tested manually

Specify the platform(s) on which this was tested:

MacOS

codecov · 2024-09-12T01:10:42Z

Codecov Report

Attention: Patch coverage is 53.28467% with 64 lines in your changes missing coverage. Please review.

Project coverage is 79.33%. Comparing base (75adf50) to head (62116d7).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
cli/start.go	15.62%	27 Missing ⚠️
node/node.go	51.16%	17 Missing and 4 partials ⚠️
http/client.go	0.00%	7 Missing ⚠️
node/store_badger.go	60.00%	2 Missing and 2 partials ⚠️
node/store_memory.go	25.00%	2 Missing and 1 partial ⚠️
cli/purge.go	92.86%	1 Missing ⚠️
node/store.go	87.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2998      +/-   ##
===========================================
- Coverage    79.47%   79.33%   -0.13%     
===========================================
  Files          331      333       +2     
  Lines        25686    25766      +80     
===========================================
+ Hits         20412    20441      +29     
- Misses        3819     3862      +43     
- Partials      1455     1463       +8

Flag	Coverage Δ
all-tests	`79.33% <53.28%> (-0.13%)`	⬇️

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

Files with missing lines	Coverage Δ
cli/cli.go	`100.00% <100.00%> (ø)`
cli/config.go	`83.67% <ø> (ø)`
cli/errors.go	`0.00% <ø> (ø)`
event/event.go	`100.00% <ø> (ø)`
http/handler.go	`81.03% <100.00%> (+0.68%)`	⬆️
http/handler_extras.go	`100.00% <100.00%> (ø)`
node/errors.go	`0.00% <ø> (ø)`
cli/purge.go	`92.86% <92.86%> (ø)`
node/store.go	`92.31% <87.50%> (-2.14%)`	⬇️
node/store_memory.go	`30.00% <25.00%> (-3.33%)`	⬇️
... and 4 more

... and 21 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 75adf50...62116d7. Read the comment docs.

AndrewSisley

The code looks good, but I have a couple of todos for you

AndrewSisley · 2024-09-12T13:02:27Z

cli/purge.go

+			return os.RemoveAll(cfg.GetString("datastore.badger.path"))
+		},
+	}
+	cmd.Flags().BoolVarP(&force, "force", "f", false, "Must be set for the operation to run")


todo: I don't think requiring -f is sufficient protection against users nuking a production database, I can't remember the details but I thought we agreed on something slightly safer when we discussed this as a team?

I only remember the force flag from the discussion. As long as its non interactive I'm open to changing it to anything else.

I think we had discussed the possibility of having a dev flag on startup with only dev instances capable of calling purge. If we do this, I think we don't even need force. Also, having a dev flag will allow for other commands to be added to dev only in the future. dev could by default require no password for the keyring for example. This could all be part of a strategy to get new devs to the "fun part" asap.

I think we had discussed the possibility of having a dev flag on startup with only dev instances capable of calling purge. If we do this, I think we don't even need force. Also, having a dev flag will allow for other commands to be added to dev only in the future. dev could by default require no password for the keyring for example. This could all be part of a strategy to get new devs to the "fun part" asap.

I don't remember this, but I like it - is kind of like a build flag without the hassle of multiple builds, and like you said it scales to other situations very nicely too

AndrewSisley · 2024-09-12T13:09:08Z

cli/purge.go

+				return ErrPurgeForceFlagRequired
+			}
+			cfg := mustGetContextConfig(cmd)
+			return os.RemoveAll(cfg.GetString("datastore.badger.path"))


thought: This will only clear the badger directory, if the user is using another datastore this command will not work (unclear if will panic, or quietly do nothing), and the added tests do not automatically cover other dbs. I cannot remember how hard-coded other CLI commands are to badger, this might be a drop in the ocean when it comes to adding support for other datastores in the CLI.

When we add more datastores we can have them all use the same datastore.path as their root path and have sub directories for each datastore type. Then we just need to delete the root path to remove them all.

AndrewSisley · 2024-09-12T13:11:59Z

cli/purge_test.go

+
+	err = cmd.Execute()
+	require.NoError(t, err)
+	assert.NoDirExists(t, dataDir)


todo: This test does not test that the badger data directory is cleared, but the hardcoded path set in the test (that currently corresponds with the default badger path). Please either change this test (or add another one) so that it will test that the correct directory is cleared, no matter what value is set within datastore.badger.path.

Why did you need to change the prod code to do this?

This was reverted after the team discussion

shahzadlone

I understand -f might not be sufficient. Perhaps if an additional WARNING message pops up and required an explicit [yes|Yes] input to continue it might be even better? However I am happy as is also and can improve it easily. Leaving LGTM but please do resolve Andy's concerns

fredcarle · 2024-09-13T17:28:41Z

cli/start.go

 			select {
+			case <-purgeSub.Message():
+				if !cfg.GetBool("development") {
+					goto SELECT


question: If the DB receives a purge command while not in development, this seems to give no feedback to the caller as to what happened. Shouldn't we log a cannot purge production database error/warning or something like that>

done. this returns and logs an error now

fredcarle · 2024-09-13T17:32:37Z

cli/start.go

 			}

-			n, err := node.NewNode(cmd.Context(), opts...)
+		START:
+			n, err := node.NewNode(cmd.Context(), nodeOpts...)


todo: Please add a top level message, if defra is in development mode, warning the user to not use this configuration in production.

fredcarle · 2024-09-13T17:45:19Z

http/handler_extras_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestPurge(t *testing.T) {


todo: Please also add integration tests that show that purging when in production mode will not work and also that purging while in development mode will in fact purge the database.

Discussed on Discord. The start command cannot currently be tested with the integration test suite and purge is handled within the start command. We will find a different way to test the effectiveness of the purge command.

The start command cannot currently be tested with the integration test suite and purge is handled within the start command

I'm curious - why can it not be tested with integration tests?

The start command cannot currently be tested with the integration test suite and purge is handled within the start command

I'm curious - why can it not be tested with integration tests?

It's possible but there are a few things that make it really difficult and not worth the effort in my opinion. No easy way for random port assignments and the timing of the command (waiting for start up / restart) will cause the tests to be flaky.

Why would any of these tests need to care about port assignment? We already have a Restart action, and whilst it isn't used that often I don't think I've seen it flake out.

Why would any of these tests need to care about port assignment?

To test the purge command you need the http api running.

Oh you changed it to go via the http client? Is that not exposing a highly destructive command a little too much? I wonder if that was an oversight in our last conversation.

Oh you changed it to go via the http client? Is that not exposing a highly destructive command a little too much? I wonder if that was an oversight in our last conversation.

That's why I've added the development mode config value that must be enabled to run purge.

Yeah, I assumed it would still only be available locally though, not over http too.

People will still mess up and accidentally set the dev mode to on in production deployments, and at the moment (prior to admin ACP, and correct config of it...) it would allow world plus dog to truncate the db.

We can't put guardrails against devs that shoot themselves in the foot to that extent. Devs can also just expose a production DB without any ACP policies. It would be just as bad. At least here we have to be explicit with putting the database in dev mode to allow the purge command.

fredcarle · 2024-09-13T19:50:06Z

http/handler_extras_test.go

+	res := rec.Result()
+	require.Equal(t, 200, res.StatusCode)
+
+	// test will timeout if purge never recieved


typo: received

fredcarle

LGTM. I like this approach.

Don't forget the typo I flagged earlier :)

AndrewSisley

LGTM, thanks Keenan - please make sure you address the remaining todos before merge (looks like a couple from other reviewers are outstanding).

islamaliev · 2024-10-15T13:17:14Z

bag bash result:
All is working, but I'd like to share some thoughts:
In normal mode:

$ defradb client purge
Error: run this command again with --force if you really want to purge all data

$ defradb client purge --force

There is no output as if it was successful.
Node's output:

Oct 15 13:57:19.878 INF cli Received purge event; restarting...
Oct 15 13:57:19.878 ERR cli failed to purge $err="cannot purge database when development mode is disabled"

I think it's would be better to output that purge can not be performed instead of letting it through and silently failing.

islamaliev · 2024-10-16T05:06:51Z

created #3140

nasdf added 2 commits September 11, 2024 17:52

add purge command

eeb02ff

make docs

c9dd6a9

nasdf added the area/cli Related to the CLI binary label Sep 12, 2024

nasdf added this to the DefraDB v0.14 milestone Sep 12, 2024

nasdf self-assigned this Sep 12, 2024

nasdf requested a review from a team September 12, 2024 00:56

AndrewSisley requested changes Sep 12, 2024

View reviewed changes

update tests

f1f20be

shahzadlone approved these changes Sep 12, 2024

View reviewed changes

nasdf added 3 commits September 12, 2024 15:49

rework purge command

162dccb

make docs

165d37f

add development flag

f5d2995

fredcarle reviewed Sep 13, 2024

View reviewed changes

add development mode banner. move purge logic to node package

d6ecc7a

nasdf force-pushed the nasdf/feat/cli-purge-command branch from cb015df to d6ecc7a Compare September 13, 2024 20:46

fredcarle approved these changes Sep 18, 2024

View reviewed changes

AndrewSisley approved these changes Sep 18, 2024

View reviewed changes

nasdf and others added 2 commits September 18, 2024 09:45

fix typo

53dde96

Merge branch 'develop' into nasdf/feat/cli-purge-command

62116d7

nasdf merged commit 2675b2e into sourcenetwork:develop Sep 18, 2024
41 of 43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: CLI purge command #2998

feat: CLI purge command #2998

nasdf commented Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Sep 12, 2024

nasdf Sep 12, 2024

fredcarle Sep 12, 2024

AndrewSisley Sep 12, 2024

AndrewSisley Sep 12, 2024

nasdf Sep 12, 2024

AndrewSisley Sep 12, 2024

nasdf Sep 12, 2024

AndrewSisley Sep 12, 2024

nasdf Sep 12, 2024

shahzadlone left a comment

fredcarle Sep 13, 2024

nasdf Sep 18, 2024

fredcarle Sep 13, 2024

nasdf Sep 18, 2024

fredcarle Sep 13, 2024

fredcarle Sep 13, 2024

AndrewSisley Sep 16, 2024

nasdf Sep 16, 2024

AndrewSisley Sep 16, 2024

nasdf Sep 16, 2024

AndrewSisley Sep 16, 2024

nasdf Sep 16, 2024

AndrewSisley Sep 16, 2024

fredcarle Sep 16, 2024

fredcarle Sep 13, 2024

nasdf Sep 18, 2024

fredcarle left a comment

AndrewSisley left a comment

islamaliev commented Oct 15, 2024 •

edited

Loading

islamaliev commented Oct 16, 2024

feat: CLI purge command #2998

feat: CLI purge command #2998

Conversation

nasdf commented Sep 12, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

islamaliev commented Oct 15, 2024 • edited Loading

islamaliev commented Oct 16, 2024

nasdf commented Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

islamaliev commented Oct 15, 2024 •

edited

Loading