PR For the Charm Review #68

jdkandersson · 2023-06-15T06:52:16Z

This PR is intended to be used for the purpose of the GitHub runner charm review

github-actions · 2023-06-15T06:54:52Z

Test coverage for 89e8919

Name                                    Stmts   Miss Branch BrPart  Cover   Missing
-----------------------------------------------------------------------------------
src/charm.py                              308     55     64     13    81%   254-264, 280-282, 291-292, 299, 310, 321->338, 326-331, 348-349, 353-354, 359-362, 378-379, 413-421, 451-453, 456->exit, 460-462, 485-490, 500-501, 503-504, 506-507, 584-585, 595
src/errors.py                              18      0      0      0   100%
src/event_timer.py                         42      8      4      0    74%   96-99, 119-122
src/github_type.py                         36      0      0      0   100%
src/lxd_type.py                            34      0      2      0   100%
src/repo_policy_compliance_service.py       5      5      0      0     0%   4-14
src/runner.py                             203     32     48     15    81%   137-145, 156-157, 164-171, 177-183, 242, 279-281, 288, 302, 312, 331, 365-370, 380, 464, 497, 523, 528-540, 554, 572
src/runner_manager.py                     195     20     80      8    88%   201-202, 216-222, 226-228, 238-239, 292, 316-320, 339, 357, 471, 491
src/runner_type.py                         46      0     10      0   100%
src/utilities.py                           55      2     12      4    91%   74->76, 78->84, 89-91
-----------------------------------------------------------------------------------
TOTAL                                     942    122    220     40    85%

Static code analysis report

Run started:2023-06-15 06:54:16.189508

Test results:
  No issues identified.

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

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

yanksyoon

Reviewed until src/errors.py :D

README.md

config.yaml

yanksyoon · 2023-06-19T02:25:01Z

requirements.txt

@@ -0,0 +1,8 @@
+ghapi


I'm wondering if it would be worth limiting the versions for the libraries to .?

Are you referring to pinning to versions of the deps?

Minor comments

script/deploy_runner.sh

yanksyoon · 2023-06-19T02:38:51Z

src/charm.py

+        if token is None:
+            token = self.config["token"]
+        if path is None:
+            path = self.config["path"]


Out of curiosity, is this not a charm state because of when this charm was written? :D
Also, I'm thinking if the token, path and service_token validation can be done at a higher level on the controller function so that _get_runner_manager can be purely about getting the RunnerManager.

I agree. You can find an example of a similar situation on this Synapse PR where we have a configuration (server_name) that needs to be set before the deployment too.

This was not a charm state when this charm was written. Also this function was already in this module.
This can be refactored.

Major comments

yanksyoon · 2023-06-19T02:40:53Z

src/charm.py

+            return
+
+        runner_manager = self._get_runner_manager()
+        if runner_manager:


Minor nitpick, I think it would be better if the not case can come earlier so that it can set BlockedStatus and return first to reduce indentation.
i.e.

Suggested change

if runner_manager:

if not runner_manager:

self.unit.status = BlockedStatus(...)

return

... # continue with execution flow

Minor comments

yanksyoon · 2023-06-19T02:42:10Z

src/charm.py

+        except TimerEnableError as ex:
+            logger.exception("Failed to start the event timer")
+            self.unit.status = BlockedStatus(
+                f"Failed to start timer for regular reconciliation and binary update checks: {ex}"
+            )
+


Out of curiosity, if the exception is caught, should it return after setting BlockedStatus?

I'm guessing this is to trigger the next event, execution of the current event should probably proceed anyway. Perhaps the blocked state should say what the operator can/ should do to resolve?

The return should be added.
In terms of the block message, I am not sure what the operator can do there.
Currently, changing the config again to trigger this hook, would retry the installing the timer. However, this does not seem to be a good design.

Maybe an action to manually install the timer, or an automatic retry for this can be added.
Currently, I am thinking not catching the TimerEnableError which causes ErrorStatus and juju would be retrying this event.

Might need to give this more thought.

Major comments

yanksyoon · 2023-06-19T02:43:13Z

src/charm.py

+        if runner_manager:
+            self.unit.status = ActiveStatus()
+        else:
+            self.unit.status = BlockedStatus("Missing token or org/repo path config")


Same comment here about having a not case first and continuing the execution flow.
i.e.

Suggested change

if runner_manager:

self.unit.status = ActiveStatus()

else:

self.unit.status = BlockedStatus("Missing token or org/repo path config")

if not runner_manager:

self.unit.status = BlockedStatus(...)

return

... # continue execution flow

Minor comments

yanksyoon · 2023-06-19T02:49:25Z

src/charm.py

+            event: Event of reconciling the runner state.
+        """
+        if not RunnerManager.runner_bin_path.is_file():
+            logger.warning("Unable to reconcile due to missing runner binary")


Out of curiosity, would there be a reason why this doesn't fall to BlockedStatus?

Since the runner binary is updated/downloaded every hour by default, the charm should be able to resolve missing binary by itself.
So a better status would be Maintenance.
Might be good for the code to set to maintenance status.

Minor comments

src/charm.py

amandahla · 2023-06-19T12:40:29Z

.flake8

@@ -0,0 +1,9 @@
+[flake8]


style: Shouldn't this file be removed? So flake8 will only be configured using pyproject.toml

Minor comments

yanksyoon

Reviewed until runner.py!

src/event_timer.py

yanksyoon · 2023-06-19T09:40:12Z

src/lxd.py

+        """
+        self._pylxd_client = pylxd_client
+
+    def all(self) -> list[LxdInstance]:


Out of curiosity, I'm wondering if there would be a reason to use a mutable list, instead of a tuple?

At least from my perspective and in my humble opinion, usually a list or an iterator is returned. If the user of the class wants to use a tuple, it can be converted.

By the way the pylxd lib uses a list (here)

This interface class might be redesign, so I have put this down as something to take notes of.

Minor comments

yanksyoon · 2023-06-19T10:59:21Z

src/repo_policy_compliance_client.py

+        token: Charm token configured for the repo policy compliance service.
+    """
+
+    def __init__(self, session: requests.Session, url: str, charm_token: str) -> None:


Out of curiosity, why is a session passed in rather than a new requests session within? Since we're passing in a url, i'm assuming that the requests will be made to different url from where the session comes from, and in that case would a new requests session be better in this case?

The session argument can be remove.
Currently, all access to the same host is done in this class. And it is unlikely this will change.

Minor comments

yanksyoon · 2023-06-19T11:01:43Z

src/runner.py

+        busy (bool): Whether GitHub marks this runner as busy.
+    """
+
+    runner_application = Path("/opt/github-runner")


Minor nitpick

Suggested change

runner_application = Path("/opt/github-runner")

runner_application_path = Path("/opt/github-runner")

This is a tough one, there are arguments for going both ways:

If there are no other variables related to this path (e.g., the contents or something like that), adding the _path postfix doesn't add a lot of value

If there are related variables that are defined, then it can be worthwhile to indicate that one hols the path handle, another the content, another the deserialised content etc

minor comment

yanksyoon · 2023-06-19T11:05:53Z

src/runner.py

+        if self.instance is None:
+            raise RunnerError("Runner operation called prior to runner creation.")


I'm wondering if the data flow could be unidirectional, without referring back to self so that we have a controller function and immutable data flow through it.

The complexity of the runner class comes from it is possible to have runner without a LXD instance and not registered on GitHub, runner with LXD instance that is registered on GitHub, runner without a LXD instance and registered to GitHub (the LXD instance might crashed), etc.

It is possible to represent all these states as different type of objects with state pattern, which would avoid assert done in the line 496, 497. This would require a major rewrite so I did not did it.

This can be a future story to explore what pattern is best suited to reduce the complexity.

Major comments

amandahla · 2023-06-19T17:29:47Z

.github/workflows/end_to_end_test.yaml

+    name: end to end test
+    runs-on: [self-hosted, linux, x64, e2e-runner]
+    steps:
+      - name: Echo hello world


~~Is there a reason for this echo here?~~
Would be possible to add a comment explaining that this echo is a connectivity test?

#70 (comment)

Minor comments

README.md

src/charm.py

amandahla · 2023-06-19T17:52:50Z

src/charm.py

+            event: Event of charm upgrade.
+        """
+        logger.info("Reinstalling dependencies...")
+        self._install_deps()


Shouldn't the SubprocessError be handled here like it is on _on_install?

Yes, the error handling is missing.

Minor comments

jdkandersson

up to github_type.py

.copier-answers.yml

jdkandersson · 2023-06-20T05:46:08Z

charmcraft.yaml

+parts:
+  charm:
+    charm-python-packages:
+      - setuptools  # for jinja2


Are all these and below still required?

I remember the ones for cryptography are for pylxd, and the installation would fail without those.
git is needed for installing from git source for pylxd, and repo-policy-compliance.
jinja2 is needed for the templating.
I think cffi is also for pylxd.

However, this was tested a while ago. Reviewing it again might be a good idea.

Minor comments

jdkandersson · 2023-06-20T05:47:22Z

config.yaml

+      runners under a repository.
+  token:
+    type: string
+    default: ""


Should this have a default? It seems that if not provided the charm isn't going to work?

I don't think we could have a default that works? (If that's the case we should mention in the description that this is required (I think we mention that path and token are required elsewhere in docs, but should do here as well).

This token refers to the github personal token, which does not have a default that could work. So this needs to be mentioned as required.

Minor comments

jdkandersson · 2023-06-20T05:49:16Z

pyproject.toml

+]
+
+[tool.coverage.report]
+fail_under = 38


A known issue that this is below our requirements

Major comments

jdkandersson · 2023-06-20T05:52:52Z

script/deploy_runner.sh

I think this is obsoleted by the automated e2e tests?

Yes, this should be removed.

Minor comments

jdkandersson · 2023-06-20T06:11:04Z

src/charm.py

+            event: Event of configuration change.
+        """
+        try:
+            self._event_timer.ensure_event_timer(


I guess this is a way to periodically trigger custom events? Not a bad pattern until perhaps pebble notixes if we can find a better source for these events than cron

@benhoyt any comment?

Yeah, this seems like a nice workaround!

jdkandersson · 2023-06-20T06:12:24Z

src/charm.py

+        except TimerEnableError as ex:
+            logger.exception("Failed to start the event timer")
+            self.unit.status = BlockedStatus(
+                f"Failed to start timer for regular reconciliation and binary update checks: {ex}"
+            )
+


I'm guessing this is to trigger the next event, execution of the current event should probably proceed anyway. Perhaps the blocked state should say what the operator can/ should do to resolve?

jdkandersson · 2023-06-20T06:13:33Z

src/charm.py

+
+        runner_manager = self._get_runner_manager()
+        if not runner_manager:
+            self.unit.status = BlockedStatus("Missing token or org/repo path config")


Can we be more specific? I.e., which one is missing?

Yes, it can be done.

Minor comments

jdkandersson · 2023-06-20T06:14:22Z

src/charm.py

+            event.fail("Missing runner binary")
+            return
+
+        online = 0


Should this be in a separate module?

Minor comments

jdkandersson · 2023-06-20T06:15:28Z

src/charm.py

+            event: Action event of reconciling the runner.
+        """
+        runner_manager = self._get_runner_manager()
+        if not runner_manager:


Perhaps get runner manager can raise an exception instead of returning None which provides the error message? It seems this message is repeated a lot

Perhaps injecting the runner manager can even be a decorator

Minor comments

weiiwang01 · 2023-06-20T07:19:19Z

src/runner.py

+        self.instance.execute(["/usr/bin/who"])
+        self.instance.execute(["/usr/bin/nslookup", "github.com"])


The exit code of the execution is not checked, same for all occurrences of self.instance.execute in the charm.

Major comments

gregory-schiano

Stopped at _on_install for charm.py
Will continue later

gregory-schiano · 2023-06-20T09:27:05Z

.github/workflows/promote_charm.yaml

+
+jobs:
+  promote-charm:
+    uses: canonical/operator-workflows/.github/workflows/promote_charm.yaml@promote-charm-base


Shouldn't we use @main ?

Minor comments

README.md

metadata.yaml

gregory-schiano · 2023-06-20T10:08:33Z

requirements.txt

I think we should pin all dependencies

Minor comments

src/charm.py

gregory-schiano · 2023-06-20T10:18:27Z

src/charm.py

+class GithubRunnerCharm(CharmBase):
+    """Charm for managing GitHub self-hosted runners."""
+
+    _stored = StoredState()


The fact that we need to get away from it should be the main comment here

gregory-schiano · 2023-06-20T10:21:57Z

src/charm.py

+        self.framework.observe(self.on.check_runners_action, self._on_check_runners_action)
+        self.framework.observe(self.on.reconcile_runners_action, self._on_reconcile_runners_action)
+        self.framework.observe(self.on.flush_runners_action, self._on_flush_runners_action)
+        self.framework.observe(self.on.update_runner_bin_action, self._on_update_runner_bin)


It's sharing the same hook as "update_runner_bin" event
Each event should have a dedicated hook and eventually call the same private method

amandahla · 2023-06-20T14:27:08Z

.github/workflows/end_to_end_test.yaml

+      - name: Deploy nginx for testing
+        run: sudo microk8s kubectl create deployment nginx --image=nginx
+      - name: Wait for nginx to be ready
+        run: sudo microk8s kubectl rollout status deployment/nginx --timeout=30m


Isn't 30m too much? Maybe 5m?

Minor comments

amandahla · 2023-06-20T14:28:30Z

.gitignore

@@ -0,0 +1,7 @@
+.tox/


I miss a standard for this file on our charms. What do you think about using https://github.com/github/gitignore/blob/main/Python.gitignore as a base?

Minor comments

README.md

amandahla · 2023-06-20T14:32:14Z

docs/how-to-guides/contribute.md

+charmcraft pack
+# Ensure you're connected to a juju k8s model
+# Configure the machine resource created by the model
+juju set-model-constraints mem=8G cores=2 root-disk=50G


Are the constraints a requirement or suggestion?

In a sense they are a requirement as the default constraints would not have enough resource.
The memory would need to be increased and root-disk decreased, once we settle on using RAM as disk for the runners.

Minor comments

amandahla · 2023-06-20T14:50:41Z

src/errors.py

@@ -0,0 +1,72 @@
+# Copyright 2023 Canonical Ltd.


style: Flask named this file as exceptions.py. What's the standard? Related canonical/jenkins-k8s-operator#9 (comment)

exceptions.py is probably a better name.

Minor comments

src/event_timer.py

amandahla · 2023-06-20T14:59:10Z

src/event_timer.py

+from jinja2 import Environment, FileSystemLoader
+
+
+class TimerEnableError(Exception):


Shouldn't this be in errors.py?

Minor comments

arturo-seijas · 2023-06-20T15:39:59Z

.github/workflows/end_to_end_test.yaml

+        run: touch /usr/local/bin/test_file
+      # "Install microk8s" step will test if the proxies settings are correct.
+      - name: Proxy set in /etc/environment
+        run: cat /etc/environment | grep HTTP_PROXY


What's the purpose of this cat /etc/environment | grep HTTP_PROXY ? Debugging? If so, how about of the rest of the proxy settings?

It is to test whether the /etc/environment file was successful written to.

Minor comments

arturo-seijas · 2023-06-20T15:41:30Z

.github/workflows/end_to_end_test.yaml

+      # "Update apt in python docker container" step will test docker client default proxy settings.
+      - name: Proxy set in docker client
+        run: cat /home/ubuntu/.docker/config.json | grep httpProxy
+      - name: Install microk8s


Can we merge all the microk8s steps in ones?

Which steps are you thinking about merging together?

Minor comments

arturo-seijas · 2023-06-20T15:42:28Z

.github/workflows/end_to_end_test.yaml

+    needs: e2e-test
+    runs-on: [self-hosted, linux, x64, e2e-runner]
+    steps:
+      - name: Docker version


Can we merge all these steps together? It will make debugging easier

arturo-seijas · 2023-06-20T15:45:00Z

README.md

+
+This means that each interval, each unit will make one or more API calls to GitHub. The interval may need to be adjusted if the number of units is large enough to trigger [Rate Limiting](https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting).
+
+## Development


Isn't this too much detail? There's already a how to contribute doc

Minor comments

README.md

config.yaml

arturo-seijas · 2023-06-20T15:49:35Z

docs/how-to-guides/contribute.md

+* All enhancements require review before being merged. Code review typically examines
+  * code quality
+  * test coverage
+  * user experience for Juju administrators of this charm.


s/administrators/operators?

Minor comments

arturo-seijas · 2023-06-20T15:50:13Z

docs/index.md

@@ -0,0 +1,5 @@
+GitHub self-hosted runner offer a way to run GitHub action workloads on non-GitHub servers.


s/offer/offers

Minor comments

src/event_timer.py

amandahla · 2023-06-21T14:05:42Z

src/lxd.py

+        Args:
+            dir: Name of the directory to create.
+        """
+        self.instance.execute(["/usr/bin/mkdir", "-p", dir_name])


No error check?

Minor comments

amandahla · 2023-06-21T14:08:11Z

src/lxd.py

+            LxdException: Unable to load the file into the LXD instance.
+        """
+        lxc_cmd = [
+            "/snap/bin/lxc",


Could "/snap/bin/lxc" be a constant?

Minor comments

amandahla · 2023-06-21T14:17:28Z

src/lxd.py

+            LxdException: Unable to load the file to the LXD instance.
+        """
+        if isinstance(content, str):
+            content = content.encode("utf-8")


Handle UnicodeError or add errors = ignore?

Minor comments

amandahla · 2023-06-21T14:19:06Z

src/lxd.py

+        with tempfile.NamedTemporaryFile() as file:
+            self.pull_file(filepath, file.name)
+
+            return file.read().decode("utf-8")


Same comment about handling errors here.

Minor comments

src/lxd.py

amandahla · 2023-06-21T14:25:25Z

src/lxd.py

+        """
+        self._pylxd_client = pylxd_client
+
+    def all(self) -> list[LxdInstance]:


Maybe I'm missing something but is this only used for the tests to get the number of instances? If this is the case, why not change to return this number instead of the entire list?

Not specifically related to this, addressing the question/ comment regarding list vs tuple. The order of preference for return values depending on what is possible:

A generator, the caller can decide what kind of data structure to use

A tuple, passing around lists can be dangerous since anyone can modify them accidentally, e.g., in a subroutine which can't happen with a tuple

A list

Maybe I'm missing something but is this only used for the tests to get the number of instances? If this is the case, why not change to return this number instead of the entire list?

I indeed only found references in test_runner and it seems it's systematically using a mocked object. So if that's the only use case we could probably find another way around

This class original was created to replace the pylxd with the load file function changed. Hence, the interface is exactly like pylxd.
Now a interface to represent the virtualization layer will be created, this old interface will be removed.
No action needed.

amandahla · 2023-06-21T14:27:17Z

src/lxd.py

+        """
+        try:
+            pylxd_instance = self._pylxd_client.instances.create(config=config, wait=wait)
+            return LxdInstance(config["name"], pylxd_instance)


Is it possible that "name" key does not exist for some reason?

Currently, it is not possible as the name is passed in on Runner class creation.
But checking can be added to the __init__ method to ensure a valid name is set.

Minor comments

src/lxd_type.py

src/utilities.py

amandahla · 2023-06-21T14:57:54Z

Stopped on utilities.py. I'll review the tests tomorrow.

nrobinaubertin

Didn't review everything

.github/workflows/end_to_end_test.yaml

README.md

nrobinaubertin · 2023-06-22T12:26:08Z

docs/how-to-guides/contribute.md

+# Assuming you're on amd64
+juju deploy ./github-runner_ubuntu-22.04-amd64_ubuntu-20.04-amd64.charm
+```
+


For the PAT tokens, just linking to the doc should be enough

templates/start.j2

tests/integration/conftest.py

amandahla · 2023-06-22T13:57:40Z

tests/integration/test_charm.py

+
+
+@pytest.mark.abort_on_fail
+async def test_build_and_deploy(ops_test):


Usually this is a fixture like here in Flask.

This is in the progress of being change as a part of add integration tests.

amandahla · 2023-06-22T14:01:30Z

tests/unit/mock.py

+        self.deleted = True
+
+    def execute(self, cmd: Sequence[str], cwd: Optional[str] = None) -> tuple[int, str, str]:
+        return 0, "", ""


Couldn't exit code be a parameter to test the behavior in case of error?

The current method in the tests is to overwrite the method with a different stub method.

amandahla · 2023-06-22T14:05:51Z

tests/unit/test_charm.py

+    @patch("charm.RunnerManager")
+    @patch("pathlib.Path.write_text")
+    @patch("subprocess.run")
+    def test_org_register(self, run, wt, rm):


nitpick: it took me some time to get that rm is runner manager

This test file is heavy out of date and is still using unittest. This will be updated in the migrate to pytest story.

amandahla · 2023-06-22T14:43:12Z

tests/unit/test_charm.py

+
+    @patch("pathlib.Path.write_text")
+    @patch("subprocess.run")
+    def test_install(self, run, wt):


For all the tests: I missed the docstrings as described in the contributing guide.

Same as above

amandahla · 2023-06-22T14:43:55Z

tests/unit/test_charm.py

+            ),
+            proxies={},
+        )
+        mock_rm.reconcile.assert_called_with(0, VirtualMachineResources(2, "7GiB", "10GiB")),


This assert is because of the default values, right?

amandahla · 2023-06-22T14:49:30Z

tests/unit/test_charm.py

+
+        harness.charm._reconcile_runners = raise_error
+        harness.charm.on.install.emit()
+        assert harness.charm.unit.status == BlockedStatus("mock error")


What kind of error is expected?

Reconcile runners can have various type of errors such as LXD error, hardware related errors (out of memory). This test checks if these unexpected errors, sets the charm into BlockedStatus.

amandahla · 2023-06-22T14:51:41Z

tests/unit/test_runner.py

+    if request.param[0] is None:
+        return None
+
+    attrs = {"status": request.param[0], "execute.return_value": (0, "", "")}


Same comment here about the possibility of exit_code being a parameter to test execution error case.

Minor comments

amandahla · 2023-06-22T14:58:51Z

tox.ini

+    bandit[toml]
+    -r{toxinidir}/requirements.txt
+commands =
+    bandit -c {toxinidir}/pyproject.toml -r {[vars]src_path}


amandahla · 2023-06-22T14:59:59Z

Finished 👍

src/github_type.py

jdkandersson · 2023-06-23T05:16:26Z

src/lxd.py

+        """
+        self._pylxd_client = pylxd_client
+
+    def all(self) -> list[LxdInstance]:


Not specifically related to this, addressing the question/ comment regarding list vs tuple. The order of preference for return values depending on what is possible:

A generator, the caller can decide what kind of data structure to use

A tuple, passing around lists can be dangerous since anyone can modify them accidentally, e.g., in a subroutine which can't happen with a tuple

A list

jdkandersson · 2023-06-23T05:21:38Z

src/lxd.py

Broadly speaking, this class feels like it is a bit specific to LXD. It would be good to think about the virtualisation management interface the charm requires without thinking about LXD too much and make this module that interface

There are also some virtualisation specific things in the other parts of the code (like in charm.py) that would be good to move to be a part of the interface

Well I feel like the API is generic enough, the naming of the class are specific and should be renamed, we can easily derive an interface from the methods
Only the ProfileManager seems pretty LXD specific

Ah yes and the exception raised also should be generic

Major comments

jdkandersson · 2023-06-23T05:28:17Z

src/runner.py

+        busy (bool): Whether GitHub marks this runner as busy.
+    """
+
+    runner_application = Path("/opt/github-runner")


This is a tough one, there are arguments for going both ways:

If there are no other variables related to this path (e.g., the contents or something like that), adding the _path postfix doesn't add a lot of value

If there are related variables that are defined, then it can be worthwhile to indicate that one hols the path handle, another the content, another the deserialised content etc

jdkandersson · 2023-06-23T05:32:18Z

src/runner.py

This is one of the key files for the runners, it would be good to write an interface definition to make it easier to understand it

Also for runner_manager.py

jdkandersson · 2023-06-23T05:43:02Z

src/utilities.py

+                try:
+                    return func(*args, **kwargs)
+                # Error caught is set by the input of the function.
+                except exception as err:  # pylint: disable=broad-exception-caught


It would be good to take this as an argument so that we're not handling arbitrary exceptions

Currently, the exception is an argument, but it is defaulting to the Exception class.
It will take a major effort to find all exceptions to retry on for every instances of usage.
Also the exception argument would need to be change to a tuple of Exceptions rather than a single type of exception.

Major comments

src/utilities.py

gregory-schiano · 2023-06-23T09:39:43Z

src/event_timer.py

+            TimerDisableError: Timer cannot be stopped. Events will be emitted continuously.
+        """
+        try:
+            # Don't check for errors in case the timer wasn't registered.


Is it worth it throwing an exception when you try to disable something already disabled ?

gregory-schiano · 2023-06-23T10:34:49Z

src/event_timer.py

I'm wondering if we really should use this "trick" to trigger regularly the charm
I know there's no good way to do it, but this seems to bring some complication here using systemd

Major comments

gregory-schiano · 2023-06-23T14:00:50Z

src/lxd.py

Well I feel like the API is generic enough, the naming of the class are specific and should be renamed, we can easily derive an interface from the methods
Only the ProfileManager seems pretty LXD specific

gregory-schiano · 2023-06-23T14:01:50Z

src/lxd.py

Ah yes and the exception raised also should be generic

gregory-schiano · 2023-06-23T14:08:36Z

src/lxd.py

+        """
+        self._pylxd_client = pylxd_client
+
+    def all(self) -> list[LxdInstance]:


Maybe I'm missing something but is this only used for the tests to get the number of instances? If this is the case, why not change to return this number instead of the entire list?

I indeed only found references in test_runner and it seems it's systematically using a mocked object. So if that's the only use case we could probably find another way around

src/lxd_type.py

gregory-schiano · 2023-06-23T15:25:46Z

src/charm.py

+
+        self.service_token = None
+
+        self.on.define_event("reconcile_runners", ReconcileRunnersEvent)


These 2 events are the one triggered by the timer service which is using SystemD scheduling to call the dispatch script from a juju run command

yhaliaw · 2023-06-30T08:54:18Z

README.md

+* `token` is a [GitHub Personal Access Token (PAT)](https://github.com/settings/tokens) (note: this is not the same as the token given in the Add a Runner instructions). The PAT token requires either:
+  * the **`repo`** ("Full control of private repositories") permission for
+use with repositories or;
+  * both the **`repo`** and **`admin:org`** ("Full control of orgs and teams, read and write org projects") permissions for use with an organization. This is necessary because the charm will create and remove runners as needed to ensure that each runner executes only one job to protect jobs from leaking information to other jobs running on the same runner.


The fine-grained token are in beta right now, but it might be good to include the instruction for fine-graned tokens.
https://docs.github.com/en/rest/overview/permissions-required-for-fine-grained-personal-access-tokens?apiVersion=2022-11-28

Minor comments

merkata · 2024-07-10T07:56:26Z

@jdkandersson can this be closed?

copy from main

89e8919

yanksyoon reviewed Jun 19, 2023

View reviewed changes

amandahla reviewed Jun 19, 2023

View reviewed changes

yanksyoon reviewed Jun 19, 2023

View reviewed changes

amandahla reviewed Jun 19, 2023

View reviewed changes

README.md Show resolved Hide resolved

amandahla reviewed Jun 19, 2023

View reviewed changes

src/charm.py Show resolved Hide resolved

amandahla reviewed Jun 19, 2023

View reviewed changes

jdkandersson commented Jun 20, 2023

View reviewed changes

weiiwang01 reviewed Jun 20, 2023

View reviewed changes

gregory-schiano reviewed Jun 20, 2023

View reviewed changes

amandahla reviewed Jun 20, 2023

View reviewed changes

README.md Show resolved Hide resolved

amandahla reviewed Jun 20, 2023

View reviewed changes

src/event_timer.py Show resolved Hide resolved

amandahla reviewed Jun 20, 2023

View reviewed changes

arturo-seijas reviewed Jun 20, 2023

View reviewed changes

README.md Show resolved Hide resolved

arturo-seijas reviewed Jun 20, 2023

View reviewed changes

config.yaml Show resolved Hide resolved

arturo-seijas reviewed Jun 20, 2023

View reviewed changes

config.yaml Show resolved Hide resolved

arturo-seijas reviewed Jun 20, 2023

View reviewed changes

src/event_timer.py Show resolved Hide resolved

arturo-seijas reviewed Jun 20, 2023

View reviewed changes

src/event_timer.py Show resolved Hide resolved