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

Diagrams #439

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

Conversation

javierdelapuente
Copy link
Collaborator

@javierdelapuente javierdelapuente commented Jan 29, 2025

Applicable spec:

Overview

C4 Diagrams for github-runner operator.

Rationale

Juju Events Changes

Module Changes

Library Changes

Checklist

@javierdelapuente javierdelapuente added the documentation Improvements or additions to documentation label Jan 29, 2025
@canonical canonical deleted a comment from github-actions bot Jan 29, 2025
@javierdelapuente javierdelapuente marked this pull request as ready for review January 29, 2025 15:35
Copy link
Contributor

Test coverage for 697fbcc

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:2025-01-29 15:36:41.156924

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):

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 adding to the docs! I have a few comments to start with, mostly just to better understand the diagrams.

@@ -11,6 +11,104 @@ Conceptually, the charm can be divided into the following:
- Management of [Python web service for checking GitHub repository settings](https://github.com/canonical/repo-policy-compliance)
- Management of dependencies


Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a header here, something like ## Overview of the GitHub runner ecosystem. Feel free to change the wording -- I think the heading will set the expectation that this diagram and section will present a high-level overview of the project.

Other options include moving this diagram to the main index.md page, the README, or both. I'd appreciate your thoughts about where is the best place for this diagram!

Comment on lines +103 to +105
The `RunnerScaler` is the main component to reconcile the desired number of runners using the `RunnerManager`.
The `RunnerManager` uses the `CloudRunnerManager` to interact with the compute infrastructure to create and manage self-hosted runner (OpenStack is currently the only implementation).
The `RunnerManager` uses the `GithubRunnerManager` to interact with the GitHub API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on this description, my impression that the RunnerManager is the main component of the charm. The RunnerManager seems to coordinate all the other components to create, manage, and scale the runners. Please correct me if my impression is wrong!

However, if this is a correct interpretation, then I would rephrase the text to say something like

The `RunnerManager` is the main component of the charm. The `RunnerManager` interacts
with the other charm components in the following ways:
* `RunnerScaler`: To reconcile the desired number of runners.
* `CloudRunnerManager`: To interact with the compute infrastructure to create and manage
  self-hosted runners. OpenStack is currently the only available cloud implementation. 
* `GithubRunnerManager`: To interact with the GitHub API.

The `RunnerManager` uses the `GithubRunnerManager` to interact with the GitHub API.

In the case of reactive runners, the `RunnerManager` will also create processes that
will be in charge of consuming events that were created from Github webhooks, and starting GitHub runners in a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
will be in charge of consuming events that were created from Github webhooks, and starting GitHub runners in a
will be in charge of consuming events that were created from GitHub webhooks and starting GitHub runners in a

In the case of reactive runners, the `RunnerManager` will also create processes that
will be in charge of consuming events that were created from Github webhooks, and starting GitHub runners in a
reactive manner. Those events are stored in `mongodb` and were enqueued by
the application [github-runner-webhook-router](https://github.com/canonical/github-runner-webhook-router).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is github-runner-webhook-router a charm? It may be better to refer to it as a charm here, let me know what you think

Comment on lines +66 to +101
```mermaid

C4Container
title Container diagram for the github-runner Charm System

System_Ext(osrunnign, "OpenStack", "OpenStack deployment used for runners")
System_Ext(github, "GitHub", "GitHub API")

Container_Boundary(c1, "GitHub Runner Charm"){
Component(runnerscaler, "RunnerScaler", "", "")


Component(runnermanager, "RunnerManager", "", "")
Component(githubrunnermanager, "GitHubRunnerManager", "", "")

Component(cloudrunnermanager, "CloudRunnerManager", "", "")
Component(openstackrunnermanager, "OpenstackRunnerManager", "", "")

Rel(runnerscaler, runnermanager, "uses", "")
Rel(runnermanager, cloudrunnermanager, "uses", "")
Rel(runnermanager, githubrunnermanager, "uses", "")
Rel(openstackrunnermanager, cloudrunnermanager, "implements", "")
}

Container_Boundary(c2, "Reactive Processes"){
Component(runnerprocess, "Reactive Spawner", "", "")
}

Rel(githubrunnermanager, github, "manages VMs", "")
Rel(openstackrunnermanager, osrunnign, "manages VMs", "")

Rel(runnermanager, runnerprocess, "creates/deleted processes", "")

Rel(runnerprocess, github, "manages VMs", "")
Rel(runnerprocess, osrunnign, "manages VMs", "")
```
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding, are these different components (RunnerManager, RunnerScaler, etc.) components of the charm, or the software components? (If there's a difference between those two categories)

The aesthetics of this diagram are a little wonky. If the different components and arrows refuse to cooperate, feel free to change to a different Mermaid syntax.

@javierdelapuente
Copy link
Collaborator Author

Clarifiy that it is a machine charm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Libraries: Out of sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants