Refactor per-channel output and add tests for fetch_all_channels by DarshanCode2005 · Pull Request #995 · NixOS/nix-security-tracker

DarshanCode2005 · 2026-04-14T20:17:42Z

# FIXME(@fricklerhandwerk): Fold that intobranch_info, so there's only one output. print("Parallel fetches results", results)
What’s changed:

Cleaner output: Each channel now prints one clear line with its details (release, state, commit) and result (fetched, already present, or error), after all fetches finish.
Standard output: Replaced print/pprint with self.stdout.write for proper command output handling.
Tests added:
- 4 unit tests for reporting logic
- 1 integration test for the full command flow with the database

fricklerhandwerk

There's some good thinking in it (especially the new test), but I recommend keeping it simple here: Aggregate the data into the collection of dicts and pprint them at the end.

fricklerhandwerk · 2026-04-20T07:02:47Z

+        coros = [repo.update_from_ref(ch.head_sha1_commit) for ch, _ in registered]
+        results = asyncio.run(wait_for_parallel_fetches(coros))


That probably also works

Suggested change

coros = [repo.update_from_ref(ch.head_sha1_commit) for ch, _ in registered]

results = asyncio.run(wait_for_parallel_fetches(coros))

results = asyncio.run(wait_for_parallel_fetches(

repo.update_from_ref(ch.head_sha1_commit) for ch, _ in registered

))

fricklerhandwerk · 2026-04-20T07:03:09Z

+                },
+                channel_branch=monitored.name,
            )
+            registered.append((channel, branch_info))


Instead of repeating all the dict field names, how about instead:

Suggested change

registered.append((channel, branch_info))

report_info = {"channel_branch": channel_branch, **defaults}

registered.append((channel, report_info))

The change can probably be a lot smaller then. I don't think there's much point in renaming the channel iterator variable.

Done , passing branch_info directly to update_or_create as the updated dict.

fricklerhandwerk · 2026-04-20T07:06:25Z

-                "head_sha1_commit": channel.revision,
-                "release_version": release_from_branch(channel.name),
+
+        # Step 1: register/update all channels in the DB, collect for reporting.


Suggested change

# Step 1: register/update all channels in the DB, collect for reporting.

I generally discourage this sort of comment for two reasons:

Which step in a sequence this is may change, and so the comment is likely to drift

It doesn't add information. The code doesn't do anything fancy and looking at it for a couple of seconds will explain it fully.

Comments should almost exclusively explain why a non-obvious piece of code is the way it is. Very rarely, if something that should be simple looks complicated, there should be a summary of what it does. This happens sometimes in this codebase where an involved DB query for can't be broken down into procedures with appropriate names (or where such a breakdown would introduce more problems than it would solve). But that's certainly not the case here.

fricklerhandwerk · 2026-04-20T07:09:59Z

 async def wait_for_parallel_fetches(
-    parallel_fetches: list[Coroutine[Any, Any, bool]],
+    parallel_fetches: Sequence[Coroutine[Any, Any, bool]],
 ) -> list[Any]:
    return await asyncio.gather(*parallel_fetches, return_exceptions=True)


Arguably this procedure does nothing on its own, why not inline it while we're at it?

Okay, removed the helper function entirely; asyncio.gather is now called inline inside asyncio.run.

fricklerhandwerk · 2026-04-20T07:18:04Z

+@pytest.mark.django_db
+@patch("shared.management.commands.fetch_all_channels.fetch_from_monitoring")
+@patch("shared.management.commands.fetch_all_channels.GitRepo")
+def test_command_upserts_channels_and_reports_fetch_results(


This is a very good test to have!

fricklerhandwerk · 2026-04-20T07:26:21Z

+def format_channel_report(
+    branch_info: dict[str, Any], fetch_result: bool | BaseException
+) -> str:
+    channel = branch_info["channel_branch"]
+    sha = branch_info["head_sha1_commit"][:12]
+    state = branch_info["state"]
+    release = branch_info.get("release_version") or "unstable"
+
+    if isinstance(fetch_result, BaseException):
+        status = f"ERROR ({fetch_result})"
+    elif fetch_result:
+        status = "fetched"
+    else:
+        status = "already present"
+
+    return f"[{channel}] release={release} state={state} commit={sha} -> {status}"


I imagined something much simpler: Just attach the fetch status to the already existing branch_info dict and pprint that.

Printing a line makes the log less readable, and will need work later to make it machine-readable once we start storing logs.

Hmm, okay format_channel_report is gone and pprints the dict directly.

fricklerhandwerk · 2026-04-20T07:31:23Z

+                },
+                channel_branch=monitored.name,
            )
+            registered.append((channel, branch_info))


We're not really using the NixChannel here -- that head_sha1_commit is also in the branch_info dict.

Okay, I've simplified this.

DarshanCode2005 added 2 commits April 15, 2026 01:38

refactor: unify per-channel output after git fetches

92a54cb

test: add tests for fetch_all_channels command

e701127

github-project-automation Bot added this to Nixpkgs Vulnerability Tracker Apr 14, 2026

chore: formatting

89d7c6b

fricklerhandwerk reviewed Apr 20, 2026

View reviewed changes

DarshanCode2005 added 2 commits April 22, 2026 18:05

test: simplify fetch_all_channels test

4d89f2d

refactor: address review comments on fetch_all_channels

f860335

		coros = [repo.update_from_ref(ch.head_sha1_commit) for ch, _ in registered]
		results = asyncio.run(wait_for_parallel_fetches(coros))

	registered.append((channel, branch_info))
	report_info = {"channel_branch": channel_branch, **defaults}
	registered.append((channel, report_info))

Conversation

DarshanCode2005 commented Apr 14, 2026

Uh oh!

fricklerhandwerk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants