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.

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

Remove FK to the main repo from a distribution tree Variant. #1782

Merged
merged 1 commit into from Jul 23, 2020

Conversation

goosemania
Copy link
Member

No issues with repository removal.
No issues with copying the same distribution tree into multiple repos.

closes #7150
https://pulp.plan.io/issues/7150

@pulpbot
Copy link
Member

pulpbot commented Jul 23, 2020

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

Comment on lines +311 to +315
is_main_repo = self._data[variant_key]["repository"] == DIST_TREE_MAIN_REPO_PATH
if is_main_repo:
repository = DIST_TREE_MAIN_REPO_PATH
packages = PACKAGES_DIRECTORY
else:
Copy link
Member Author

Choose a reason for hiding this comment

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

No change in logic here.
I had to reread this part many times, and always was confused, so I added the constant in a hope t o make it more readable.

Comment on lines -363 to -365
else:
repository = self._data[addon_key]["repository"]
packages = PACKAGES_DIRECTORY
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe there is a case when addon can point to a main repo, or is there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I have no idea. I guess we can wait until we find one that does.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, I did it because I thought it could be a possibility, but it was just a guess

@goosemania
Copy link
Member Author

No issues with repository removal.
No issues with copying the same distribution tree into multiple repos.

closes #7150
https://pulp.plan.io/issues/7150

closes #7096
https://pulp.plan.io/issues/7096

closes #7046
https://pulp.plan.io/issues/7046

[nocoverage]
@fao89
Copy link
Member

fao89 commented Jul 23, 2020

@fao89 , @daviddavis, @dkliban , what's the purpose of this?
https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/tasks/synchronizing.py#L989-L990

it was a re-sync problem, the StagesAPI machinery could handle the DTs, but it was erroring on addons and variants

Copy link
Member

@fao89 fao89 left a comment

Choose a reason for hiding this comment

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

LGTM! I just have one question, have you ran the performance tests locally? These 2 tests provide good feedback: https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/tests/performance/test_pulp_to_pulp.py#L135-L141

@goosemania
Copy link
Member Author

@fao89 , I have not. I usually let cron do that. Why do you point to those 2 specifically? two non-kickstart ones?

@fao89
Copy link
Member

fao89 commented Jul 23, 2020

@fao89 , I have not. I usually let cron do that. Why do you point to those 2 specifically? two non-kickstart ones?

my intention was to point to the kickstart, I point those because one of them has 2 variants (and usually it is the one that breaks on CI for subtle changes)

@goosemania goosemania merged commit a7c000c into pulp:master Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants