Skip comment lines in mirrorlist

[cognifloyd]

fixes #7354
https://pulp.plan.io/issues/7354

The fedora mirrorlists have a comment at the top. For example:

https://mirrors.fedoraproject.org/mirrorlist?repo=epel-8&arch=x86_64&country=US

# repo = epel-8 arch = x86_64 country = US 

The fetch_mirror function's docstring says it skips non-matching lines, but the task dies trying to access .group(2) on None.

        "error": {
            "description": "'NoneType' object has no attribute 'group'",
            "traceback": "  File \"/usr/local/lib/pulp/lib64/python3.6/site-packages/rq/worker.py\", line 934, in perform_job\n    rv = job.perform()\n  File \"/usr/local/lib/pulp/lib64/python3.6/site-packages/rq/job.py\", line 686, in perform\n    self._result = self._execute()\n  File \"/usr/local/lib/pulp/lib64/python3.6/site-packages/rq/job.py\", line 692, in _execute\n    return self.func(*self.args, **self.kwargs)\n  File \"/usr/local/lib/pulp/lib64/python3.6/site-packages/pulp_rpm/app/tasks/synchronizing.py\", line 211, in synchronize\n    remote_url = fetch_remote_url(remote)\n  File \"/usr/local/lib/pulp/lib64/python3.6/site-packages/pulp_rpm/app/tasks/synchronizing.py\", line 138, in fetch_remote_url\n    return fetch_mirror(remote)\n  File \"/usr/local/lib/pulp/lib64/python3.6/site-packages/pulp_rpm/app/tasks/synchronizing.py\", line 124, in fetch_mirror\n    repodata_exists = get_repomd_file(remote, match.group(2))\n"
        },

So, this PR skips lines that don't match the URL regex. I tested it by hotpatching my test installation and restarting the workers.

[pulpbot]

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

[goosemania]

@cognifloyd, thank you very much for the fix!
See my comment about the test. I hope adding it is quite straightforward and not much of a burden, it will definitely bring a lot of benefit.

[goosemania]

Good catch!

[goosemania]

Coverage is about tests. I can see how it is not clear from this page. Apologies. I'll update the language here.

We aim to add a test with every fix or feature.
I believe the link you shared in the issue, the mirror list for epel8, would be a good addition to our daily tests.
We have functional tests which run with every PR and we have performance/daily tests which run once a day.

Could you add one similar to this https://github.com/pulp/pulp_rpm/blob/3.6/pulp_rpm/tests/performance/test_sync.py#L167-L169 ? This way we'll keep this line you added to the coverage and we'll test daily that a mirror list works.

[cognifloyd]

I added this line because one of the tests complained that I didn't change anything in this file. It seemed rather odd, but I added it anyway to silence the linter (or whatever was checking it).

[pep8speaks]

Hello @cognifloyd! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-23 15:59:08 UTC

[cognifloyd]

I've added a test with that URL now. Thanks for pointing out where to add it.

[cognifloyd]

I added this line because one of the tests complained that I didn't change anything in this file. It seemed rather odd, but I added it anyway to silence the linter (or whatever was checking it).

[goosemania]

Sweeet! Thank you very much.

FWIW, the coverage check prompts/reminds to add a test :) It was very helpful to hear your view on this, it's very clear that we need to improve the language here, so contributors are not confused what is being asked.