Skip to content
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

Clean lxd code #423

Open
wants to merge 75 commits into
base: main
Choose a base branch
from
Open

Clean lxd code #423

wants to merge 75 commits into from

Conversation

javierdelapuente
Copy link
Collaborator

@javierdelapuente javierdelapuente commented Dec 18, 2024

Applicable spec:

Overview

This PR removed LXD runners from the main branch. The reason for this is that there were really two different code paths without a common interface in the charm.

LXD runners will still be supported in the track local-lxd (see announcement) https://discourse.charmhub.io/t/important-update-new-track-local-lxd-for-lxd-runners-in-the-github-runner-charm/16139.

This PR has also strong implications for documentation, as lot of information is now outdated. As a first step it has been removed, but some of it, like a tutorial, will have to be created in following PRs.

Rationale

Maintaining two codepaths inhibits big refactors and also is a risk for regression, as we are concentrating our development in openstack runners and we can break lxd. Having two different tracks brings clarity and reduces risks and improves maintanability.

If in the future lxd support is required in the main branch, it could be better added using a common interface for both types of runners, using an external lxc cluster (or microcloud cluster).

Juju Events Changes

Module Changes

Library Changes

Checklist

  • The charm style guide was applied.
  • The contributing guide was applied.
  • The changes are compliant with ISD054 - Managing Charm Complexity
  • The documentation is generated using src-docs.
  • The documentation for charmhub is updated.
  • The PR is tagged with appropriate label (urgent, trivial, complex).
  • The changelog is updated with changes that affects the users of the charm.
  • The changes do not introduce any regression in code or tests related to LXD runner mode.

Copy link
Contributor

@erinecon erinecon left a comment

Choose a reason for hiding this comment

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

Thanks for restructuring the docs! I took this opportunity to do a more careful review to try and switch the docs to using US English.

docs/index.md Outdated Show resolved Hide resolved
docs/local-lxd/explanation/charm-architecture.md Outdated Show resolved Hide resolved
docs/local-lxd/explanation/charm-architecture.md Outdated Show resolved Hide resolved
docs/local-lxd/explanation/charm-architecture.md Outdated Show resolved Hide resolved
docs/local-lxd/explanation/charm-architecture.md Outdated Show resolved Hide resolved
docs/local-lxd/reference/integrations.md Outdated Show resolved Hide resolved
docs/local-lxd/reference/token-scopes.md Outdated Show resolved Hide resolved
docs/local-lxd/reference/token-scopes.md Outdated Show resolved Hide resolved
docs/local-lxd/reference/token-scopes.md Outdated Show resolved Hide resolved
docs/local-lxd/reference/token-scopes.md Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
# GitHub Runner Operator

A [Juju](https://juju.is/) [charm](https://juju.is/docs/olm/charmed-operators) for deploying and managing [GitHub self-hosted runners](https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners) on virtual machines. The charm maintains a set of ephemeral self-hosted runners, each isolated in a single-use virtual machine instance.
A [Juju](https://juju.is/) [charm](https://juju.is/docs/olm/charmed-operators) for deploying and managing [GitHub self-hosted runners](https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners) on virtual machines. The charm maintains a set of self-hosted runners, each isolated in a single-use virtual machine instance.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the runners are still ephemeral. i.e., the runner disappear after one job.

Although the "each isolated in a single-use virtual machine instance" part also points that out.

I am fine either way.

Copy link
Collaborator

@cbartz cbartz left a comment

Choose a reason for hiding this comment

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

Thank you Javi!

| [Overview](https://charmhub.io/github-runner)</br> Overview of the charm </br> | [How-to guides](https://charmhub.io/github-runner/docs/how-to-openstack-runner) </br> Step-by-step guides covering key operations and common tasks |
| [Reference](https://charmhub.io/github-runner/docs/reference-actions) </br> Technical information - specifications, APIs, architecture | [Explanation](https://charmhub.io/github-runner/docs/explanation-charm-architecture) </br> Concepts - discussion and clarification of key topics |

If you want to use ephemeral LXD virtual machines spawned by charm, you can refer to the section [Track local-lxd](https://charmhub.io/github-runner/docs/local-lxd).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If you want to use ephemeral LXD virtual machines spawned by charm, you can refer to the section [Track local-lxd](https://charmhub.io/github-runner/docs/local-lxd).
If you want to use ephemeral LXD virtual machines spawned by the charm, you can refer to the section [Track local-lxd](https://charmhub.io/github-runner/docs/local-lxd).

@@ -726,7 +670,7 @@ async def basic_app_fixture(request: pytest.FixtureRequest) -> Application:


@pytest_asyncio.fixture(scope="function", name="instance_helper")
async def instance_helper_fixture(request: pytest.FixtureRequest) -> InstanceHelper:
async def instance_helper_fixture(request: pytest.FixtureRequest) -> OpenStackInstanceHelper:
Copy link
Collaborator

Choose a reason for hiding this comment

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

the dynamic fixture request is no longer required

Copy link
Contributor

Test coverage for 02be48c

Name                 Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------
src/charm.py           329     78     46      9    72%   266-268, 286-292, 307-308, 327-328, 343, 345, 347, 353-356, 380, 393-395, 417-418, 430-431, 454-455, 465, 514-516, 521-530, 586-619, 633-638, 653-696, 704-705, 727
src/charm_state.py     336     16     62      4    93%   220-232, 432-436, 519-520, 822->exit, 825->828, 832-833, 870-871
src/errors.py           13      0      0      0   100%
src/event_timer.py      52      6      0      0    88%   105-106, 143-144, 160-161
src/logrotate.py        43      0      2      0   100%
src/utilities.py        25      3      4      1    79%   66-69
----------------------------------------------------------------
TOTAL                  798    103    114     14    84%

Static code analysis report

Run started:2024-12-21 11:44:06.660889

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1602
  Total lines skipped (#nosec): 2
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 3

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

source: scripts
organize:
build-lxd-image.sh: scripts/build-lxd-image.sh
reactive_runner.py: scripts/reactive_runner.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

The source docs can be deleted now, we will figure out an alternative

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants