add nix-collect-garbage endpoint #21

Open
def wants to merge 70 commits from def/nix-collect-garbage-endpoint into master
Collaborator
There is no content yet.
inex was assigned by def 2022-11-22 19:46:43 +02:00
def added spent time 2022-11-22 19:47:03 +02:00
25min
def added the due date 2022-11-24 2022-11-22 19:47:21 +02:00
def requested review from inex 2022-11-22 20:31:40 +02:00
inex was unassigned by def 2022-11-22 20:31:47 +02:00
def self-assigned this 2022-11-22 20:31:47 +02:00
inex refused to review 2022-11-22 20:38:24 +02:00
def added spent time 2022-11-23 06:35:51 +02:00
2h
def added 1 commit 2022-11-24 04:09:30 +02:00
def added spent time 2022-11-24 04:12:25 +02:00
1h
def added 1 commit 2022-11-24 04:33:09 +02:00
def added spent time 2022-11-24 04:33:38 +02:00
30min
def added spent time 2022-11-26 05:01:54 +02:00
40min
def added spent time 2022-12-03 20:28:26 +02:00
4h
def added 1 commit 2022-12-03 20:29:31 +02:00
def requested review from inex 2022-12-03 20:29:54 +02:00
def added 1 commit 2022-12-03 20:31:27 +02:00
inex requested changes 2022-12-08 18:19:02 +02:00
inex left a comment
Owner

Where are the tests?

Where are the tests?
def added 1 commit 2022-12-13 03:46:56 +02:00
def added 1 commit 2022-12-19 04:31:53 +02:00
def requested review from inex 2022-12-19 04:32:36 +02:00
def changed title from WIP: add nix-collect-garbage endpoint to add nix-collect-garbage endpoint 2022-12-19 04:32:50 +02:00
inex approved these changes 2022-12-19 15:49:37 +02:00
inex left a comment
Owner

You have tests in test_graphql folder, but there are no graphql tests it the file.

Now hook this up to GraphQL API and test on real server.

You have tests in `test_graphql` folder, but there are no graphql tests it the file. Now hook this up to GraphQL API and test on real server.
@ -0,0 +5,4 @@
COMPLETED_WITH_ERROR = "Completed with an error"
RESULT_WAAS_NOT_FOUND_ERROR = "We are sorry, result was not found :("

typo

typo
def marked this conversation as resolved
@ -0,0 +67,4 @@
for line in stream:
if "deleting '/nix/store/" in line:
percent += package_equal_to_percent

This may lead to percents not summing up to 100. You better count the number of paths deleted and divide them by a total.

This may lead to percents not summing up to 100. You better count the number of paths deleted and divide them by a total.
def marked this conversation as resolved
inex requested changes 2022-12-19 15:50:02 +02:00
inex left a comment
Owner

You have tests in test_graphql folder, but there are no graphql tests it the file.

Now hook this up to GraphQL API and test on real server.

You have tests in `test_graphql` folder, but there are no graphql tests it the file. Now hook this up to GraphQL API and test on real server.
def added 1 commit 2022-12-27 02:17:46 +02:00
def added 1 commit 2023-03-23 19:49:47 +02:00
def added 1 commit 2023-04-03 17:42:05 +03:00
def added 2 commits 2023-04-05 13:53:14 +03:00
def added 13 commits 2023-06-18 07:45:05 +03:00
def added 1 commit 2023-06-20 00:25:31 +03:00
def added 1 commit 2023-06-20 22:26:07 +03:00
def added 1 commit 2023-06-21 09:52:43 +03:00
def added 1 commit 2023-06-26 22:34:03 +03:00
continuous-integration/drone/pr Build is failing Details
continuous-integration/drone/push Build is failing Details
19005158a8
fix: add timewait
alexoundos reviewed 2023-06-29 15:13:38 +03:00
@ -0,0 +191,4 @@
(JobStatus.RUNNING, 10, "Сleaning...", ""),
(JobStatus.RUNNING, 15, "Сleaning...", ""),
(JobStatus.RUNNING, 20, "Сleaning...", ""),
(JobStatus.RUNNING, 25, "Сleaning...", ""),
Collaborator

Is it cyrillic letter С?

Is it cyrillic letter `С`?

yes, it is, @def should have fixed this.

yes, it is, @def should have fixed this.
Poster
Collaborator

fixed

fixed
def marked this conversation as resolved
def added 1 commit 2023-06-30 19:51:24 +03:00
continuous-integration/drone/push Build is failing Details
continuous-integration/drone/pr Build is failing Details
66970e6375
fix: stupid russian C
def requested review from inex 2023-06-30 19:51:54 +03:00
inex requested changes 2023-07-28 00:39:57 +03:00
inex left a comment
Owner

Rebase on the new branch

Rebase on the new branch
@ -0,0 +222,4 @@
assert log_event == reference
# андр конcтракнш

status update?

status update?
def marked this conversation as resolved
def added 1 commit 2023-08-03 04:48:15 +03:00
continuous-integration/drone/push Build is failing Details
continuous-integration/drone/pr Build is failing Details
7c68f05040
tests: delete old, fix
def added 1 commit 2023-08-07 05:24:58 +03:00
continuous-integration/drone/pr Build is failing Details
continuous-integration/drone/push Build is failing Details
1640ed44f6
tests: add success check
def requested review from inex 2023-08-07 05:25:07 +03:00
inex requested changes 2023-08-07 11:52:20 +03:00
inex left a comment
Owner

Rebase ASAP, you have conflicts.

Rebase ASAP, you have conflicts.
def added 2 commits 2023-08-07 15:56:45 +03:00
continuous-integration/drone/push Build is failing Details
a316f8b910
tests: fix
def added 1 commit 2023-08-07 15:58:47 +03:00
continuous-integration/drone/push Build is failing Details
be821fe1e0
Delete 'dump.rdb'
def added 1 commit 2023-08-07 16:00:02 +03:00
continuous-integration/drone/push Build is failing Details
aadf0cb1a3
Delete 'zsh-config.nix'
def added 1 commit 2023-08-07 16:38:53 +03:00
continuous-integration/drone/push Build is failing Details
446220a9c5
fix: replace os to subprocess
def added 1 commit 2023-09-01 18:21:47 +03:00
def requested review from inex 2023-09-01 18:24:50 +03:00
inex requested review from houkime 2023-09-27 12:55:00 +03:00
houkime requested changes 2023-09-27 19:28:53 +03:00
@ -127,2 +130,4 @@
code=500,
)
@strawberry.mutation(permission_classes=[IsAuthenticated])
Collaborator

Here you wrote that it is a mutation, but you test it as a query.

Here you wrote that it is a mutation, but you test it as a query.
Poster
Collaborator

If you mean I don't ask for mutation fields, I've changed that.

so, done?

If you mean I don't ask for mutation fields, I've changed that. so, done?

In tests, rename query to mutation

In tests, rename query to mutation
Poster
Collaborator

oh, oke. done

oh, oke. done
inex marked this conversation as resolved
@ -0,0 +19,4 @@
)
def run_nix_collect_garbage():
Collaborator

Add typing to functions

Add typing to functions
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +25,4 @@
).stdout
def parse_line(line):
Collaborator

You mean some very specific type of line here. Please, paste an example as a docstring and maybe rename function for clarity

You mean some very specific type of line here. Please, paste an example as a docstring and maybe rename function for clarity
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +31,4 @@
if match is None:
return (
JobStatus.FINISHED,
Collaborator

There is JobStatus.Error, please use it

There is `JobStatus.Error`, please use it

Also, the whole tuple doesn't look very good. Why do you return the tuple[Literal[JobStatus.FINISHED], Literal[100], Literal['Completed with an error'], Literal['We are sorry, result was not found :(']] | tuple[Literal[JobStatus.FINISHED], Literal[100], Literal['Cleaning completed.'], str] type?

Also, the whole tuple doesn't look very good. Why do you return the `tuple[Literal[JobStatus.FINISHED], Literal[100], Literal['Completed with an error'], Literal['We are sorry, result was not found :(']] | tuple[Literal[JobStatus.FINISHED], Literal[100], Literal['Cleaning completed.'], str]` type?
Poster
Collaborator

(1) done

(1) done
inex marked this conversation as resolved
@ -0,0 +40,4 @@
else:
return (
JobStatus.FINISHED,
100,
Collaborator

In current version of jobs, it is not necessary to set progress to 100 explicitly when finishing the job

In current version of jobs, it is not necessary to set progress to 100 explicitly when finishing the job
Poster
Collaborator

Okay, but I won't change it so I don't have to rewrite the argument return.

Okay, but I won't change it so I don't have to rewrite the argument return.

you should rewrite the return type of this function

you should rewrite the return type of this function
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +43,4 @@
100,
CLEAR_COMPLETED,
f"{match.group(0)} have been cleared",
)
Collaborator

You can pass job as an argument to a function to not have these complex resturns

You can pass job as an argument to a function to not have these complex resturns
Poster
Collaborator

Yeah, but I've already done it my way

Yeah, but I've already done it my way

you didn't

you didn't
@ -0,0 +46,4 @@
)
def stream_process(job, stream, total_dead_packages):
Collaborator

Consider renaming to process_stream.
We do not stream processes here, we process streams.

Consider renaming to `process_stream`. We do not stream processes here, we process streams.
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +129,4 @@
response = authorized_client.post(
"/graphql",
json={
"query": RUN_NIX_COLLECT_GARBAGE_QUERY,
Collaborator

This is not a query, this is a Mutation. Both semantically and you wrote it yourself.

This is not a query, this is a Mutation. Both semantically and you wrote it yourself.
Poster
Collaborator

If you mean I don't ask for mutation fields, I've changed that.

so, done?

If you mean I don't ask for mutation fields, I've changed that. so, done?

no, it is about naming.

no, it is about naming.
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +137,4 @@
assert response.json().get("data") is not None
assert response.json()["data"]["system"]["nixCollectGarbage"]["success"] is True
assert response.json()["data"]["system"]["nixCollectGarbage"]["message"] is not None
assert response.json()["data"]["system"]["nixCollectGarbage"]["success"] == True
Collaborator

is True already checks that it is a boolean and it is True

`is True` already checks that it is a boolean and it is True
Poster
Collaborator

done

done
inex marked this conversation as resolved
houkime reviewed 2023-09-27 19:42:36 +03:00
@ -0,0 +69,4 @@
mock = mocker.patch(
"selfprivacy_api.jobs.nix_collect_garbage.set_job_status_wrapper",
autospec=True,
return_value=set_job_status,
Collaborator

what are you trying to do here exactly?(dead code?)

what are you trying to do here exactly?(dead code?)
Poster
Collaborator

done

done
inex marked this conversation as resolved
inex requested changes 2023-09-29 18:38:20 +03:00
inex left a comment
Owner

Declare return types.

Declare return types.
@ -129,0 +137,4 @@
return GenericJobMutationReturn(
success=True,
code=200,
message="Cleaning started..,",
  • why is there a ,
  • why not "Garbage collector started"
- why is there a `,` - why not "Garbage collector started"
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +7,4 @@
COMPLETED_WITH_ERROR = "Completed with an error"
RESULT_WAS_NOT_FOUND_ERROR = "We are sorry, result was not found :("

So the user sees "We are sorry, result was not found :(" after starting garbage collection. What would this even mean?

So the user sees "We are sorry, result was not found :(" after starting garbage collection. What would this even mean?
Poster
Collaborator

done

done

Remove emotions from the message.

Remove emotions from the message.
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +100,4 @@
if dead_packages == 0:
Jobs.update(
job=job,

You've declared that job is of Jobs type, but this function expects the Job type.

You've declared that `job` is of `Jobs` type, but this function expects the `Job` type.
Poster
Collaborator

done

done

You've broken it even further. Also improve your tests to check the status of Job object.

You've broken it even further. Also improve your tests to check the status of Job object.
Poster
Collaborator

the error highlighting tells me that I've definitely fixed it now.

the error highlighting tells me that I've definitely fixed it now.
Poster
Collaborator

done

done
inex marked this conversation as resolved
def changed title from add nix-collect-garbage endpoint to WIP: add nix-collect-garbage endpoint 2023-10-05 17:36:29 +03:00
def added 3 commits 2023-10-12 01:10:02 +03:00
def added 1 commit 2023-10-12 01:18:38 +03:00
continuous-integration/drone/push Build is failing Details
f2ead0c77d
fix: typos
def added 1 commit 2023-10-12 01:26:36 +03:00
continuous-integration/drone/push Build is failing Details
70a498d0e5
fix: missclick with Job jobs
def added 1 commit 2023-10-12 01:40:57 +03:00
continuous-integration/drone/push Build is failing Details
98eef8d08e
fix: tests, jobs.error return
def requested review from inex 2023-10-12 01:41:44 +03:00
def added 1 commit 2023-10-12 01:59:41 +03:00
continuous-integration/drone/push Build is failing Details
eda5923cc6
tests: fix query
def requested review from houkime 2023-10-12 02:00:36 +03:00
inex requested changes 2023-10-13 20:00:44 +03:00
@ -0,0 +14,4 @@
CLEAR_COMPLETED = "Cleaning completed."
def run_nix_store_print_dead() -> PIPE:

Variable PIPE not allowed in type expression

Variable `PIPE` not allowed in type expression
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +25,4 @@
)
def run_nix_collect_garbage() -> subprocess.Popen:

Expression of type "IO[bytes] | None" cannot be assigned to return type "Popen[Unknown]"
Type "IO[bytes] | None" cannot be assigned to type "Popen[Unknown]"
"IO[bytes]" is incompatible with "Popen[Unknown]"

Expression of type "IO[bytes] | None" cannot be assigned to return type "Popen[Unknown]" Type "IO[bytes] | None" cannot be assigned to type "Popen[Unknown]" "IO[bytes]" is incompatible with "Popen[Unknown]"
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +58,4 @@
)
def process_stream(job, stream, total_dead_packages):

no typing

no typing
Poster
Collaborator

done

done
inex marked this conversation as resolved
@ -0,0 +89,4 @@
)
def get_dead_packages(output):

no typing

no typing
Poster
Collaborator

done

done
inex marked this conversation as resolved
inex requested changes 2023-10-14 13:05:46 +03:00
@ -0,0 +18,4 @@
RESULT_WAS_NOT_FOUND_ERROR,
)
pytest_plugins = ("pytest_asyncio",)
  • Do you actually need this dependency?
  • This breaks CI. New deps must be discussed.
- Do you actually need this dependency? - This breaks CI. New deps must be discussed.
Poster
Collaborator

done

done
Poster
Collaborator

deadcode

deadcode
inex marked this conversation as resolved
def added 1 commit 2023-10-16 19:40:12 +03:00
continuous-integration/drone/push Build is failing Details
c0f8b9026b
fix: corrected from comment from pr
inex requested review from inex 2023-10-17 19:46:27 +03:00
def added 1 commit 2023-11-11 23:13:26 +02:00
continuous-integration/drone/push Build is failing Details
9114bc6ae0
fix: remove unused imports
def added 1 commit 2023-11-11 23:16:14 +02:00
continuous-integration/drone/push Build is failing Details
d31ad487eb
fix: corrected from comment from pr
def added 1 commit 2023-11-11 23:38:39 +02:00
continuous-integration/drone/push Build is failing Details
3be5816f51
fix: del unused return from parse line
Poster
Collaborator

relevant for the review

relevant for the review
def changed title from WIP: add nix-collect-garbage endpoint to add nix-collect-garbage endpoint 2023-11-11 23:39:59 +02:00
def added 1 commit 2023-11-15 14:47:12 +02:00
continuous-integration/drone/push Build is failing Details
03e6d45279
fix: final typos fix
def changed title from add nix-collect-garbage endpoint to WIP: add nix-collect-garbage endpoint 2023-11-17 21:08:30 +02:00
def added 1 commit 2023-11-29 11:03:08 +02:00
continuous-integration/drone/push Build is failing Details
372eadd306
tests: fix test_parse_line
def changed title from WIP: add nix-collect-garbage endpoint to add nix-collect-garbage endpoint 2023-11-29 11:03:38 +02:00
def added 1 commit 2024-01-17 17:26:18 +02:00
continuous-integration/drone/push Build is failing Details
bb1e1bcae0
fix: rename functions
def added 1 commit 2024-01-23 19:01:34 +02:00
inex requested changes 2024-01-26 12:58:17 +02:00
inex left a comment
Owner

Tests are not mocked, nix-collect-garbage is called on the host machine.

Tests are not mocked, `nix-collect-garbage` is called on the host machine.
def added 1 commit 2024-01-30 16:04:52 +02:00
continuous-integration/drone/push Build is failing Details
045ad30ec3
fix: add mock for nix collect garbage
def added 1 commit 2024-01-30 16:05:05 +02:00
def requested review from inex 2024-01-30 16:06:30 +02:00
houkime requested changes 2024-01-31 13:23:20 +02:00
@ -194,0 +201,4 @@
return GenericJobMutationReturn(
success=True,
code=200,
Collaborator

add a handler for an error too

add a handler for an error too
@ -0,0 +46,4 @@
if match is None:
Jobs.update(
job=job,
status=JobStatus.ERROR,
Collaborator

Errored jobs are currently not handled by this endpoint.
Add a handler and a test where the call errors out.

Errored jobs are currently not handled by this endpoint. Add a handler and a test where the call errors out.
inex requested changes 2024-02-01 15:08:02 +02:00
@ -0,0 +124,4 @@
"""
def test_graphql_nix_collect_garbage(authorized_client, mock_delete_old_gens_and_print_dead):

Lacking test of trying to call mutation without auth

Lacking test of trying to call mutation without auth
Poster
Collaborator

done

done
def added 2 commits 2024-02-03 21:11:07 +02:00
def added 1 commit 2024-02-05 11:46:51 +02:00
continuous-integration/drone/push Build is failing Details
29596a4f47
docs: changed the description of the error class
def added 1 commit 2024-02-08 16:40:23 +02:00
continuous-integration/drone/push Build is failing Details
567f094336
fix: frem review
def requested review from inex 2024-02-08 16:40:44 +02:00
def requested review from houkime 2024-02-08 16:40:46 +02:00
Poster
Collaborator

just MERGE it !

image

just MERGE it ! ![image](/attachments/07059dbd-af46-4b32-9264-ba4b6035eeb0)
def added 1 commit 2024-02-15 01:00:07 +02:00
continuous-integration/drone/push Build is failing Details
7d58b76a5a
fix: rm .gitignore
def added 1 commit 2024-02-15 01:02:08 +02:00
continuous-integration/drone/push Build is failing Details
5d6cdd1b62
fix: add .gitignore from master (git's doing crazy stuff)
Poster
Collaborator

This pull request has changes conflicting with the target branch.

.gitignore

FUUUU YOU

> This pull request has changes conflicting with the target branch. > .gitignore FUUUU YOU
Collaborator

rebase please (or merge)
Then run test
otherwise lgtm

rebase please (or merge) Then run test otherwise lgtm
def added 2 commits 2024-03-01 17:02:26 +02:00
def force-pushed def/nix-collect-garbage-endpoint from bfad0aaeb9 to 8ce83f7b7e 2024-03-01 17:09:55 +02:00 Compare
def added 1 commit 2024-03-01 17:10:36 +02:00
Poster
Collaborator

done

done
Poster
Collaborator

What are you waiting for?

What are you waiting for?

It is Sunday

It is Sunday
inex requested changes 2024-03-04 00:19:47 +02:00
@ -0,0 +56,4 @@
@pytest.fixture
def mock_delete_old_gens_and_return_dead_report(mocker):

I would suggest mocking system calls, and not your own functions.

I would suggest mocking system calls, and not your own functions.
@ -0,0 +127,4 @@
"""
def test_graphql_nix_collect_garbage(authorized_client, mock_delete_old_gens_and_return_dead_report):

This test just hangs and doesn't finish

This test just hangs and doesn't finish
Poster
Collaborator

done

done

Wait for #98 to merge and use the fp mock afterwards.

Wait for https://git.selfprivacy.org/SelfPrivacy/selfprivacy-rest-api/pulls/98 to merge and use the `fp` mock afterwards.
Collaborator

@def please gitignore pycache, it shows 195 files changed

@def please gitignore pycache, it shows 195 files changed
def added 66 commits 2024-04-10 17:58:25 +03:00
continuous-integration/drone/pr Build is failing Details
continuous-integration/drone/push Build is failing Details
19005158a8
fix: add timewait
continuous-integration/drone/push Build is failing Details
continuous-integration/drone/pr Build is failing Details
66970e6375
fix: stupid russian C
continuous-integration/drone/push Build is failing Details
continuous-integration/drone/pr Build is failing Details
7c68f05040
tests: delete old, fix
continuous-integration/drone/pr Build is failing Details
continuous-integration/drone/push Build is failing Details
1640ed44f6
tests: add success check
continuous-integration/drone/push Build is failing Details
a316f8b910
tests: fix
continuous-integration/drone/push Build is failing Details
be821fe1e0
Delete 'dump.rdb'
continuous-integration/drone/push Build is failing Details
aadf0cb1a3
Delete 'zsh-config.nix'
continuous-integration/drone/push Build is failing Details
446220a9c5
fix: replace os to subprocess
continuous-integration/drone/push Build is failing Details
f2ead0c77d
fix: typos
continuous-integration/drone/push Build is failing Details
70a498d0e5
fix: missclick with Job jobs
continuous-integration/drone/push Build is failing Details
98eef8d08e
fix: tests, jobs.error return
continuous-integration/drone/push Build is failing Details
eda5923cc6
tests: fix query
continuous-integration/drone/push Build is failing Details
c0f8b9026b
fix: corrected from comment from pr
continuous-integration/drone/push Build is failing Details
9114bc6ae0
fix: remove unused imports
continuous-integration/drone/push Build is failing Details
d31ad487eb
fix: corrected from comment from pr
continuous-integration/drone/push Build is failing Details
3be5816f51
fix: del unused return from parse line
continuous-integration/drone/push Build is failing Details
03e6d45279
fix: final typos fix
continuous-integration/drone/push Build is failing Details
372eadd306
tests: fix test_parse_line
continuous-integration/drone/push Build is failing Details
bb1e1bcae0
fix: rename functions
continuous-integration/drone/push Build is failing Details
045ad30ec3
fix: add mock for nix collect garbage
continuous-integration/drone/push Build is failing Details
29596a4f47
docs: changed the description of the error class
continuous-integration/drone/push Build is failing Details
567f094336
fix: frem review
def added 1 commit 2024-04-10 18:07:41 +03:00
continuous-integration/drone/push Build is passing Details
a4b1c058f7
style: fix black
def changed title from add nix-collect-garbage endpoint to WIP: add nix-collect-garbage endpoint 2024-04-12 01:23:04 +03:00
def added 1 commit 2024-04-13 13:58:24 +03:00
continuous-integration/drone/push Build is passing Details
2024ff4865
tests: add fp mock
def changed title from WIP: add nix-collect-garbage endpoint to add nix-collect-garbage endpoint 2024-04-13 13:59:12 +03:00
def requested review from inex 2024-04-13 13:59:14 +03:00
Poster
Collaborator

image

finally

![image](/attachments/3e5fd2d4-d357-4a64-af5c-104f0403034c) finally
All checks were successful
continuous-integration/drone/push Build is passing
This Pull Request doesn't have enough approvals yet. 0 of 1 approvals granted.
You are not authorized to merge this pull request.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
4 Participants
Notifications
Total Time Spent: 8 hours 35 minutes
def
8 hours 35 minutes
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

2022-11-24

Dependencies

No dependencies set.

Reference: SelfPrivacy/selfprivacy-rest-api#21
There is no content yet.