Bug #17704

Lint and refactor check-mirrors

Added by intrigeri 2020-05-10 08:48:14 . Updated 2020-05-10 09:22:23 .

Status:
Needs Validation
Priority:
Normal
Assignee:
sajolida
Category:
Infrastructure
Target version:
Start date:
Due date:
% Done:

0%

Feature Branch:
lint
Type of work:
Code
Blueprint:

Starter:
Affected tool:
check-mirrors
Deliverable for:

Description

In its 8.5 years of existence, check-mirrors.rb has grown a lot — some would say “organically” — without us taking much time to refactor the code along the way. IMO this program it has become too long, poorly structured, hard to understand, to maintain, and to modify.

Additionally, the code style is quite inconsistent both internally and wrt. Ruby best practices.

This issue tracks a first iteration about these 2 technical debt problems, not aiming at perfection whatsoever, but instead with the goal to handle a first batch of low hanging fruits, and give ourselves a better starting point next time we work on this code base.

Subtasks

History

#1 Updated by intrigeri 2020-05-10 09:22:23

  • Status changed from In Progress to Needs Validation
  • Assignee changed from intrigeri to sajolida

Hi sajolida,

I’m now reasonably satisfied with this branch and would like to see it merged. There’s no emergency at all, only the fact that I’d prefer to base unrelated future work on this branch (e.g. for Bug #17702).

Since we have no automated tests, here’s what I did:

  • locally (sid), with --fast: I tried --ip, --url-prefix, and none of them (i.e. all mirrors)
  • on misc.lizard (Stretch):
    • --fast --debug seems to work fine, although I’m still waiting for a full run to complete: the networking issues lizard has been experiencing for weeks makes this take a while
    • --debug i.e. all mirrors, i.e. what the cronjob does + debug output: seems to work fine, but here as well I did not wait for a full run to complete
  • on both my sid and misc.lizard:
    • ./check-mirrors.rb --allow-multiple --fast --channel stable --ip $(dig +short mirrors.kernel.org | tail -n1) tails-amd64-4.6 i.e. what the RM would do

My branch also fixes a couple bugs (af6e302f09ae00bb831713d9d873151d8b0a3e58) and drops an option that AFAIK we’ve not used for years (aea86070f8358ca8873502c9199a2f9003d21348).

I’m not sure what kind of code review we want here. I suspect a detailed line-by-line review could be a bit overkill, but I would appreciate if you could take a cursory look at the changes. In particular, if anything feels like a regression to you, for example in terms of your own ability to work on this code base, I’d be very interested to hear about it :)

I’ve not tested the Rubocop config with any other version of Rubocop than the one I have on sid. It’s been useful for me to modernize the code base, identify design problems, and establish a saner baseline. But for now, I would say feel free to ignore this Rubocop stuff entirely when you work on this code base. If you’re curious and want to try it: install the rubocop package and run the rubocop command from a checkout of this branch. I might run it from time to time and fix style regressions occasionally, we’ll see. At some point I’d like to have these checks run via GitLab CI, but that’s not the topic today.