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
Fix multiple bugs related to modularity #1254
Conversation
@goosemania, I will just fix all 3 in this pull request as they will all be fixed with very similar / the same changes |
04e7180
to
ab7e5ae
Compare
Ensure we don't publish module metadata files for repositories where the source repo had none. Use the correct mechanism for determining the checksum used in the name of the modules.yaml.gz metadata file. Additionally, refactor the module publish step to be more similar to the others. closes #4252 https://pulp.plan.io/issues/4252 closes #4253 https://pulp.plan.io/issues/4253
18703c7
to
a6a8e5d
Compare
Actually, I believe I'm mistaken about https://pulp.plan.io/issues/4268 being an issue in the first place. Only one of the files is actually listed in repomd.xml and therefore visible to tooling. It's just a difference between Pulp 3 and Pulp 2 that throws me off. The extra file would eventually get picked up by the RemoveOldRepodata step. |
""" | ||
with open(item.storage_path) as fp_in: | ||
document = fp_in.read() | ||
checksum = hashlib.sha256(document).hexdigest() |
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.
I'm not 100% sure why this check is necessary, but given that it was considered important enough to do + assign a coded exception to, I'm leaving it here.
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.
@dalley, is it still a valid comment? or an outdated one? I don't follow what you mean here. It looks like the code which performs a check is removed
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.
Ah I see, you moved it to metadata/modules.py
. Never mind.
super(ModulesFileContext, self).finalize() | ||
|
||
def add_document(self, document, doc_checksum): | ||
checksum = hashlib.sha256(document).hexdigest() |
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.
@dralley , it seems like that any checksum type except the sha256 raises exception (in the code below), doesn't it?
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.
I didn't experience that when I tested w/ checksum-type=sha1. I believe this particular checksum (of the content) is always sha256. It's for internal validation and isn't used in the name of the file or anything.
https://github.com/pulp/pulp_rpm/blob/2-master/plugins/pulp_rpm/plugins/db/models.py#L1808
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.
Thank you, @dralley !
Everything works as expected now.
Ensure we don't publish module metadata files for repositories where the source repo had none.
Use the correct mechanism for determining the checksum used in the name
of the modules.yaml.gz metadata file.
Additionally, refactor the module publish step to be more similar to the
others.
closes #4252
https://pulp.plan.io/issues/4252
closes #4253
https://pulp.plan.io/issues/4253