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

As a user, I can perform a content-only mirror of a repo. #2146

Merged
merged 1 commit into from
Oct 18, 2021
Merged

As a user, I can perform a content-only mirror of a repo. #2146

merged 1 commit into from
Oct 18, 2021

Conversation

dralley
Copy link
Contributor

@dralley dralley commented Oct 6, 2021

@pulpbot
Copy link
Member

pulpbot commented Oct 6, 2021

Attached issue: https://pulp.plan.io/issues/9316

@dralley dralley force-pushed the mirror-modes branch 2 times, most recently from 5e66a86 to 620ad7e Compare October 6, 2021 19:49
@dralley dralley changed the title [prototype] As a user, I can perform a content-only mirror of a repo. As a user, I can perform a content-only mirror of a repo. Oct 6, 2021
@dralley dralley marked this pull request as ready for review October 6, 2021 19:49
@dralley dralley force-pushed the mirror-modes branch 8 times, most recently from e13ea3d to bb172c9 Compare October 9, 2021 01:08
@dralley dralley force-pushed the mirror-modes branch 2 times, most recently from c9f0e25 to 295269d Compare October 13, 2021 01:51
@dralley dralley requested a review from ipanova October 13, 2021 02:11
@dralley dralley force-pushed the mirror-modes branch 2 times, most recently from c6072d4 to a860877 Compare October 13, 2021 02:35
@dralley
Copy link
Contributor Author

dralley commented Oct 13, 2021

The CLI tests are failing due to ACS being pulled out, PR here: pulp/pulp-cli#392

ipanova
ipanova reviewed Oct 13, 2021
View reviewed changes
pulp_rpm/app/viewsets.py Outdated Show resolved Hide resolved
@dralley
Copy link
Contributor Author

dralley commented Oct 14, 2021 •
edited
Loading

The mirrorlist test is flaky because the mirror that tends to be picked is the MIT one, and it's broken.

@dralley
Copy link
Contributor Author

dralley commented Oct 14, 2021

And I suppose we can have the sync_policy vs. sync_mode discussion again one last time

ipanova
ipanova reviewed Oct 15, 2021
View reviewed changes
pulp_rpm/app/serializers/repository.py Outdated
mirror = data.get("mirror", None)
sync_policy = data.get("sync_policy", None)

if mirror and sync_policy:
Copy link
Member

Choose a reason for hiding this comment

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

if 'mirror' in data and 'sync_policy' in data....

otherwise

$ http :24817/pulp/api/v3/repositories/rpm/rpm/46335757-c221-4539-874b-6e21bcce6674/sync/ remote=/pulp/api/v3/remotes/rpm/rpm/3a95a54f-31cb-40ff-a7fa-33c38ed8085d/ sync_policy=mirror_complete mirror=False
HTTP/1.1 202 Accepted
Access-Control-Expose-Headers: Correlation-ID
Allow: POST, OPTIONS
Connection: close
Content-Length: 67
Content-Type: application/json
Correlation-ID: 0313a5fad5314321b0ce0aac60497ef6
Date: Fri, 15 Oct 2021 10:36:53 GMT
Referrer-Policy: same-origin
Server: gunicorn
Vary: Accept, Cookie
X-Content-Type-Options: nosniff
X-Frame-Options: DENY

{
    "task": "/pulp/api/v3/tasks/f5511ee9-3efa-4d22-a5db-9638c2412c3a/"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I'll also add a test for this.

ipanova
ipanova reviewed Oct 15, 2021
View reviewed changes
pulp_rpm/tests/functional/api/test_consume_content.py
@@ -111,11 +132,11 @@ def test_with_xml_base_in_metadata(self):
completely different base path when looking up relative paths.
"""
delete_orphans()
self.do_test("immediate", mirror=False, url=REPO_WITH_XML_BASE_URL)
self.do_test("immediate", sync_policy="mirror_content_only", url=REPO_WITH_XML_BASE_URL)
Copy link
Member

Choose a reason for hiding this comment

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

it probably does not matter whether the policy is additive or mirror_content_only in this testcase?

Copy link
Contributor Author

@dralley dralley Oct 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right, it does not. But this is expected to fail with metadata mirroring so I figured I ought to use a mirror mode since it would be more likely to be mixed up with future changes.

@ipanova
Copy link
Member

ipanova commented Oct 15, 2021

And I suppose we can have the sync_policy vs. sync_mode discussion again one last time

I'd prefer to stick to sync_policy, since users are already familiar with this concept in the remote's download policy.
I've tested the PR and it works fine apart when making changes to the sync_policy/autopublish in between the syncs

@dralley dralley force-pushed the mirror-modes branch 4 times, most recently from e2c47d5 to af767c1 Compare October 15, 2021 16:18
ipanova
ipanova approved these changes Oct 18, 2021
View reviewed changes
Copy link
Member

@ipanova ipanova left a comment

Choose a reason for hiding this comment

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

Looks good, I'll review policy change in the other PR

@ipanova ipanova mentioned this pull request Oct 18, 2021
Merged
@dralley
Copy link
Contributor Author

dralley commented Oct 18, 2021

Test failure unrelated

goosemania
goosemania reviewed Oct 18, 2021
View reviewed changes
CHANGES/9316.feature Outdated Show resolved Hide resolved
goosemania
goosemania reviewed Oct 18, 2021
View reviewed changes
docs/workflows/create_sync_publish.rst Outdated Show resolved Hide resolved
docs/workflows/create_sync_publish.rst Show resolved Hide resolved
pulp_rpm/app/serializers/repository.py Show resolved Hide resolved
pulp_rpm/app/serializers/repository.py Outdated Show resolved Hide resolved
pulp_rpm/app/serializers/repository.py Show resolved Hide resolved
pulp_rpm/app/tasks/synchronizing.py Outdated Show resolved Hide resolved
pulp_rpm/app/viewsets.py
Comment on lines +142 to +145
err_msg = (
"Cannot use '{}' in combination with a 'mirror_complete' or "
"'mirror_content_only' sync policy."
)
Copy link
Member

Choose a reason for hiding this comment

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

Could you remind me why we are not using i18n here? to preserve api parameter names?

Copy link
Contributor Author

@dralley dralley Oct 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, is there a way to i18n the string without changing particular substrings?

requirements.txt Outdated Show resolved Hide resolved
@dralley dralley merged commit 3b275d9 into pulp:master Oct 18, 2021
@dralley dralley deleted the mirror-modes branch October 18, 2021 21:49
@dralley dralley mentioned this pull request Feb 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swadeley swadeley swadeley left review comments

@goosemania goosemania goosemania left review comments

@ipanova ipanova ipanova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dralley @pulpbot @ipanova @swadeley @goosemania