-
Notifications
You must be signed in to change notification settings - Fork 124
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
Use better names for distribution tree subrepos #2185
Conversation
Attached issue: https://pulp.plan.io/issues/9566 |
26df1bb
to
3db98d4
Compare
3db98d4
to
9ae35ba
Compare
@@ -478,7 +478,7 @@ def is_subrepo(directory): | |||
if repodata == DIST_TREE_MAIN_REPO_PATH: | |||
treeinfo["repositories"].update({repodata: None}) | |||
continue | |||
name = f"{repodata}-{treeinfo['hash']}" | |||
name = f"{repodata}-{treeinfo['hash']}-{repository_pk}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goosemania One question, is there a reason we need to create new repos every time the treeinfo hash changes? Could we just create new versions on existing repos rather than entirely new repos? It seems like the current strategy would create a lot of junk data but there might be a good reason for it that I'm not aware of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No good reason that I'm aware of.
I think we did not want to deduplicate sub repos because their management looked complicated.
But I do not see a reason why not to create a new repo version.
The only question is how would you generate the repo name? If there were any changes, a treeinfo hash would likely be changed.
Just name = f"{repodata}-{repository_pk}"
without the hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking more about it, I'm not sure how to use subrepo versions. It seems that there should be some dist tree identifier in the subrepo name. I think we included it so subrepos are not reused when dist tree changes, to handle potential rollbacks better.
Imagine that you synced a distree with subrepos into repo version 1 of the main repo.
Now something changed in a sub repo, and distree is slightly different now, so a new dist tree content unit should be created and added to a repo version 2 of the main repo.
If for sub repo changes we will just create a new repo version and relate the same subrepo to the newly created distree content, whenever we roll back a main repo to repo version 1, we'll have no info or control which repo version each sub repo is supposed to be at, we'll use the latest and it will be wrong.
---------------- ----------------
| repo version 1 | | repo version 2 |
---------------- ----------------
| dist tree 1 | | dist tree 2 |
------------- -------------
\ /
----------
| sub repo |
----------
| v1 |
----
| v2 |
----
repos = list() | ||
for repo in RpmRepository.objects.filter(user_hidden=True).only("name").iterator(): | ||
if repo.name.count("-") == 1: | ||
repo.name = "-".join(repo.name, str(repo.pk)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also FWIW I'm pretty sure you were right the first time, we have to
go through every dist tree and check which repos its addons and variants refer to.
so I need to go back and do that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, unfortunately. Reading code now, I see that a repository_pk of a main repo is used, not the subrepo, that means we need to go through every dist tree.
It's not too late to change approach and support multiple naming schemes :D
Replacing with @goosemania 's pull request |
closes: #9566
https://pulp.plan.io/issues/9566