Deserialize treeinfo files in a scpecific order #1639
Conversation
There was a problem hiding this comment.
All comments are related to sync openSUSE Leap 15.1 from here:
http://download.opensuse.org/distribution/leap/15.1/repo/oss/
Summarizing:
- By our side, the issue was the disorderly deserialization
- By productmd, the validation on
shortfor release section - By repo, for not having timestamp
pulp_rpm/app/kickstart/treeinfo.py
Outdated
| deserialize_order = "header release tree variants checksums images stage2 media".split() | ||
| sections = parser._sections.keys() | ||
|
|
||
| for section in sections: | ||
| for section in deserialize_order: |
There was a problem hiding this comment.
ProductMD follows a specific order to deserialize:
https://github.com/release-engineering/productmd/blob/master/productmd/treeinfo.py#L120
There was a problem hiding this comment.
it would be helpful to have it in the comments so anyone who looks later at the code understood why there is some special handling present.
pulp_rpm/app/kickstart/treeinfo.py
Outdated
| if "release" in sections: | ||
| release = parser._sections.get("release") | ||
| release["short"] = release.get("short", release["name"].split()[0]) |
There was a problem hiding this comment.
it was breaking because the treeinfo file didn't have a short name
pulp_rpm/app/kickstart/treeinfo.py
Outdated
| @@ -55,6 +55,37 @@ def load(self, f): | |||
| except MissingSectionHeaderError: | |||
| raise TypeError(_("Treeinfo file should have INI format")) | |||
|
|
|||
| def sections_from_general(self, parser): | |||
There was a problem hiding this comment.
general is formed by data from other sections:
https://productmd.readthedocs.io/en/latest/treeinfo-1.0.html
for this case, it had data on general, but not on the "original sections", so I built these sections from the general, using the link above as reference
There was a problem hiding this comment.
@fabricio-aguiar , @daviddavis , does it mean that during publication we'll get a different treeinfo file?
Or do we keep treeinfo file untouched?
There was a problem hiding this comment.
treeinfo file will be untouched, it just changes the way we read it
pulp_rpm/app/kickstart/treeinfo.py
Outdated
|
|
||
| if general and "tree" not in sections: | ||
| tree = {} | ||
| fake_timestamp = "".join([str(ord(a)) for a in general["name"].replace(" ", "")]) |
There was a problem hiding this comment.
this is super dirty!
the treeinfo file didn't have a timestamp
http://download.opensuse.org/distribution/leap/15.1/repo/oss/.treeinfo
[header]
version = 1.0
[release]
name = openSUSE Leap
version = 15.1
[general]
arch = x86_64
family = openSUSE Leap
name = openSUSE Leap 15.1
version = 15.1
platforms = x86_64,xen
[images-x86_64]
kernel = boot/x86_64/loader/linux
initrd = boot/x86_64/loader/initrd
[images-xen]
kernel = boot/x86_64/loader/linux
initrd = boot/x86_64/loader/initrd
depending on the header version, productmd validates if timestamp is integer or float.
I needed something unique by treeinfo file, and it should be a number, so I took the general.name (in this case is: openSUSE Leap 15.1), and replaced the letter by numbers
There was a problem hiding this comment.
@fabricio-aguiar , naive question. Can we just set timestamp to 0 if it's not available?
Why do we need something unique?
There was a problem hiding this comment.
I think it is not naive, actually it is much better than what I did. I thought on uniqueness because of unique_together, but putting a 0 would solve it
|
@goosemania @daviddavis this PR unblocks @pavelpicka |
fao89
changed the title
|
Changing this PR since the main issue was addressed on release-engineering/productmd#141 |
| for section in sections: | ||
| # ProductMD follows a specific order to deserialize: | ||
| # https://github.com/release-engineering/productmd/blob/master/productmd/treeinfo.py#L120 | ||
| deserialize_order = "header release tree variants checksums images stage2 media".split() |
There was a problem hiding this comment.
It's an interesting approach, it wouldn't cross my mind to create a list this way!
I'm on the fence.
Having a list (or maybe a tuple in this case is better) explicitly seems to me the obvious choice because the goal is to have a sequence. At the same time it would mean many quotation marks which can be seen as bringing noise.
Having it as a string and then split is a bit unintuitive way to introduce a sequence but there is no noise with quotes.
|
Attached issue: https://pulp.plan.io/issues/6322 |
https://pulp.plan.io/issues/6322
closes #6322