Copy modulemd_defaults units as well while populating ubi repos #59
Conversation
|
Please review if the changes done is OK or not, |
ubipop/_pulp_client.py
Outdated
| def search_module_defaults(self, repo, name, stream): | ||
| url = "repositories/{REPO_ID}/search/units/".format(REPO_ID=repo.repo_id) | ||
| criteria = {"type_ids": ["modulemd_defaults"], | ||
| "filters": {"unit": {"name": name, "stream": stream}} |
There was a problem hiding this comment.
not well indented
There was a problem hiding this comment.
Hm, looks good to me.
Do you mean I should do it like this?
criteria = {
'fourspace' "type_ids": ["modulemd_defaults"],
"filters": {"unit": {"name": name, "stream": stream}
}
Or, can you give an example of standard indent here?
ubipop/_pulp_client.py
Outdated
| query_list = [] | ||
| for md_d in module_defaults: | ||
| query_list.append({'$and': [{'name': md_d.name}, | ||
| {'stream': md_d.stream}, |
There was a problem hiding this comment.
also not standard indent
JayZ12138
changed the title
JayZ12138
force-pushed
the
6738
branch
3 times, most recently
from
61c32a9
to
693bd9f
Compare
| @@ -182,7 +208,8 @@ def associate_units(self, src_repo, dest_repo, units, type_ids): | |||
| def _get_query_list(self, type_ids, units): | |||
There was a problem hiding this comment.
Hmm... guess this isn't introduced by this commit, but semantics of this method seem a bit strange.
It accepts type_ids which presumably is a list/tuple, but instead of processing everything in the list, it uses if, elif, elif to select only a single thing from the list... so for example, if type_ids is ["modulemd", "rpm"] then it behaves the same as if type_ids was just ["modulemd"]. Why is modulemd higher priority than modulemd_defaults which is higher priority than rpm/srpm? What's the point of it being a list anyway?
I'm not asking for this to be changed here though, since it wasn't introduced in this PR.
ubipop/_pulp_client.py
Outdated
| def name_profiles(self): | ||
| """ | ||
| flatten the profles and prepend name | ||
| 'ruby:2.5:common:unique' |
There was a problem hiding this comment.
Is this a syntax you've seen somewhere else in tools dealing with modules, or did you just make it up here?
I think it's a poor choice due to sharing the same format with module specifiers themselves, typically given as N:S:V:C:A with trailing portions able to be dropped. It's also a bit weird that : is doubling up here as "separator between different types of data" and "separator between items next to each other in a list".
So, for example, if I just have a string 'ruby:2.5:common:unique' how do I know whether that's referring to module named 'ruby', stream '2.5', version 'common', context 'unique'? Or module named 'ruby', stream '2.5' containing profiles named 'common' and 'unique' ?
Actually, isn't this format itself ambiguous even if considered alone? You're returning only a single string and every separator is ':', even for multiple keys under self.profiles (i.e. multiple streams). So, correct me if I'm wrong, but it seems both of these:
"name": "ruby",
"profiles": {
"2.5": ["common"],
"3.1": ["default", "minimal"],
},
"name": "ruby",
"profiles": {
"2.5": ["common", "3.1", "default", "minimal"],
},
...would be flattened to:
ruby:2.5:common:3.1:default:minimal
Hence this is actually an ambiguous transformation, multiple different inputs can map to the same outputs. I'm sure the above case is not that likely, nevertheless it seems technically unsafe for use in the diffs here.
If this is just an arbitrary format you're making up here, would it be better to choose something which is unambiguous both when considered alone and when compared with the existing N:S:V:C:A scheme?
Although, going a bit further - why do we even need to be flattening this into a string for diff purposes? If you want to be able to test whether two objects are equal, the most straightforward way to do that is to implement __eq__ on the class and then check instance1 == instance2. It's unnecessarily convoluted to instead introduce some flattened string formatting just so you can check instance1.string_formatted == instance2.string_formatted. Though I didn't check whether that's also used for some logging purposes.
There was a problem hiding this comment.
Yeah. It's made up myself, I was trying to make something that's unique to a modulemd_defaults unit, so we can determine if it's already in the repo/need to be replaced or not. Didn't expect we should be able to tell what the unit is from that string.
I did implement eq in the ModueDefaults class, then I see how it's dealt with module and rpm, so I changed to this.
I'll do changes, either make the string more clear or use eq instead.
There was a problem hiding this comment.
I changed the format to 'ruby:[2.5:common,unique]' now, but I still feel a little strange to have [] inside the string. Do you think it's OK?
Or I'll change to eq tomorrow.
There was a problem hiding this comment.
Yeah, I think this change addresses the main points I'd raised.
I think the code around here is still more complicated than it needs to be - it still should be unnecessary to make up string representations of objects just to store and compare them. That's why we have the ability to override __hash__, __eq__ and similar methods in the first place, so you can compare objects, use them in dicts and so on for your own custom classes just as easily as you can do for integers, strings etc.
I understand there's some urgency to get this fixed though and I don't want to block it just for that.
If there modulemd_defaults for specific modulemd in the same repo, then it should be copied to the ubi repos. Ubipop now looks for modulemd_defaults units after searching modulemd units with the same name and stream. If there's any, add it to push queue. This fixes release-engineering#56
If there modulemd_defaults for specific modulemd in the same repo,
then it should be copied to the ubi repos.
Ubipop now looks for modulemd_defaults units after searching modulemd
units with the same name and stream. If there's any, add it to push
queue.
This fixes #56