Skip to content

Commit

Permalink
feat: validate ops.main() call for operator framework charms
Browse files Browse the repository at this point in the history
  • Loading branch information
dimaqq committed Sep 20, 2024
1 parent f7a8be3 commit 8c87a50
Show file tree
Hide file tree
Showing 14 changed files with 364 additions and 0 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ dmypy.json
*~
/charmcraft/_version.py
/results/
.*.swp

# Spread files
.spread-reuse*.yaml
95 changes: 95 additions & 0 deletions charmcraft/linters.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import ast
import os
import pathlib
import re
import shlex
import typing
from collections.abc import Generator
Expand Down Expand Up @@ -523,6 +524,99 @@ def run(self, basedir: pathlib.Path) -> str:
return self.Result.OK


class OpsMainCall(Linter):
"""Check that the entrypoint contains call to ops.main()."""

name = "ops-main-call"
url = f"{BASE_DOCS_URL}#heading--ops-main-call"
text = ""

def run(self, basedir: pathlib.Path) -> str:
"""Check preconditions and validate there's an ops.main() call."""
if Framework().run(basedir) != Framework.Result.OPERATOR:
self.text = "The charm is not based on the operator framework"
return self.Result.NONAPPLICABLE

entrypoint = get_entrypoint_from_dispatch(basedir)
if entrypoint is None:
self.text = "Cannot find a proper 'dispatch' script pointing to an entrypoint."
return self.Result.NONAPPLICABLE

if not entrypoint.exists():
self.text = f"Cannot find the entrypoint file: {str(entrypoint)!r}"
return self.Result.NONAPPLICABLE

if not self._check_main_calls(entrypoint.read_text()):
self.text = f"The ops.main() call missing from {str(entrypoint)!r}."
return self.Result.ERROR

return self.Result.OK

def _check_main_calls(self, code: str):
tree = ast.parse(code)
imports = self._ops_main_imports(tree)
return self._detect_main_calls(tree, imports=imports)

def _ops_main_imports(self, tree: ast.AST) -> dict[str, str]:
"""Analyze imports and return a mapping {local_name: imported thing}."""
rv = {}

class ImportVisitor(ast.NodeVisitor):
def visit_Import(self, node: ast.Import): # noqa: N802
for alias in node.names:
# Detect statements like `import ops`
if alias.name == "ops":
rv[alias.asname or alias.name] = "ops"
if alias.name == "ops.main" and alias.asname:
rv[alias.asname] = "ops.main"
elif alias.name.startswith("ops.") and not alias.asname:
rv["ops"] = "ops"

def visit_ImportFrom(self, node: ast.ImportFrom): # noqa: N802
for alias in node.names:
# Detect statements like `from ops import main [as ops_main]`
if node.module in ("ops", "ops.main") and alias.name == "main":
rv[alias.asname or alias.name] = f"{node.module}.main"

ImportVisitor().visit(tree)
return rv

def _detect_main_calls(self, tree: ast.AST, *, imports: dict[str, str]) -> bool:
main_call_sites = []

class OpsMainFinder(ast.NodeVisitor):
def visit_Call(self, node: ast.Call): # noqa: N802
match node.func:
# Matches statements like `ops.main.main(...)`
case ast.Attribute(
value=ast.Attribute(value=ast.Name(id=first), attr=second),
attr=third,
):
call_site = f"{first}.{second}.{third}(...)"
# Matches statements like `ops.main(...)`
case ast.Attribute(value=ast.Name(id=first), attr=second):
call_site = f"{first}.{second}(...)"
# Matches statements like `main(...)`
case ast.Name(id=first):
call_site = f"{first}(...)"
case _:
call_site = "_dummy()"

match = re.match(r"^([a-zA-Z_][a-zA-Z0-9_]*)(.*)", call_site)
if not match:
raise ValueError("impossible")
alias, rest = match.groups()
resolved = f"{imports.get(alias, '_dummy')}{rest}"

if resolved in ("ops.main(...)", "ops.main.main(...)"):
main_call_sites.append(call_site)

self.generic_visit(node)

OpsMainFinder().visit(tree)
return any(main_call_sites)


class AdditionalFiles(Linter):
"""Check that the charm does not contain any additional files in the prime directory.
Expand Down Expand Up @@ -584,5 +678,6 @@ def run(self, basedir: pathlib.Path) -> str:
NamingConventions,
Framework,
Entrypoint,
OpsMainCall,
AdditionalFiles,
]
38 changes: 38 additions & 0 deletions tests/integration/commands/test_analyse.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
#
# For further info, check https://github.com/canonical/charmcraft


import json
import sys
import zipfile
from argparse import ArgumentParser, Namespace
from pathlib import Path

import pytest
from craft_cli import CraftError
Expand Down Expand Up @@ -368,3 +370,39 @@ def test_only_fatal(emitter, service_factory, config, monkeypatch, fake_project_

emitter.assert_progress("check-lint: [FATAL] text (url)", permanent=True)
assert retcode == 1


def zip_directory(directory_path: Path, zip_path: Path):
"""Directory to zip with contents and permissions."""
with zipfile.ZipFile(str(zip_path), "w", zipfile.ZIP_DEFLATED) as zipf:
for file_path in directory_path.rglob("*"):
rel_path = file_path.relative_to(directory_path)
zip_info = zipfile.ZipInfo(str(rel_path))
zip_info.external_attr = (file_path.stat().st_mode & 0o777) << 16

if file_path.is_dir():
zip_info.filename += "/"
zipf.writestr(zip_info, "")
else:
zipf.writestr(zip_info, file_path.read_bytes())


@pytest.fixture
def linter_charms(request):
return request.config.rootpath / "tests/integration/ops-main-linter-charms"


@pytest.mark.parametrize(("charm", "rv"), [("smoke", 0), ("negative", 2)])
def test_ops_main_linter(
tmp_path: Path, linter_charms: Path, emitter, config, charm: str, rv: int
):
zip_directory(linter_charms / charm, (charm_path := tmp_path / "this.charm"))

retcode = Analyse(config=config).run(
Namespace(filepath=charm_path, force=None, format=None, ignore=None)
)

assert retcode == rv

if rv:
assert "ops.main() call missing" in str(emitter.interactions)
16 changes: 16 additions & 0 deletions tests/integration/ops-main-linter-charms/negative/charmcraft.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
name: smoke
type: charm
title: n/a
summary: n/a
description: n/a

base: ubuntu@24.04
build-base: ubuntu@24.04
platforms:
arm64:

parts:
charm:
plugin: charm
source: .
4 changes: 4 additions & 0 deletions tests/integration/ops-main-linter-charms/negative/dispatch
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#!/bin/sh

JUJU_DISPATCH_PATH="${JUJU_DISPATCH_PATH:-$0}" PYTHONPATH=lib:venv \
exec ./src/charm.py
11 changes: 11 additions & 0 deletions tests/integration/ops-main-linter-charms/negative/metadata.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
name: smoke
type: charm
title: n/a
summary: n/a
description: n/a

base: ubuntu@24.04
build-base: ubuntu@24.04
platforms:
arm64:
12 changes: 12 additions & 0 deletions tests/integration/ops-main-linter-charms/negative/src/charm.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Copyright 2024 Canonical Ltd.
import ops # type: ignore


class SomeCharm(ops.CharmBase): ...


# ruff: noqa: ERA001
# charmcraft analyse should detect that ops.main() call is missing
#
# if __name__ == "__main__":
# ops.main(SomeCharm)
Empty file.
16 changes: 16 additions & 0 deletions tests/integration/ops-main-linter-charms/smoke/charmcraft.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
name: smoke
type: charm
title: n/a
summary: n/a
description: n/a

base: ubuntu@24.04
build-base: ubuntu@24.04
platforms:
arm64:

parts:
charm:
plugin: charm
source: .
4 changes: 4 additions & 0 deletions tests/integration/ops-main-linter-charms/smoke/dispatch
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#!/bin/sh

JUJU_DISPATCH_PATH="${JUJU_DISPATCH_PATH:-$0}" PYTHONPATH=lib:venv \
exec ./src/charm.py
11 changes: 11 additions & 0 deletions tests/integration/ops-main-linter-charms/smoke/metadata.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
name: smoke
type: charm
title: n/a
summary: n/a
description: n/a

base: ubuntu@24.04
build-base: ubuntu@24.04
platforms:
arm64:
9 changes: 9 additions & 0 deletions tests/integration/ops-main-linter-charms/smoke/src/charm.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Copyright 2024 Canonical Ltd.
import ops # type: ignore


class SomeCharm(ops.CharmBase): ...


if __name__ == "__main__":
ops.main(SomeCharm)
Empty file.
Loading

0 comments on commit 8c87a50

Please sign in to comment.