Launchpad itself

Merge ~xnox/launchpad:copy-signing-bool into launchpad:master

Proposed by Dimitri John Ledkov on 2023-01-20

Status:	Needs review
Proposed branch:	~xnox/launchpad:copy-signing-bool
Merge into:	launchpad:master
Diff against target:	137 lines (+41/-2) 5 files modified lib/lp/archivepublisher/scripts/publish_ftpmaster.py (+3/-1) lib/lp/soyuz/scripts/custom_uploads_copier.py (+4/-0) lib/lp/soyuz/scripts/initialize_distroseries.py (+1/-0) lib/lp/soyuz/scripts/packagecopier.py (+11/-1) lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py (+22/-0)
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Colin Watson (community)	2023-01-20	Needs Information on 2023-03-14
Ubuntu Package Archive Administrators	2023-02-20	Pending
Review via email: mp+436161@code.qastaging.launchpad.net

Commit message

custom_uploads_copier.py: Support excluding signing

Add support in custom_uploads_copier to skip copying signing (uefi & signing).

Hook into initialise_distroseries with packagecopier to exclude copying signing. Not sure if this codepath is actually ever used.

Hook into publish_ftpmaster to exclude signing upon preparing to publish fresh series for the first time.

This patch by-itself doesn't expose similar functionality to the `copy-package` exported API, but it will help to implement that later on as well.

Note I only did checks with pre-commit, so unittests were not run, and I didn't do a deploy of development launchpad to test new series init to see if this does what I am intending for it to do.

Revision history for this message

Dimitri John Ledkov (xnox) wrote on 2023-02-20:

I was expecting for some archive publishing tests to fail, upon asserting that singing tarballs did not get copied up upon distro series init. But out of the sets of tests that I run, none failed. Maybe this is tested somewhere else, or hasn't been asserted before.

Revision history for this message

Colin Watson (cjwatson) wrote on 2023-03-14:

My biggest concern here is that the selection of custom upload types to exclude seems quite arbitrary just from looking at the code here, and it's not going to be obvious to future developers what overall properties of the system they need to maintain. At the moment I'm not even sure I understand the rationale well enough to be clear on whether this is the best approach.

Could you please add some commentary somewhere (in the actual code, not just in supporting material such as commit messages or merge proposal conversations) that explains the "why" of this change, as well as the "what"?

review: Needs Information

Revision history for this message

Dimitri John Ledkov (xnox) wrote on 2023-03-14:

I agree reading the diff it is not obvious, and it is very subtle as to what & why is being changed. In the code-paths that rarely anyone uses or changes. As this is literary only twice a year operation.

Revision history for this message

Colin Watson (cjwatson) wrote on 2023-03-14:

Yep, that's exactly why we need it to be clearly spelled out, if even I don't quite understand it :-)

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.