refactor(garm): move workload config to entrypoint - #304
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the garm charm/workload boundary so that GARM’s file-based configuration (config.toml + provider files) is rendered inside the container entrypoint from environment variables, aligning the workload more closely with the paas-charm 12-factor model.
Changes:
- Added a Python-based GARM entrypoint (
garm_entrypoint.py) plus dedicated unit tests for config rendering and execution behavior. - Updated the
garmcharm to pass workload configuration via environment variables and to point the Pebble service at the entrypoint via a customAppwrapper. - Updated the GARM rockcraft definition to include Python dependencies and install the entrypoint script, and adjusted related unit tests and ignore rules.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
garm-rockcraft.yaml |
Adds Python dependencies + installs the entrypoint script; modifies the base Pebble layer command/startup behavior. |
charms/garm/uv.lock |
Updates the lockfile’s declared requires-python constraint. |
charms/garm/tests/unit/test_garm_entrypoint.py |
New unit tests covering entrypoint config rendering, path safety, and exec behavior. |
charms/garm/tests/unit/test_charm.py |
Updates Scenario-based tests to validate env-driven configuration and the new service naming/command behavior. |
charms/garm/src/garm_entrypoint.py |
New entrypoint implementation: reads env, renders TOML/YAML files, and execs the GARM binary. |
charms/garm/src/charm.py |
Refactors charm-side config handling to env-only, introduces GarmApp, and updates restart/config logic. |
.gitignore |
Ignores virtualenv and additional charm/terraform generated directories. |
Suppressed comments (2)
charms/garm/src/charm.py:193
GarmApp.restart()acceptsrerun_migrationsbut discards it by callingsuper().restart()without passing the flag. This can prevent the charm’srerun_migrationspath from working as intended.
class GarmCharm(paas_charm.go.Charm):
"""GARM charm — manages the GARM service via Pebble."""
def __init__(self, *args: typing.Any) -> None:
"""Initialize the charm.
charms/garm/src/charm.py:334
- The
_compute_statedocstring still claims it returns provider configuration files to push, but the refactor now passes provider config viaGARM_PROVIDERS_JSONand the entrypoint owns file rendering. This makes the docstring misleading.
Returns a ``ComputeState`` containing the environment variables the
GARM entrypoint needs and the provider configuration files to push.
When paas-charm gains native ``_compute_state`` support, this data
gathering can be returned directly and the ``GarmApp`` wrapper
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
charms/garm/src/garm_entrypoint.py:127
GARM_PROVIDERS_JSONis decoded withjson.loads(...)without handlingJSONDecodeErroror validating the decoded type. If the env var is set but malformed (or not a JSON list), the entrypoint will crash with a traceback or behave unexpectedly.
providers_json = env.get("GARM_PROVIDERS_JSON", "")
providers = json.loads(providers_json) if providers_json else []
charms/garm/src/garm_entrypoint.py:83
POSTGRESQL_DB_PORTis parsed withint(...)without validation; a malformed value will raiseValueErrorand crash the entrypoint with a traceback. It’s better to raise a clearValueErrorthatmain()can log and exit on.
This issue also appears on line 126 of the same file.
db_port = int(env.get("POSTGRESQL_DB_PORT", DEFAULT_DB_PORT))
db_name = env.get("POSTGRESQL_DB_NAME", DEFAULT_DB_NAME)
…m:canonical/github-runner-operators into chore/refactor-reconcile-isd-6013
yhaliaw
left a comment
There was a problem hiding this comment.
Major changes needed
🤝 Human review with AI assistance.
Summary
Test plan
tox -c tox.toml -e static— passed.tox -c tox.toml -e fmt— passed.ruff checkandruff format --checkon changed production files — passed.tests/unit/test_garm_entrypoint.py— 7 passed.Note: the full unit environment is currently blocked during collection because
tests/unit/test_garm_toml.pystill imports config-rendering helpers removed by this refactor (_build_provider_list,_proxy_environment, andrender_garm_toml).