[Pulp 2] Fix broken repodata signing if sqlite is enabled #2168
Conversation
dralley
changed the title
|
Attached issue: https://pulp.plan.io/issues/9206 |
dralley
force-pushed
the
fix-signing-pulp2
branch
from
745adb7
to
476121c
Compare
|
Tested here: https://pulp.plan.io/issues/9206#note-10 |
| self.add_child(RemoveOldRepodataStep()) | ||
| self.add_child(GenerateSqliteForRepoStep(self.get_working_dir())) | ||
| if config.get_boolean('gpg_sign_metadata'): |
There was a problem hiding this comment.
Signing used to happen as part of CloseRemoMetadataStep, but GenerateSqliteForRepoStep was rewriting the repomd.xml afterwards and immediately invalidating the signature. So we need to split the signing logic away from CloseRepoMetadataStep and make it a separate step that runs at the end.
| self.add_child(RemoveOldRepodataStep()) | ||
| self.add_child(GenerateSqliteForRepoStep(self.get_working_dir())) |
There was a problem hiding this comment.
What is the reason for moving that step?
There was a problem hiding this comment.
The reasoning was that GenerateSqliteForRepoStep is making changes to the metadata files that aren't tracked by the code that makes sure the RemoveOldRepodataStep doesn't touch them, so it feels safer to do it after the cleanup. I'm 99.9% sure that it would still have been fine anyway because "old" is time-based and usually measured in days, but I dislike that it was the only reason it wasn't broken.
| def finalize(self): | ||
| sign_options = configuration.get_gpg_sign_options( | ||
| self.get_repo(), self.get_config() | ||
| ) | ||
| assert sign_options | ||
| signer = util.Signer(options=sign_options) | ||
| signer.sign(self.repomd) |
There was a problem hiding this comment.
I had an impression that semantically in finalize, we usually perform some cleanup or closing actions and such. Here we are signing, basically performing the main action this step is for, so my natural approach would be to put it into process_main. Is it because this code was performed in the finalizing stage originally, you decided to keep it here?
There was a problem hiding this comment.
I was actually unsure about that because process_main seems to sometimes be used on a per-item basis. I can rename it to process_main if that would be more correct.
There was a problem hiding this comment.
I guess it's safer to leave it in finalize, since it was there before, and actually it would be more correct because finalization of other steps will be run by that time. If we put it in the process_main it might not happen in the correct order.
Anyway, you convinced me :) Let's keep it as you did it.
|
It looks good to me, I'd ask exd to test/review. |
|
cc @rohanpm, in case you'd like to check it out |
|
Well, I'm just going to merge this. EXD can suggest further changes when they around to reviewing |
closes: #9206
https://pulp.plan.io/issues/9206