Consolidate launchpad interaction scripts into single launchpad_copy.py#110
Conversation
|
CI checks failed on this PR:
I'll investigate and push a fix. |
fbb1e2f to
5302259
Compare
|
CI fix: Removed |
rtibbles
left a comment
There was a problem hiding this comment.
This is a great start, but I think we can do some more cleanup.
Also as we are updating Python code here, let's add ruff to the pre-commit config, and add the rule to prevent inline imports. We can copy additional configuration from https://github.com/learningequality/le-skills/blob/main/pyproject.toml
scripts/launchpad_copy.py
Outdated
| from collections import defaultdict | ||
|
|
||
| try: | ||
| from launchpadlib.launchpad import Launchpad |
There was a problem hiding this comment.
I don't think there's a need to guard this, we should just raise an import error and make sure we are documenting dependencies properly (in pyproject.toml if needed) and ensuring we are installing the required dependencies in actions files and documenting this too.
There was a problem hiding this comment.
Fixed — removed import guard, imports now fail loudly. Added distro-info to CI apt deps.
scripts/launchpad_copy.py
Outdated
| RELEASE_PPA_NAME = "kolibri" | ||
| PACKAGE_WHITELIST = ["kolibri-server"] | ||
| SOURCE_SERIES = "jammy" | ||
| FALLBACK_SERIESES = ["plucky", "noble", "jammy", "focal"] |
There was a problem hiding this comment.
I realize this is copied from the original, but I think we should get rid of it and rely on querying for these values based on Ubuntu supported versions plus ELTS versions.
There was a problem hiding this comment.
Fixed — removed FALLBACK_SERIESES. Now relies on ubuntu-distro-info --supported-esm for dynamic discovery including ESM/ELTS.
scripts/launchpad_copy.py
Outdated
| # --- Caching decorators --- | ||
|
|
||
|
|
||
| class once: |
There was a problem hiding this comment.
Can you not just use stdlib memoize instead of this?
There was a problem hiding this comment.
Fixed — replaced with functools.cached_property.
scripts/launchpad_copy.py
Outdated
| return value | ||
|
|
||
|
|
||
| def cache(fn): |
There was a problem hiding this comment.
Fixed — replaced with functools.cache.
scripts/launchpad_copy.py
Outdated
| """Discover supported Ubuntu series dynamically, with hardcoded fallback.""" | ||
| try: | ||
| out = subprocess.check_output( | ||
| ["ubuntu-distro-info", "--supported"], text=True |
There was a problem hiding this comment.
Would this include ELTS as well? We want to support as wide a range as we can, and are only really limited by the bundled Python version (must be 3.6 or higher and 3.13 or lower).
There was a problem hiding this comment.
Now uses --supported-esm flag which should include ESM/ELTS versions.
scripts/launchpad_copy.py
Outdated
| def get_supported_series(source_series): | ||
| """Discover supported Ubuntu series dynamically, with hardcoded fallback.""" | ||
| try: | ||
| out = subprocess.check_output( |
There was a problem hiding this comment.
Also, we shouldn't mock this in tests, we should actually test the real call here to validate this code.
There was a problem hiding this comment.
Fixed — tests now call real ubuntu-distro-info, skipped on systems where it's not available. Added distro-info to CI apt install.
scripts/launchpad_copy.py
Outdated
|
|
||
|
|
||
| def enable_http_debugging(): | ||
| import httplib2 |
There was a problem hiding this comment.
What's the point of these inline imports?
There was a problem hiding this comment.
Fixed — moved httplib2 import to top level.
scripts/launchpad_copy.py
Outdated
| set_up_logging(logging.INFO) | ||
|
|
||
|
|
||
| def main(): |
There was a problem hiding this comment.
Let's put main last? Organise the code so callers come after their callees.
There was a problem hiding this comment.
Fixed — main() is now last. Code ordered: utilities → wrapper class → CLI → subcommand functions → main.
tests/vcr_config.py
Outdated
|
|
||
| import pytest | ||
|
|
||
| try: |
There was a problem hiding this comment.
Note, I don't think we need this. We should define the test dependencies, and just assume it exists.
There was a problem hiding this comment.
Fixed — removed try/except guard, imports vcr directly.
|
Thanks for the thorough review! Addressed all feedback:
|
5302259 to
462a6e4
Compare
|
Added PLC0415 ruff rule to prevent inline imports as requested. Fixed CI lint failures in kolibri_server_setup.py (unsorted imports, unused |
d4b377e to
f3935c2
Compare
rtibbles
left a comment
There was a problem hiding this comment.
Just a couple more thoughts but this is looking good.
|
Addressed the two remaining review comments. Ruff with PLC0415 was already added in the previous round. |
…ote subcommands Replace the separate copy_to_other_distributions.py and copy_package_proposed_to_ppa.py scripts with a single launchpad_copy.py that provides both operations as subcommands. Uses stdlib shutil.which instead of external 'which' package, adds proper error handling, and supports --dry-run for both subcommands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
CI checks failed on this PR:
I'll investigate and push a fix. |
Add pytest-based unit tests with VCR cassette support for testing launchpad_copy.py. Add a GitHub Actions workflow for running the Python test suite. Configure ruff linter and formatter via pyproject.toml and pre-commit hooks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update build_debian.yml to use the new launchpad_copy.py script. Remove the now-superseded copy_package_proposed_to_ppa.py and copy_to_other_distributions.py scripts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c6f36a7 to
db720a6
Compare
rtibbles
left a comment
There was a problem hiding this comment.
Good work, this is good to merge.
Phase 1: Create consolidated script with shared infrastructure
Phase 1: Create Consolidated Script with Shared Infrastructure
Goal
Create
scripts/launchpad_copy.pywith the sharedLaunchpadWrapperclass, argparse skeleton with two subcommands, and shared utilities. This phase builds the foundation that phases 2 and 3 plug into.Task 1: Create the shared infrastructure module
Files:
scripts/launchpad_copy.pyStep 1: Write the script skeleton with imports, constants, and argparse
Create
scripts/launchpad_copy.pywith:Step 2: Add the caching decorators (preserved from copy_to_other_distributions.py)
Step 3: Add the LaunchpadWrapper class with shared auth, PPA lookup, package filtering
This class is adapted from
copy_to_other_distributions.pybut generalized to support looking up both PPAs:Step 4: Add utility functions (logging, series discovery)
Step 5: Add argparse main with subcommand skeleton
Step 6: Verify
--helpworksRun:
python3 scripts/launchpad_copy.py --helpExpected: Shows usage with subcommands, exits 0.
Run:
python3 scripts/launchpad_copy.py copy-to-series --helpExpected: Shows copy-to-series help, exits 0.
Run:
python3 scripts/launchpad_copy.py promote --helpExpected: Shows promote help, exits 0.
Step 7: Commit
Phase 2: Implement `copy-to-series` subcommand
Phase 2: Implement
copy-to-seriesSubcommandGoal
Replace
cmd_copy_to_seriesplaceholder with full logic fromcopy_to_other_distributions.py. This replicates the behavior of iterating usable sources in the source series, checking each target series, queueing copies for missing packages, and performing the copies.Task 2: Implement
cmd_copy_to_seriesFiles:
scripts/launchpad_copy.py(replacecmd_copy_to_seriesplaceholder)Step 1: Replace
cmd_copy_to_serieswith the full implementationReplace the placeholder function with:
Step 2: Verify
--helpstill worksRun:
python3 scripts/launchpad_copy.py --helpExpected: exits 0, shows both subcommands.
Step 3: Commit
Phase 3: Implement `promote` subcommand
Phase 3: Implement
promoteSubcommandGoal
Replace
cmd_promoteplaceholder with logic fromcopy_package_proposed_to_ppa.py. This copies all published, whitelisted packages fromkolibri-proposedtokolibriPPA, handling obsolete series gracefully.Task 3: Implement
cmd_promoteFiles:
scripts/launchpad_copy.py(replacecmd_promoteplaceholder)Step 1: Replace
cmd_promotewith the full implementationReplace the placeholder function with:
Step 2: Verify
--helpstill worksRun:
python3 scripts/launchpad_copy.py --helpExpected: exits 0.
Step 3: Commit
Phase 4: Unit tests with pytest + vcrpy/mocking
Phase 4: Unit Tests with pytest + vcrpy/mocking
Goal
Add comprehensive unit tests for
scripts/launchpad_copy.pyusing pytest, unittest.mock, and vcrpy where appropriate. Since the Launchpad API requires credentials, most tests will use mocking. VCR is set up for potential future use with recorded cassettes.Task 4: Set up test infrastructure
Files:
tests/conftest.pytests/vcr_config.pyStep 1: Create test infrastructure files
tests/conftest.py:"""Shared pytest fixtures for launchpad_copy tests."""tests/vcr_config.py(following the pattern from kolibri-installer-debian):Step 2: Create the cassettes directory
Step 3: Commit
Task 5: Write unit tests for shared infrastructure
Files:
tests/test_launchpad_copy.pyStep 1: Write tests for caching decorators, series discovery, logging setup, and CLI parsing
Step 2: Run the tests
Run:
python3 -m pytest tests/test_launchpad_copy.py -vExpected: All tests pass.
Step 3: Commit
Task 6: Write unit tests for LaunchpadWrapper and subcommands
Files:
tests/test_launchpad_copy.py(append new test classes)Step 1: Add mock-based tests for LaunchpadWrapper
Append to
tests/test_launchpad_copy.py:Step 2: Run all tests
Run:
python3 -m pytest tests/test_launchpad_copy.py -vExpected: All tests pass.
Step 3: Commit
Phase 5: CI setup and workflow updates, delete old scripts
Phase 5: CI Setup, Workflow Updates, Delete Old Scripts
Goal
Set up a GitHub Actions workflow for Python tests, update
build_debian.ymlto use the new script, and delete the old scripts.Task 7: Create CI workflow for Python tests
Files:
.github/workflows/python_tests.ymlStep 1: Create the workflow file
Step 2: Verify actionlint passes
Run:
pre-commit run actionlint --all-filesExpected: Passes.
Step 3: Commit
Task 8: Update
build_debian.ymlto use new scriptFiles:
.github/workflows/build_debian.ymlStep 1: Update the
copy_to_other_distributionsjobChange the script invocation from:
python3 scripts/copy_to_other_distributions.pyto:
python3 scripts/launchpad_copy.py copy-to-seriesStep 2: Update the
copy_package_from_proposed_to_ppajobChange the script invocation from:
python3 scripts/copy_package_proposed_to_ppa.pyto:
python3 scripts/launchpad_copy.py promoteStep 3: Run pre-commit to validate YAML and actionlint
Run:
pre-commit run --all-filesExpected: All checks pass.
Step 4: Commit
Task 9: Delete old scripts
Files:
scripts/copy_to_other_distributions.pyscripts/copy_package_proposed_to_ppa.pyStep 1: Delete the old scripts
Step 2: Verify remaining scripts directory
Run:
ls scripts/Expected:
create_lp_creds.pyandlaunchpad_copy.pyStep 3: Run all tests to ensure nothing broke
Run:
python3 -m pytest tests/ -vExpected: All tests pass.
Step 4: Run pre-commit
Run:
pre-commit run --all-filesExpected: All checks pass.
Step 5: Commit
Summary
Consolidates
copy_to_other_distributions.pyandcopy_package_proposed_to_ppa.pyinto a singlescripts/launchpad_copy.pywith two argparse subcommands:copy-to-series— replacescopy_to_other_distributions.py. Copies packages from a source series to all other supported Ubuntu series within a PPA, with dynamic series discovery viaubuntu-distro-info.promote— replacescopy_package_proposed_to_ppa.py. Copies all published whitelisted packages fromkolibri-proposedPPA tokolibriPPA.Shared logic (Launchpad auth, PPA lookup, package whitelist filtering, obsolete-series error handling) is unified into a
LaunchpadWrapperclass with caching. The--verbosedefault is fixed fromNoneto0to preventTypeErrorin Python 3.Also adds 32 unit tests with pytest and vcrpy configuration, a GitHub Actions CI workflow for running tests on PRs, and updates
build_debian.ymlto use the new script.References
Reviewer guidance
python scripts/launchpad_copy.py --helpto verify CLI structurepython -m pytest tests/ -vto execute the 32 unit testsLaunchpadWrappercaching pattern is preserved from the originalcopy_to_other_distributions.pyOncedescriptor andcachedecorator provide memoization for Launchpad API callsTest evidence
Test execution (32 tests passing)
CLI help verification
Pre-commit linting
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Closes #106