add nix-collect-garbage endpoint #21
Labels
No Label
Bug
Contributions welcome
Did not do
Duplicate
Feature
Module
Backups
Module
GraphQL
Priority
High
Priority
Low
Priority
Medium
Refactor
Severity
High
Severity
Low
Severity
Medium
No Milestone
No project
No Assignees
4 Participants
Notifications
Total Time Spent: 8 hours 35 minutes
Due Date
def
8 hours 35 minutes
Dependencies
No dependencies set.
Reference: SelfPrivacy/selfprivacy-rest-api#21
Loading…
Reference in New Issue
There is no content yet.
Delete Branch "def/nix-collect-garbage-endpoint"
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?
Where are the tests?
WIP: add nix-collect-garbage endpointto add nix-collect-garbage endpointYou 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
@ -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.
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 +191,4 @@
(JobStatus.RUNNING, 10, "Сleaning...", ""),
(JobStatus.RUNNING, 15, "Сleaning...", ""),
(JobStatus.RUNNING, 20, "Сleaning...", ""),
(JobStatus.RUNNING, 25, "Сleaning...", ""),
Is it cyrillic letter
С
?yes, it is, @def should have fixed this.
fixed
Rebase on the new branch
@ -0,0 +222,4 @@
assert log_event == reference
# андр конcтракнш
status update?
Rebase ASAP, you have conflicts.
@ -127,2 +130,4 @@
code=500,
)
@strawberry.mutation(permission_classes=[IsAuthenticated])
Here you wrote that it is a mutation, but you test it as a query.
If you mean I don't ask for mutation fields, I've changed that.
so, done?
In tests, rename query to mutation
oh, oke. done
@ -0,0 +19,4 @@
)
def run_nix_collect_garbage():
Add typing to functions
done
@ -0,0 +25,4 @@
).stdout
def parse_line(line):
You mean some very specific type of line here. Please, paste an example as a docstring and maybe rename function for clarity
done
@ -0,0 +31,4 @@
if match is None:
return (
JobStatus.FINISHED,
There is
JobStatus.Error
, please use itAlso, 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?(1) done
@ -0,0 +40,4 @@
else:
return (
JobStatus.FINISHED,
100,
In current version of jobs, it is not necessary to set progress to 100 explicitly when finishing the job
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
done
@ -0,0 +43,4 @@
100,
CLEAR_COMPLETED,
f"{match.group(0)} have been cleared",
)
You can pass job as an argument to a function to not have these complex resturns
Yeah, but I've already done it my way
you didn't
@ -0,0 +46,4 @@
)
def stream_process(job, stream, total_dead_packages):
Consider renaming to
process_stream
.We do not stream processes here, we process streams.
done
@ -0,0 +129,4 @@
response = authorized_client.post(
"/graphql",
json={
"query": RUN_NIX_COLLECT_GARBAGE_QUERY,
This is not a query, this is a Mutation. Both semantically and you wrote it yourself.
If you mean I don't ask for mutation fields, I've changed that.
so, done?
no, it is about naming.
done
@ -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
is True
already checks that it is a boolean and it is Truedone
@ -0,0 +69,4 @@
mock = mocker.patch(
"selfprivacy_api.jobs.nix_collect_garbage.set_job_status_wrapper",
autospec=True,
return_value=set_job_status,
what are you trying to do here exactly?(dead code?)
done
Declare return types.
@ -129,0 +137,4 @@
return GenericJobMutationReturn(
success=True,
code=200,
message="Cleaning started..,",
,
done
@ -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?
done
Remove emotions from the message.
done
@ -0,0 +100,4 @@
if dead_packages == 0:
Jobs.update(
job=job,
You've declared that
job
is ofJobs
type, but this function expects theJob
type.done
You've broken it even further. Also improve your tests to check the status of Job object.
the error highlighting tells me that I've definitely fixed it now.
done
add nix-collect-garbage endpointto WIP: add nix-collect-garbage endpoint@ -0,0 +14,4 @@
CLEAR_COMPLETED = "Cleaning completed."
def run_nix_store_print_dead() -> PIPE:
Variable
PIPE
not allowed in type expressiondone
@ -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]"
done
@ -0,0 +58,4 @@
)
def process_stream(job, stream, total_dead_packages):
no typing
done
@ -0,0 +89,4 @@
)
def get_dead_packages(output):
no typing
done
@ -0,0 +18,4 @@
RESULT_WAS_NOT_FOUND_ERROR,
)
pytest_plugins = ("pytest_asyncio",)
done
deadcode
relevant for the review
WIP: add nix-collect-garbage endpointto add nix-collect-garbage endpointadd nix-collect-garbage endpointto WIP: add nix-collect-garbage endpointWIP: add nix-collect-garbage endpointto add nix-collect-garbage endpointTests are not mocked,
nix-collect-garbage
is called on the host machine.@ -194,0 +201,4 @@
return GenericJobMutationReturn(
success=True,
code=200,
add a handler for an error too
@ -0,0 +46,4 @@
if match is None:
Jobs.update(
job=job,
status=JobStatus.ERROR,
Errored jobs are currently not handled by this endpoint.
Add a handler and a test where the call errors out.
@ -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
done
just MERGE it !
FUUUU YOU
rebase please (or merge)
Then run test
otherwise lgtm
bfad0aaeb9
to8ce83f7b7e
done
What are you waiting for?
It is Sunday
@ -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.
@ -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
done
Wait for #98 to merge and use the
fp
mock afterwards.@def please gitignore pycache, it shows 195 files changed
add nix-collect-garbage endpointto WIP: add nix-collect-garbage endpointWIP: add nix-collect-garbage endpointto add nix-collect-garbage endpointfinally