Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip comment lines in mirrorlist #1820

Merged
merged 1 commit into from
Aug 24, 2020
Merged

Skip comment lines in mirrorlist #1820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/7354.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix sync from mirrorlist with comments (like fedora's mirrorlist).
1 change: 1 addition & 0 deletions coverage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Manual Coverage
| As a user, I can sync and skip specific type (srpm) | NO | |
| As a user, I can sync opensuse repository | NO | |
| As a user, I can sync from a mirror list | YES | |
| As a user, I can sync from a mirror list with comments | YES | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

| As a user, I can sync from CDN using certificates | YES | |
| **Duplicates** | | |
| As a user, I have only one advisory with the same id in a repo version | YES | |
Expand Down
2 changes: 2 additions & 0 deletions pulp_rpm/app/tasks/synchronizing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ def fetch_mirror(remote):
with open(result.path) as mirror_list_file:
for mirror in mirror_list_file:
match = re.match(url_pattern, mirror)
if not match:
continue
Comment on lines +124 to +125
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

repodata_exists = get_repomd_file(remote, match.group(2))
if match and repodata_exists:
return match.group(2)
Expand Down
1 change: 1 addition & 0 deletions pulp_rpm/tests/functional/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@
EPEL7_URL = "https://dl.fedoraproject.org/pub/epel/7/x86_64/"
RPM_CDN_APPSTREAM_URL = "https://cdn.redhat.com/content/dist/rhel8/8.2/x86_64/appstream/os/"
RPM_CDN_BASEOS_URL = "https://cdn.redhat.com/content/dist/rhel8/8.2/x86_64/baseos/os/"
EPEL8_MIRRORLIST_URL = "https://mirrors.fedoraproject.org/mirrorlist?repo=epel-8&arch=x86_64"

PULP_TYPE_ADVISORY = 'rpm.advisory'
PULP_TYPE_PACKAGE = 'rpm.package'
Expand Down
5 changes: 5 additions & 0 deletions pulp_rpm/tests/performance/test_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
CENTOS8_BASEOS_URL,
CENTOS8_KICKSTART_APP_URL,
CENTOS8_KICKSTART_BASEOS_URL,
EPEL8_MIRRORLIST_URL,
)
from pulp_rpm.tests.functional.utils import (
gen_rpm_client,
Expand Down Expand Up @@ -168,6 +169,10 @@ def test_centos8_kickstart_appstream_on_demand(self):
"""Kickstart Sync CentOS 8 AppStream."""
self.rpm_sync(url=CENTOS8_KICKSTART_APP_URL)

def test_epel8_mirrorlist_with_comment(self):
"""Kickstart Sync EPEL 8 (which includes a comment line)."""
self.rpm_sync(url=EPEL8_MIRRORLIST_URL)


class CDNTestCase(unittest.TestCase):
"""Sync a repository from CDN."""
Expand Down