Skip to content

Conversation

@cmatsuoka
Copy link
Contributor

@cmatsuoka cmatsuoka commented Dec 4, 2025

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint && make test?
  • Have you added an entry to the changelog (docs/reference/changelog.rst)?

Note for reviewers: see the expected output in tests.

@cmatsuoka cmatsuoka force-pushed the work/CRAFT-4923-Prototype-craft-backend-for-lp-test branch 4 times, most recently from 4af3fff to 7ff9ba8 Compare December 4, 2025 19:32
@cmatsuoka cmatsuoka changed the base branch from main to spike/launchpad-testing December 4, 2025 19:41
@cmatsuoka cmatsuoka requested a review from Copilot December 4, 2025 19:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements support for the lp-test backend in the craft testing framework. The lp-test backend uses OpenStack for running spread tests, as an alternative to the existing local lxd-vm backend or CI backend.

Key changes:

  • Added automatic detection of lp-test backend when LP_TEST_ACCOUNT environment variable is set
  • Implemented OpenStack backend configuration for lp-test with system image mappings for Ubuntu 20.04, 22.04, and 24.04
  • Extended spread models to support OpenStack-specific fields (endpoint, account, key, location, plan, halt_timeout) and image specifications per system

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
tests/unit/services/test_testing.py Added comprehensive tests for both regular craft backend and lp-test backend spread file processing
tests/unit/models/test_spread.py Updated existing tests with new images parameter and added test for lp-test backend transformation
craft_application/services/testing.py Implemented lp-test backend detection, configuration, and image mapping for Ubuntu systems
craft_application/models/spread.py Extended models to support image field in systems and OpenStack backend fields, updated transformation logic to handle image mapping

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka cmatsuoka force-pushed the work/CRAFT-4923-Prototype-craft-backend-for-lp-test branch from 7ff9ba8 to 5bf3a17 Compare December 4, 2025 19:52
@cmatsuoka cmatsuoka marked this pull request as ready for review December 4, 2025 20:21
@cmatsuoka cmatsuoka requested a review from a team as a code owner December 4, 2025 20:21
Comment on lines +148 to +153
endpoint: str | None = None
account: str | None = None
key: str | None = None
location: str | None = None
plan: str | None = None
halt_timeout: str | None = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC halt_timeout applies to all backends. What about the rest of these?

The main reason I ask is because of https://github.com/lengau/spread-schema


_SYSTEM_IMAGES = {
"lp-test": {
"ubuntu-20.04": "ubuntu-focal-daily-amd64",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can lp-test use the ubuntu-minimal images?

systems:
- ubuntu-24.04:
- ubuntu-22.04
- ubuntu-20.04
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the expected behaviour if we had:

Suggested change
- ubuntu-20.04
- ubuntu-20.04:
image: ubuntu-focal-daily-amd64

@lengau lengau requested review from a team and Copilot December 5, 2025 20:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +222 to 223
# Don't pipe output into stream if spread runs in interactive
if is_interactive:
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on line 222 is now disconnected from the conditional on line 223. Move the comment to line 223 (directly above the if statement) to maintain clarity.

Copilot uses AI. Check for mistakes.
Comment on lines +178 to 179
entry: dict[str, SpreadSystem] = {}
if isinstance(item, str):
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable entry is declared before the isinstance check but only used in the else branch (line 188 onwards). Move the declaration to line 187 (after the continue statement) to scope it closer to its usage.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants