From 8c4959a5bab8de94c663956c6342bb5c457661e9 Mon Sep 17 00:00:00 2001 From: Alex Lowe Date: Tue, 17 Sep 2024 08:45:38 -0400 Subject: [PATCH 1/5] build(deps): update typing linters --- pyproject.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 39c351974..d6d32da11 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,8 +67,8 @@ lint = [ "yamllint", ] types = [ - "mypy[reports]~=1.5", - "pyright==1.1.366", + "mypy[reports]~=1.11", + "pyright==1.1.380", "types-python-dateutil", "types-PyYAML", "types-requests<2.31.0.20240312", # Frozen until we can get urllib3 v2 From 3f9a7c6206ff63dbf8a0de8ebe78c886e8b229c2 Mon Sep 17 00:00:00 2001 From: Alex Lowe Date: Tue, 17 Sep 2024 09:03:35 -0400 Subject: [PATCH 2/5] chore(cleanup): remove unused cmdbase module --- charmcraft/cmdbase.py | 74 ------------------- tests/integration/commands/test_analyse.py | 3 +- .../commands/test_resource_revisions.py | 7 +- .../commands/test_store_commands.py | 13 ++-- 4 files changed, 10 insertions(+), 87 deletions(-) delete mode 100644 charmcraft/cmdbase.py diff --git a/charmcraft/cmdbase.py b/charmcraft/cmdbase.py deleted file mode 100644 index b7035e688..000000000 --- a/charmcraft/cmdbase.py +++ /dev/null @@ -1,74 +0,0 @@ -# Copyright 2020-2022 Canonical Ltd. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# -# For further info, check https://github.com/canonical/charmcraft - -"""Infrastructure for common base commands functionality.""" - -import json - -import craft_cli -from craft_cli import ArgumentParsingError, CraftError - -JSON_FORMAT = "json" -FORMAT_HELP_STR = "Produce the result in the specified format (currently only 'json')" - - -class BaseCommand(craft_cli.BaseCommand): - """Subclass this to create a new command. - - The following default attribute is provided beyond craft-cli ones: - - The subclass must be declared in the corresponding section of main.COMMAND_GROUPS. - - If the command may produce the result in a programmatic-friendly format, it - should call the 'include_format_option' method to properly affect the parser and - then emit only one message with the result of the 'format_content' method. - """ - - def format_content(self, fmt, content): - """Format the content.""" - if fmt == JSON_FORMAT: - return json.dumps(content, indent=4) - raise ValueError("Specified format not supported.") - - def include_format_option(self, parser): - """Add the 'format' option to this parser.""" - parser.add_argument( - "--format", - choices=[JSON_FORMAT], - help=FORMAT_HELP_STR, - ) - - def _check_config(self, config_file: bool = False, bases: bool = False) -> None: - """Check if valid config contents exists. - - - config_file: if True, check if a valid "charmcraft.yaml" file exists. - - bases: if True, check if a valid "bases" in "charmcraft.yaml" exists. - - :raises ArgumentParsingError: if 'charmcraft.yaml' file is missing. - :raises CraftError: if any specified config are missing or invalid. - """ - if config_file and not self.config.project.config_provided: - raise ArgumentParsingError( - "The specified command needs a valid 'charmcraft.yaml' configuration file (in " - "the current directory or where specified with --project-dir option); see " - "the reference: https://discourse.charmhub.io/t/charmcraft-configuration/4138" - ) - - if bases and self.config.bases is None: - raise CraftError( - "The specified command needs a valid 'bases' in 'charmcraft.yaml' configuration " - "file (in the current directory or where specified with --project-dir option)." - ) diff --git a/tests/integration/commands/test_analyse.py b/tests/integration/commands/test_analyse.py index 36e00fc49..2b69778d5 100644 --- a/tests/integration/commands/test_analyse.py +++ b/tests/integration/commands/test_analyse.py @@ -24,7 +24,6 @@ from charmcraft import linters from charmcraft.application.commands.analyse import Analyse -from charmcraft.cmdbase import JSON_FORMAT from charmcraft.models.lint import LintResult @@ -85,7 +84,7 @@ def test_integration_linters(fake_project_dir, emitter, config, monkeypatch): ) -@pytest.mark.parametrize("indicated_format", [None, JSON_FORMAT]) +@pytest.mark.parametrize("indicated_format", [None, "json"]) def test_complete_set_of_results( check, emitter, service_factory, config, monkeypatch, fake_project_dir, indicated_format ): diff --git a/tests/integration/commands/test_resource_revisions.py b/tests/integration/commands/test_resource_revisions.py index 3e7f8c636..3948e3209 100644 --- a/tests/integration/commands/test_resource_revisions.py +++ b/tests/integration/commands/test_resource_revisions.py @@ -27,7 +27,6 @@ from charmcraft import store from charmcraft.application.commands import ListResourceRevisionsCommand -from charmcraft.cmdbase import JSON_FORMAT from charmcraft.env import CharmhubConfig @@ -47,7 +46,7 @@ def validate_params(config, ephemeral=False, needs_auth=True): yield store_mock -@pytest.mark.parametrize("formatted", [None, JSON_FORMAT]) +@pytest.mark.parametrize("formatted", [None, "json"]) def test_resourcerevisions_simple(emitter, store_mock, config, formatted): """Happy path of one result from the Store.""" store_response = [ @@ -90,7 +89,7 @@ def test_resourcerevisions_simple(emitter, store_mock, config, formatted): emitter.assert_messages(expected) -@pytest.mark.parametrize("formatted", [None, JSON_FORMAT]) +@pytest.mark.parametrize("formatted", [None, "json"]) def test_resourcerevisions_empty(emitter, store_mock, config, formatted): """No results from the store.""" store_response = [] @@ -105,7 +104,7 @@ def test_resourcerevisions_empty(emitter, store_mock, config, formatted): emitter.assert_message("No revisions found.") -@pytest.mark.parametrize("formatted", [None, JSON_FORMAT]) +@pytest.mark.parametrize("formatted", [None, "json"]) def test_resourcerevisions_ordered_by_revision(emitter, store_mock, config, formatted): """Results are presented ordered by revision in the table.""" # three Revisions with all values weirdly similar, the only difference is revision, so diff --git a/tests/integration/commands/test_store_commands.py b/tests/integration/commands/test_store_commands.py index 873c40cb6..fd0af34db 100644 --- a/tests/integration/commands/test_store_commands.py +++ b/tests/integration/commands/test_store_commands.py @@ -22,7 +22,6 @@ from charmcraft import env from charmcraft.application.commands import FetchLibCommand -from charmcraft.cmdbase import JSON_FORMAT from charmcraft.store.models import Library from tests import factory @@ -44,7 +43,7 @@ def validate_params(config, ephemeral=False, needs_auth=True): # region fetch-lib tests -@pytest.mark.parametrize("formatted", [None, JSON_FORMAT]) +@pytest.mark.parametrize("formatted", [None, "json"]) def test_fetchlib_simple_downloaded(emitter, store_mock, tmp_path, monkeypatch, config, formatted): """Happy path fetching the lib for the first time (downloading it).""" monkeypatch.chdir(tmp_path) @@ -228,7 +227,7 @@ def test_fetchlib_simple_updated(emitter, store_mock, tmp_path, monkeypatch, con @pytest.mark.skipif(sys.platform == "win32", reason="Windows not [yet] supported") -@pytest.mark.parametrize("formatted", [None, JSON_FORMAT]) +@pytest.mark.parametrize("formatted", [None, "json"]) def test_fetchlib_all(emitter, store_mock, tmp_path, monkeypatch, config, formatted): """Update all the libraries found in disk.""" monkeypatch.chdir(tmp_path) @@ -338,7 +337,7 @@ def test_fetchlib_all(emitter, store_mock, tmp_path, monkeypatch, config, format assert saved_file.read_text() == "new lib content 2" -@pytest.mark.parametrize("formatted", [None, JSON_FORMAT]) +@pytest.mark.parametrize("formatted", [None, "json"]) def test_fetchlib_store_not_found(emitter, store_mock, config, formatted): """The indicated library is not found in the store.""" store_mock.get_libraries_tips.return_value = {} @@ -364,7 +363,7 @@ def test_fetchlib_store_not_found(emitter, store_mock, config, formatted): emitter.assert_message(error_message) -@pytest.mark.parametrize("formatted", [None, JSON_FORMAT]) +@pytest.mark.parametrize("formatted", [None, "json"]) def test_fetchlib_store_is_old(emitter, store_mock, tmp_path, monkeypatch, config, formatted): """The store has an older version that what is found locally.""" monkeypatch.chdir(tmp_path) @@ -403,7 +402,7 @@ def test_fetchlib_store_is_old(emitter, store_mock, tmp_path, monkeypatch, confi emitter.assert_message(error_message) -@pytest.mark.parametrize("formatted", [None, JSON_FORMAT]) +@pytest.mark.parametrize("formatted", [None, "json"]) def test_fetchlib_store_same_versions_same_hash( emitter, store_mock, tmp_path, monkeypatch, config, formatted ): @@ -444,7 +443,7 @@ def test_fetchlib_store_same_versions_same_hash( emitter.assert_message(error_message) -@pytest.mark.parametrize("formatted", [None, JSON_FORMAT]) +@pytest.mark.parametrize("formatted", [None, "json"]) def test_fetchlib_store_same_versions_different_hash( emitter, store_mock, tmp_path, monkeypatch, config, formatted ): From a96b23951639cc71cbd098420cc5432f1434cc9d Mon Sep 17 00:00:00 2001 From: Alex Lowe Date: Tue, 17 Sep 2024 09:12:04 -0400 Subject: [PATCH 3/5] chore(cleanup): remove unused registry module --- charmcraft/store/__init__.py | 10 - charmcraft/store/registry.py | 463 --------- tests/commands/test_store_registry.py | 1297 ------------------------- 3 files changed, 1770 deletions(-) delete mode 100644 charmcraft/store/registry.py delete mode 100644 tests/commands/test_store_registry.py diff --git a/charmcraft/store/__init__.py b/charmcraft/store/__init__.py index 5bf9cc4d4..dd8362fe8 100644 --- a/charmcraft/store/__init__.py +++ b/charmcraft/store/__init__.py @@ -18,22 +18,12 @@ from charmcraft.store.client import build_user_agent, AnonymousClient, Client from charmcraft.store import models from charmcraft.store.models import LibraryMetadataRequest -from charmcraft.store.registry import ( - OCIRegistry, - HashingTemporaryFile, - LocalDockerdInterface, - ImageHandler, -) from charmcraft.store.store import Store, AUTH_DEFAULT_TTL, AUTH_DEFAULT_PERMISSIONS __all__ = [ "build_user_agent", "AnonymousClient", "Client", - "OCIRegistry", - "HashingTemporaryFile", - "ImageHandler", - "LocalDockerdInterface", "AUTH_DEFAULT_PERMISSIONS", "AUTH_DEFAULT_TTL", "Store", diff --git a/charmcraft/store/registry.py b/charmcraft/store/registry.py deleted file mode 100644 index 53d8027b6..000000000 --- a/charmcraft/store/registry.py +++ /dev/null @@ -1,463 +0,0 @@ -# Copyright 2021-2022 Canonical Ltd. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# -# For further info, check https://github.com/canonical/charmcraft - -"""Module to work with OCI registries.""" - -import base64 -import gzip -import hashlib -import io -import json -import os -import tarfile -import tempfile -from typing import Any -from urllib.request import parse_http_list, parse_keqv_list - -import requests -import requests_unixsocket # type: ignore[import-untyped] -from craft_cli import CraftError, emit - -# some mimetypes -CONFIG_MIMETYPE = "application/vnd.docker.container.image.v1+json" -MANIFEST_V2_MIMETYPE = "application/vnd.docker.distribution.manifest.v2+json" -LAYER_MIMETYPE = "application/vnd.docker.image.rootfs.diff.tar.gzip" -JSON_RELATED_MIMETYPES = { - "application/json", - "application/vnd.docker.distribution.manifest.v1+prettyjws", # signed manifest - MANIFEST_V2_MIMETYPE, -} -OCTET_STREAM_MIMETYPE = "application/octet-stream" - -# downloads and uploads happen in chunks; this size is mostly driven by the usage in the upload -# blob, where the cost in time is similar for small and large chunks (we need to balance having -# it large enough for speed, but not too large because of memory consumption) -CHUNK_SIZE = 2**20 - - -def assert_response_ok( - response: requests.Response, expected_status: int = 200 -) -> dict[str, Any] | None: - """Assert the response is ok.""" - if response.status_code != expected_status: - ct = response.headers.get("Content-Type", "") - if ct.split(";")[0] in JSON_RELATED_MIMETYPES: - errors = response.json().get("errors") - else: - errors = None - raise CraftError( - "Wrong status code from server " - f"(expected={expected_status}, got={response.status_code})", - details=f"errors={errors} headers={response.headers}", - ) - - if response.headers.get("Content-Type") not in JSON_RELATED_MIMETYPES: - return None - - result = response.json() - if "errors" in result: - raise CraftError("Response with errors from server: {}".format(result["errors"])) - return result - - -class OCIRegistry: - """Interface to a generic OCI Registry.""" - - def __init__(self, server, image_name, *, username="", password=""): - self.server = server - self.image_name = image_name - self.auth_token = None - - if username: - _u_p = f"{username}:{password}" - self.auth_encoded_credentials = base64.b64encode(_u_p.encode("ascii")).decode("ascii") - else: - self.auth_encoded_credentials = None - - def __eq__(self, other): - return ( - self.server == other.server - and self.image_name == other.image_name - and self.auth_encoded_credentials == other.auth_encoded_credentials - ) - - def _authenticate(self, auth_info): - """Get the auth token.""" - headers = {} - if self.auth_encoded_credentials is not None: - headers["Authorization"] = f"Basic {self.auth_encoded_credentials}" - - emit.trace(f"Authenticating! {auth_info}") - url = "{realm}?service={service}&scope={scope}".format_map(auth_info) - response = requests.get(url, headers=headers) - - result = assert_response_ok(response) - return result["token"] - - def _get_url(self, subpath): - """Build the URL completing the subpath.""" - return f"{self.server}/v2/{self.image_name}/{subpath}" - - def _get_auth_info(self, response): - """Parse a 401 response and get the needed auth parameters.""" - www_auth = response.headers["Www-Authenticate"] - if not www_auth.startswith("Bearer "): - raise ValueError("Bearer not found") - return parse_keqv_list(parse_http_list(www_auth[7:])) - - def _hit(self, method, url, headers=None, log=True, **kwargs): - """Hit the specific URL, taking care of the authentication.""" - if headers is None: - headers = {} - if self.auth_token is not None: - headers["Authorization"] = f"Bearer {self.auth_token}" - - if log: - emit.trace(f"Hitting the registry: {method} {url}") - response = requests.request(method, url, headers=headers, **kwargs) - if response.status_code == 401: - # token expired or missing, let's get another one and retry - try: - auth_info = self._get_auth_info(response) - except (ValueError, KeyError) as exc: - raise CraftError(f"Bad 401 response: {exc}; headers: {response.headers!r}") - self.auth_token = self._authenticate(auth_info) - headers["Authorization"] = f"Bearer {self.auth_token}" - response = requests.request(method, url, headers=headers, **kwargs) - - return response - - def _is_item_already_uploaded(self, url): - """Verify if a generic item is uploaded.""" - response = self._hit("HEAD", url) - - if response.status_code == 200: - # item is there, done! - uploaded = True - elif response.status_code == 404: - # confirmed item is NOT there - uploaded = False - else: - # something else is going on, log what we have and return False so at least - # we can continue with the upload - emit.debug( - f"Bad response when checking for uploaded {url!r}: " - f"{response.status_code!r} (headers={response.headers})", - ) - uploaded = False - return uploaded - - def is_manifest_already_uploaded(self, reference): - """Verify if the manifest is already uploaded, using a generic reference. - - If yes, return its digest. - """ - emit.progress("Checking if manifest is already uploaded") - url = self._get_url(f"manifests/{reference}") - return self._is_item_already_uploaded(url) - - def is_blob_already_uploaded(self, reference): - """Verify if the blob is already uploaded, using a generic reference. - - If yes, return its digest. - """ - emit.progress("Checking if the blob is already uploaded") - url = self._get_url(f"blobs/{reference}") - return self._is_item_already_uploaded(url) - - def upload_manifest(self, manifest_data, reference): - """Upload a manifest.""" - url = self._get_url(f"manifests/{reference}") - headers = { - "Content-Type": MANIFEST_V2_MIMETYPE, - } - emit.progress(f"Uploading manifest with reference {reference}") - response = self._hit("PUT", url, headers=headers, data=manifest_data.encode("utf8")) - assert_response_ok(response, expected_status=201) - emit.progress("Manifest uploaded OK") - - def upload_blob(self, filepath, size, digest): - """Upload the blob from a file.""" - # get the first URL to start pushing the blob - emit.progress("Getting URL to push the blob") - url = self._get_url("blobs/uploads/") - response = self._hit("POST", url) - assert_response_ok(response, expected_status=202) - upload_url = response.headers["Location"] - range_from, range_to_inclusive = (int(x) for x in response.headers["Range"].split("-")) - emit.progress(f"Got upload URL ok with range {range_from}-{range_to_inclusive}") - if range_from != 0: - raise CraftError( - "Server error: bad range received", details=f"Range={response.headers['Range']!r}" - ) - - # this `range_to_inclusive` alteration is a side effect of the range being inclusive. The - # server tells us that it already has "0-80", means that it has 81 bytes (from 0 to 80 - # inclusive), we set from_position in 81 and read from there. Going down, "0-1" would mean - # it has bytes 0 and 1; But "0-0" is special, it's what the server returns when it does - # not have ANY bytes at all. So we comply with Range parameter, but addressing this - # special case; worst think it could happen is that we start from 0 when the server - # has 1 byte already, which is not a problem. - if range_to_inclusive == 0: - range_to_inclusive = -1 - from_position = range_to_inclusive + 1 - - # start the chunked upload - with open(filepath, "rb") as fh: - with emit.progress_bar("Uploading...", size) as progress: - if from_position: - fh.seek(from_position) - progress.advance(from_position) - - while True: - chunk = fh.read(CHUNK_SIZE) - if not chunk: - break - - progress.advance(len(chunk)) - end_position = from_position + len(chunk) - headers = { - "Content-Length": str(len(chunk)), - "Content-Range": f"{from_position}-{end_position}", - "Content-Type": OCTET_STREAM_MIMETYPE, - } - response = self._hit( - "PATCH", upload_url, headers=headers, data=chunk, log=False - ) - assert_response_ok(response, expected_status=202) - - upload_url = response.headers["Location"] - from_position += len(chunk) - headers = { - "Content-Length": "0", - "Connection": "close", - } - emit.progress("Closing the upload") - closing_url = f"{upload_url}&digest={digest}" - - response = self._hit("PUT", closing_url, headers=headers, data="") - assert_response_ok(response, expected_status=201) - emit.progress("Upload finished OK") - if response.headers["Docker-Content-Digest"] != digest: - raise CraftError("Server error: the upload is corrupted") - - -class HashingTemporaryFile(io.FileIO): - """A temporary file that keeps the hash and length of what is written.""" - - def __init__(self): - tmp_file = tempfile.NamedTemporaryFile(mode="wb", delete=False) - self.file_handler = tmp_file.file - super().__init__(tmp_file.name, mode="wb") - self.total_length = 0 - self.hasher = hashlib.sha256() - - @property - def hexdigest(self): - """Calculate the digest.""" - return self.hasher.hexdigest() - - def write(self, data): - """Intercept real write to feed hasher and length count.""" - self.total_length += len(data) - self.hasher.update(data) - super().write(data) - - -class LocalDockerdInterface: - """Functionality to interact with a local Docker daemon.""" - - # the address of the dockerd socket - dockerd_socket_baseurl = "http+unix://%2Fvar%2Frun%2Fdocker.sock" - - def __init__(self): - self.session = requests_unixsocket.Session() - - def get_image_info_from_id(self, image_id: str) -> dict | None: - """Get the info for a specific image using its id. - - Returns None to flag that the requested id was not found for any reason. - """ - url = self.dockerd_socket_baseurl + f"/images/{image_id}/json" - try: - response = self.session.get(url) - except requests.exceptions.ConnectionError: - emit.debug( - "Cannot connect to /var/run/docker.sock , please ensure dockerd is running.", - ) - return None - - if response.status_code == 200: - # image is there, we're fine - return response.json() - - # 404 is the standard response to "not found", if not exactly that let's log - # for proper debugging - if response.status_code != 404: - emit.debug(f"Bad response when validating local image: {response.status_code}") - return None - return None - - def get_image_info_from_digest(self, digest: str) -> dict | None: - """Get the info for a specific image using its digest. - - Returns None to flag that the requested digest was not found for any reason. - """ - url = self.dockerd_socket_baseurl + "/images/json" - try: - response = self.session.get(url) - except requests.exceptions.ConnectionError: - emit.debug( - "Cannot connect to /var/run/docker.sock , please ensure dockerd is running.", - ) - return None - - if response.status_code != 200: - emit.debug(f"Bad response when validating local image: {response.status_code}") - return None - - for image_info in response.json(): - if image_info["RepoDigests"] is None: - continue - if any(digest in repo_digest for repo_digest in image_info["RepoDigests"]): - return image_info - return None - - def get_streamed_image_content(self, image_id: str) -> requests.Response: - """Stream the content of a specific image.""" - url = self.dockerd_socket_baseurl + f"/images/{image_id}/get" - return self.session.get(url, stream=True) - - -class ImageHandler: - """Provide specific functionalities around images.""" - - def __init__(self, registry): - self.registry = registry - - def check_in_registry(self, digest: str) -> bool: - """Verify if the image is present in the registry.""" - return self.registry.is_manifest_already_uploaded(digest) - - def _extract_file( - self, image_tar: str, name: str, compress: bool = False - ) -> tuple[str, int, str]: - """Extract a file from the tar and return its info. Optionally, gzip the content.""" - emit.progress(f"Extracting file {name!r} from local tar (compress={compress})") - src_filehandler = image_tar.extractfile(name) - mtime = image_tar.getmember(name).mtime - - hashing_temp_file = HashingTemporaryFile() - if compress: - # open the gzip file using the temporary file handler; use the original name and time - # as 'filename' and 'mtime' correspondingly as those go to the gzip headers, - # to ensure same final hash across different runs - dst_filehandler = gzip.GzipFile( - fileobj=hashing_temp_file, - mode="wb", - filename=os.path.basename(name), - mtime=mtime, - ) - else: - dst_filehandler = hashing_temp_file - try: - while True: - chunk = src_filehandler.read(CHUNK_SIZE) - if not chunk: - break - dst_filehandler.write(chunk) - finally: - dst_filehandler.close() - # gzip does not automatically close the underlying file handler, let's do it manually - hashing_temp_file.close() - - digest = f"sha256:{hashing_temp_file.hexdigest}" - return hashing_temp_file.name, hashing_temp_file.total_length, digest - - def _upload_blob(self, filepath: str, size: int, digest: str) -> None: - """Upload the blob (if necessary).""" - # if it's already uploaded, nothing to do - if self.registry.is_blob_already_uploaded(digest): - emit.progress("Blob was already uploaded") - else: - self.registry.upload_blob(filepath, size, digest) - - # finally remove the temp filepath - os.unlink(filepath) - - def upload_from_local(self, image_info: dict[str, Any]) -> str | None: - """Upload the image from the local registry. - - Returns the new remote digest. - """ - dockerd = LocalDockerdInterface() - local_image_size = image_info["Size"] - image_id = image_info["Id"] - - emit.progress(f"Getting the image from the local repo; size={local_image_size}") - response = dockerd.get_streamed_image_content(image_id) - - tmp_exported = tempfile.NamedTemporaryFile(mode="wb", delete=False) - with emit.progress_bar("Reading image...", local_image_size) as progress: - for chunk in response.iter_content(CHUNK_SIZE): - progress.advance(len(chunk)) - tmp_exported.file.write(chunk) - tmp_exported.close() - - # open the image tar and inspect it to get the config and layers from the only one - # manifest inside (as it's a list of one) - image_tar = tarfile.open(tmp_exported.name) - local_manifest = json.load(image_tar.extractfile("manifest.json")) - (local_manifest,) = local_manifest - config_name = local_manifest.get("Config") - layer_names = local_manifest["Layers"] - manifest = { - "mediaType": MANIFEST_V2_MIMETYPE, - "schemaVersion": 2, - } - - if config_name is not None: - fpath, size, digest = self._extract_file(image_tar, config_name) - emit.progress(f"Uploading config blob, size={size}, digest={digest}") - self._upload_blob(fpath, size, digest) - manifest["config"] = { - "digest": digest, - "mediaType": CONFIG_MIMETYPE, - "size": size, - } - - manifest["layers"] = manifest_layers = [] - len_layers = len(layer_names) - for idx, layer_name in enumerate(layer_names, 1): - fpath, size, digest = self._extract_file(image_tar, layer_name, compress=True) - emit.progress(f"Uploading layer blob {idx}/{len_layers}, size={size}, digest={digest}") - self._upload_blob(fpath, size, digest) - manifest_layers.append( - { - "digest": digest, - "mediaType": LAYER_MIMETYPE, - "size": size, - } - ) - - # remove the temp tar file - os.unlink(tmp_exported.name) - - # upload the manifest - manifest_data = json.dumps(manifest) - digest = "sha256:{}".format(hashlib.sha256(manifest_data.encode("utf8")).hexdigest()) - self.registry.upload_manifest(manifest_data, digest) - return digest diff --git a/tests/commands/test_store_registry.py b/tests/commands/test_store_registry.py deleted file mode 100644 index 22aef17ec..000000000 --- a/tests/commands/test_store_registry.py +++ /dev/null @@ -1,1297 +0,0 @@ -# Copyright 2021-2022 Canonical Ltd. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# -# For further info, check https://github.com/canonical/charmcraft - -"""Tests for the OCI Registry related functionality (code in store/registry.py).""" - -import base64 -import gzip -import hashlib -import io -import json -import pathlib -import sys -import tarfile -from unittest.mock import call, patch - -import pytest -import requests -from craft_cli import CraftError - -from charmcraft import const -from charmcraft.store import registry -from charmcraft.store.registry import ( - CONFIG_MIMETYPE, - LAYER_MIMETYPE, - MANIFEST_V2_MIMETYPE, - OCTET_STREAM_MIMETYPE, - ImageHandler, - LocalDockerdInterface, - OCIRegistry, - assert_response_ok, -) - -# -- tests for response verifications - - -def create_response( - status_code=200, headers=None, raw_content=b"", json_content=None, content_type=None -): - """Create a fake requests' response.""" - if headers is None: - headers = {} - - if json_content is not None: - headers.setdefault("Content-Type", content_type or "application/json") - content_bytes = json.dumps(json_content).encode("utf8") - else: - content_bytes = raw_content - - resp = requests.Response() - resp.status_code = status_code - resp.raw = io.BytesIO(content_bytes) - resp.headers = headers # not case insensitive, but good enough - return resp - - -def test_assert_response_ok_simple_json(): - """Simple case for a good response with JSON content.""" - test_content = {"foo": 2, "bar": 1} - response = create_response(json_content=test_content) - result = assert_response_ok(response) - assert result == test_content - - -def test_assert_response_ok_not_json(): - """A good non-json response.""" - response = create_response(raw_content=b"stuff") - result = assert_response_ok(response) - assert result is None - - -def test_assert_response_ok_different_status(): - """A good response with a different status code.""" - test_content = {"foo": 2, "bar": 1} - response = create_response(json_content=test_content, status_code=201) - result = assert_response_ok(response, expected_status=201) - assert result == test_content - - -def test_assert_response_errors_in_result(): - """Response is as expected but server flags errors.""" - errors = [{"foo": "bar"}] - test_content = {"errors": errors} - response = create_response(json_content=test_content) - with pytest.raises(CraftError) as cm: - assert_response_ok(response) - assert str(cm.value) == f"Response with errors from server: {errors}" - - -def test_assert_response_bad_status_code_with_json_errors(): - """Different status code than expected, with the server including errors.""" - errors = [{"foo": "bar"}] - test_content = {"errors": errors} - response = create_response(status_code=404, json_content=test_content) - with pytest.raises(CraftError) as cm: - assert_response_ok(response) - error = cm.value - assert str(error) == "Wrong status code from server (expected=200, got=404)" - assert error.details == f"errors={errors} headers={{'Content-Type': 'application/json'}}" - - -def test_assert_response_bad_status_code_with_extra_json_errors(): - """The server still including errors, weird content type.""" - errors = [{"foo": "bar"}] - test_content = {"errors": errors} - response = create_response( - status_code=404, - json_content=test_content, - content_type="application/json;stuff", - ) - with pytest.raises(CraftError) as cm: - assert_response_ok(response) - error = cm.value - assert str(error) == "Wrong status code from server (expected=200, got=404)" - assert error.details == f"errors={errors} headers={{'Content-Type': 'application/json;stuff'}}" - - -def test_assert_response_bad_status_code_blind(): - """Different status code than expected, no more info.""" - response = create_response(status_code=500, raw_content=b"stuff") - with pytest.raises(CraftError) as cm: - assert_response_ok(response) - error = cm.value - assert str(error) == "Wrong status code from server (expected=200, got=500)" - assert error.details == "errors=None headers={}" - - -# -- tests for OCIRegistry auth & hit helpers - - -def test_auth_simple(responses): - """Simple authentication.""" - responses.add( - responses.GET, - "https://auth.fakereg.com?service=test-service&scope=test-scope", - json={"token": "test-token"}, - ) - - ocireg = OCIRegistry("https://fakereg.com", "test-image") - auth_info = { - "realm": "https://auth.fakereg.com", - "service": "test-service", - "scope": "test-scope", - } - token = ocireg._authenticate(auth_info) - assert token == "test-token" - sent_auth_header = responses.calls[0].request.headers.get("Authorization") - assert sent_auth_header is None - - -def test_auth_with_credentials(emitter, responses): - """Authenticate passing credentials.""" - responses.add( - responses.GET, - "https://auth.fakereg.com?service=test-service&scope=test-scope", - json={"token": "test-token"}, - ) - - ocireg = OCIRegistry( - "https://fakereg.com", - "test-image", - username="test-user", - password="test-password", - ) - auth_info = { - "realm": "https://auth.fakereg.com", - "service": "test-service", - "scope": "test-scope", - } - token = ocireg._authenticate(auth_info) - assert token == "test-token" - sent_auth_header = responses.calls[0].request.headers.get("Authorization") - expected_encoded = base64.b64encode(b"test-user:test-password") - assert sent_auth_header == "Basic " + expected_encoded.decode("ascii") - - # generic auth indication is logged but NOT the credentials - expected = f"Authenticating! {auth_info}" - emitter.assert_trace(expected) - - -def test_auth_with_just_username(responses): - """Authenticate passing credentials.""" - responses.add( - responses.GET, - "https://auth.fakereg.com?service=test-service&scope=test-scope", - json={"token": "test-token"}, - ) - - ocireg = OCIRegistry("https://fakereg.com", "test-image", username="test-user") - auth_info = { - "realm": "https://auth.fakereg.com", - "service": "test-service", - "scope": "test-scope", - } - token = ocireg._authenticate(auth_info) - assert token == "test-token" - sent_auth_header = responses.calls[0].request.headers.get("Authorization") - expected_encoded = base64.b64encode(b"test-user:") - assert sent_auth_header == "Basic " + expected_encoded.decode("ascii") - - -def test_hit_simple_initial_auth_ok(emitter, responses): - """Simple GET with auth working at once.""" - # set the Registry with an initial token - ocireg = OCIRegistry("https://fakereg.com", "test-image") - ocireg.auth_token = "some auth token" - - # fake a 200 response - responses.add(responses.GET, "https://fakereg.com/api/stuff") - - # try it - response = ocireg._hit("GET", "https://fakereg.com/api/stuff") - assert response == responses.calls[0].response - - # verify it authed ok - sent_auth_header = responses.calls[0].request.headers.get("Authorization") - assert sent_auth_header == "Bearer some auth token" - - # logged what it did - expected = "Hitting the registry: GET https://fakereg.com/api/stuff" - emitter.assert_trace(expected) - - -def test_hit_simple_re_auth_ok(responses): - """Simple GET but needing to re-authenticate.""" - # set the Registry - ocireg = OCIRegistry("https://fakereg.com", "test-image") - ocireg.auth_token = "some auth token" - - # need to set up two responses! - # - the 401 response with the proper info to re-auth - # - the request that actually works - headers = { - "Www-Authenticate": ( - 'Bearer realm="https://auth.fakereg.com/token",' - 'service="https://fakereg.com",scope="repository:library/stuff:pull"' - ) - } - responses.add(responses.GET, "https://fakereg.com/api/stuff", headers=headers, status=401) - responses.add(responses.GET, "https://fakereg.com/api/stuff") - - # try it, isolating the re-authentication (tested separately above) - with patch.object(ocireg, "_authenticate") as mock_auth: - mock_auth.return_value = "new auth token" - response = ocireg._hit("GET", "https://fakereg.com/api/stuff") - assert response == responses.calls[1].response - mock_auth.assert_called_with( - { - "realm": "https://auth.fakereg.com/token", - "scope": "repository:library/stuff:pull", - "service": "https://fakereg.com", - } - ) - - # verify it authed ok both times, with corresponding tokens, and that it stored the new one - sent_auth_header = responses.calls[0].request.headers.get("Authorization") - assert sent_auth_header == "Bearer some auth token" - sent_auth_header = responses.calls[1].request.headers.get("Authorization") - assert sent_auth_header == "Bearer new auth token" - assert ocireg.auth_token == "new auth token" - - -def test_hit_simple_re_auth_problems(responses): - """Bad response from the re-authentication process.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - - # set only one response, a 401 which is broken and all will end there - headers = {"Www-Authenticate": "broken header"} - responses.add(responses.GET, "https://fakereg.com/api/stuff", headers=headers, status=401) - - # try it, isolating the re-authentication (tested separately above) - expected = ( - "Bad 401 response: Bearer not found; headers: {.*'Www-Authenticate': 'broken header'.*}" - ) - with pytest.raises(CraftError, match=expected): - ocireg._hit("GET", "https://fakereg.com/api/stuff") - - -def test_hit_different_method(responses): - """Simple request using something else than GET.""" - # set the Registry with an initial token - ocireg = OCIRegistry("https://fakereg.com", "test-image") - ocireg.auth_token = "some auth token" - - # fake a 200 response - responses.add(responses.POST, "https://fakereg.com/api/stuff") - - # try it - response = ocireg._hit("POST", "https://fakereg.com/api/stuff") - assert response == responses.calls[0].response - - -def test_hit_including_headers(responses): - """A request including more headers.""" - # set the Registry with an initial token - ocireg = OCIRegistry("https://fakereg.com", "test-image") - ocireg.auth_token = "some auth token" - - # fake a 200 response - responses.add(responses.POST, "https://fakereg.com/api/stuff") - - # try it - response = ocireg._hit("POST", "https://fakereg.com/api/stuff", headers={"FOO": "bar"}) - assert response == responses.calls[0].response - - # check that it sent the requested header AND the automatic auth one - sent_headers = responses.calls[0].request.headers - assert sent_headers.get("FOO") == "bar" - assert sent_headers.get("Authorization") == "Bearer some auth token" - - -def test_hit_extra_parameters(responses): - """The request can include extra parameters.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - - # fake a 200 response - responses.add(responses.PUT, "https://fakereg.com/api/stuff") - - # try it - response = ocireg._hit("PUT", "https://fakereg.com/api/stuff", data=b"test-payload") - assert response == responses.calls[0].response - assert responses.calls[0].request.body == b"test-payload" - - -def test_hit_no_log(emitter, responses): - """Simple request but avoiding log.""" - # set the Registry with an initial token - ocireg = OCIRegistry("https://fakereg.com", "test-image") - ocireg.auth_token = "some auth token" - - # fake a 200 response - responses.add(responses.PUT, "https://fakereg.com/api/stuff") - - # try it - ocireg._hit("PUT", "https://fakereg.com/api/stuff", log=False) - - # nothing shown! - emitter.assert_interactions(None) - - -# -- tests for other OCIRegistry helpers: checkers if stuff uploaded - - -def test_ociregistry_is_manifest_uploaded(): - """Check the simple call with correct path to the generic verifier.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - with patch.object(ocireg, "_is_item_already_uploaded") as mock_verifier: - mock_verifier.return_value = "whatever" - result = ocireg.is_manifest_already_uploaded("test-reference") - assert result == "whatever" - url = "https://fakereg.com/v2/test-image/manifests/test-reference" - mock_verifier.assert_called_with(url) - - -def test_ociregistry_is_blob_uploaded(): - """Check the simple call with correct path to the generic verifier.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - with patch.object(ocireg, "_is_item_already_uploaded") as mock_verifier: - mock_verifier.return_value = "whatever" - result = ocireg.is_blob_already_uploaded("test-reference") - assert result == "whatever" - url = "https://fakereg.com/v2/test-image/blobs/test-reference" - mock_verifier.assert_called_with(url) - - -def test_ociregistry_is_item_uploaded_simple_yes(responses): - """Simple case for the item already uploaded.""" - ocireg = OCIRegistry("http://fakereg.com/", "test-image") - url = "http://fakereg.com/v2/test-image/stuff/some-reference" - responses.add(responses.HEAD, url) - - # try it - result = ocireg._is_item_already_uploaded(url) - assert result is True - - -def test_ociregistry_is_item_uploaded_simple_no(responses): - """Simple case for the item NOT already uploaded.""" - ocireg = OCIRegistry("http://fakereg.com/", "test-image") - url = "http://fakereg.com/v2/test-image/stuff/some-reference" - responses.add(responses.HEAD, url, status=404) - - # try it - result = ocireg._is_item_already_uploaded(url) - assert result is False - - -@pytest.mark.parametrize("redir_status", [302, 307]) -def test_ociregistry_is_item_uploaded_redirect(responses, redir_status): - """The verification is redirected to somewhere else.""" - ocireg = OCIRegistry("http://fakereg.com/", "test-image") - url1 = "http://fakereg.com/v2/test-image/stuff/some-reference" - url2 = "http://fakereg.com/real-check/test-image/stuff/some-reference" - responses.add(responses.HEAD, url1, status=redir_status, headers={"Location": url2}) - responses.add(responses.HEAD, url2, status=200) - - # try it - result = ocireg._is_item_already_uploaded(url1) - assert result is True - - -def test_ociregistry_is_item_uploaded_strange_response(responses, emitter): - """Unexpected response.""" - ocireg = OCIRegistry("http://fakereg.com/", "test-image") - url = "http://fakereg.com/v2/test-image/stuff/some-reference" - responses.add(responses.HEAD, url, status=400, headers={"foo": "bar"}) - - # try it - result = ocireg._is_item_already_uploaded(url) - assert result is False - expected = ( - "Bad response when checking for uploaded " - "'http://fakereg.com/v2/test-image/stuff/some-reference': 400 " - "(headers={'Content-Type': 'text/plain', 'foo': 'bar'})" - ) - emitter.assert_debug(expected) - - -# -- test for the OCIRegistry manifest upload - - -def test_ociregistry_upload_manifest_v2(responses, emitter): - """Upload a V2 manifest.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - - url = "https://fakereg.com/v2/test-image/manifests/test-reference" - responses.add(responses.PUT, url, status=201) - - # try it - raw_manifest_data = "test-manifest" - ocireg.upload_manifest(raw_manifest_data, "test-reference") - - # check logs - emitter.assert_progress("Uploading manifest with reference test-reference") - emitter.assert_progress("Manifest uploaded OK") - - # check header and data sent - assert responses.calls[0].request.headers["Content-Type"] == MANIFEST_V2_MIMETYPE - assert responses.calls[0].request.body == raw_manifest_data.encode("ascii") - - -# -- tests for the OCIRegistry blob upload - - -def test_ociregistry_upload_blob_complete(tmp_path, emitter, responses, monkeypatch): - """Complete upload of a binary to the registry.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - base_url = "https://fakereg.com/v2/test-image/" - - # fake the first initial response - pump_url_1 = base_url + "fakeurl-1" - responses.add( - responses.POST, - base_url + "blobs/uploads/", - status=202, - headers={"Location": pump_url_1, "Range": "0-0"}, - ) - - # and the intermediate ones, chained - pump_url_2 = base_url + "fakeurl-2" - pump_url_3 = base_url + "fakeurl-3" - pump_url_4 = base_url + "fakeurl-4" - responses.add(responses.PATCH, pump_url_1, status=202, headers={"Location": pump_url_2}) - responses.add(responses.PATCH, pump_url_2, status=202, headers={"Location": pump_url_3}) - responses.add(responses.PATCH, pump_url_3, status=202, headers={"Location": pump_url_4}) - - # finally, the closing url - responses.add( - responses.PUT, - base_url + "fakeurl-4&digest=test-digest", - status=201, - headers={"Docker-Content-Digest": "test-digest"}, - ) - - # prepare a fake content that will be pushed in 3 parts - monkeypatch.setattr(registry, "CHUNK_SIZE", 3) - bytes_source = tmp_path / "testfile" - bytes_source.write_text("abcdefgh") - - # call! - ocireg.upload_blob(bytes_source, 8, "test-digest") - - # check all the sent headers - expected_headers_per_request = [ - {}, # nothing special in the initial one - { - "Content-Length": "3", - "Content-Range": "0-3", - "Content-Type": OCTET_STREAM_MIMETYPE, - }, - { - "Content-Length": "3", - "Content-Range": "3-6", - "Content-Type": OCTET_STREAM_MIMETYPE, - }, - { - "Content-Length": "2", - "Content-Range": "6-8", - "Content-Type": OCTET_STREAM_MIMETYPE, - }, - {"Content-Length": "0", "Connection": "close"}, # closing - ] - for idx, expected_headers in enumerate(expected_headers_per_request): - sent_headers = responses.calls[idx].request.headers - for key, value in expected_headers.items(): - assert sent_headers.get(key) == value - - emitter.assert_interactions( - [ - call("progress", "Getting URL to push the blob"), - call( - "trace", - "Hitting the registry: POST https://fakereg.com/v2/test-image/blobs/uploads/", - ), - call("progress", "Got upload URL ok with range 0-0"), - call("progress_bar", "Uploading...", 8), - call("advance", 3), - call("advance", 3), - call("advance", 2), - call("progress", "Closing the upload"), - call( - "trace", - ( - "Hitting the registry: PUT " - "https://fakereg.com/v2/test-image/fakeurl-4&digest=test-digest" - ), - ), - call("progress", "Upload finished OK"), - ] - ) - - -def test_ociregistry_upload_blob_bad_initial_response(responses): - """Bad initial response when starting to upload.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - base_url = "https://fakereg.com/v2/test-image/" - - # fake the first initial response with problems - responses.add(responses.POST, base_url + "blobs/uploads/", status=500) - - # call! - msg = r"Wrong status code from server \(expected=202, got=500\).*" - with pytest.raises(CraftError, match=msg): - ocireg.upload_blob("test-filepath", 8, "test-digest") - - -def test_ociregistry_upload_blob_bad_upload_range(responses): - """Received a broken range info.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - base_url = "https://fakereg.com/v2/test-image/" - - # fake the first initial response with problems - responses.add( - responses.POST, - base_url + "blobs/uploads/", - status=202, - headers={"Location": "test-next-url", "Range": "9-9"}, - ) - - # call! - with pytest.raises(CraftError) as cm: - ocireg.upload_blob("test-filepath", 8, "test-digest") - error = cm.value - assert str(error) == "Server error: bad range received" - assert error.details == "Range='9-9'" - - -def test_ociregistry_upload_blob_resumed(tmp_path, emitter, responses): - """The upload is resumed after server indication to do so.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - base_url = "https://fakereg.com/v2/test-image/" - - # fake the first initial response, indicating that the store has already the first 5 bytes - pump_url_1 = base_url + "fakeurl-1" - responses.add( - responses.POST, - base_url + "blobs/uploads/", - status=202, - headers={"Location": pump_url_1, "Range": "0-4"}, - ) # has bytes in position 0, 1, 2, 3 & 4 - - # and the intermediate one - pump_url_2 = base_url + "fakeurl-2" - responses.add(responses.PATCH, pump_url_1, status=202, headers={"Location": pump_url_2}) - - # finally, the closing url - responses.add( - responses.PUT, - base_url + "fakeurl-2&digest=test-digest", - status=201, - headers={"Docker-Content-Digest": "test-digest"}, - ) - - # prepare a fake content - bytes_source = tmp_path / "testfile" - bytes_source.write_text("abcdefgh") - - # call! - ocireg.upload_blob(bytes_source, 8, "test-digest") - - # check all the sent headers - expected_headers_per_request = [ - {}, # nothing special in the initial one - { - "Content-Length": "3", - "Content-Range": "5-8", - "Content-Type": OCTET_STREAM_MIMETYPE, - }, - {"Content-Length": "0", "Connection": "close"}, # closing - ] - for idx, expected_headers in enumerate(expected_headers_per_request): - sent_headers = responses.calls[idx].request.headers - for key, value in expected_headers.items(): - assert sent_headers.get(key) == value - - emitter.assert_interactions( - [ - call("progress", "Getting URL to push the blob"), - call( - "trace", - "Hitting the registry: POST https://fakereg.com/v2/test-image/blobs/uploads/", - ), - call("progress", "Got upload URL ok with range 0-4"), - call("progress_bar", "Uploading...", 8), - call("advance", 5), - call("advance", 3), - call("progress", "Closing the upload"), - call( - "trace", - ( - "Hitting the registry: PUT " - "https://fakereg.com/v2/test-image/fakeurl-2&digest=test-digest" - ), - ), - call("progress", "Upload finished OK"), - ] - ) - - -def test_ociregistry_upload_blob_bad_response_middle(tmp_path, responses, monkeypatch): - """Bad response from the server when pumping bytes.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - base_url = "https://fakereg.com/v2/test-image/" - - # fake the first initial response - pump_url_1 = base_url + "fakeurl-1" - responses.add( - responses.POST, - base_url + "blobs/uploads/", - status=202, - headers={"Location": pump_url_1, "Range": "0-0"}, - ) - - # and the intermediate ones, chained, with a crash - pump_url_2 = base_url + "fakeurl-2" - responses.add(responses.PATCH, pump_url_1, status=202, headers={"Location": pump_url_2}) - responses.add(responses.PATCH, pump_url_2, status=504) - - # prepare a fake content that will be pushed in 3 parts - monkeypatch.setattr(registry, "CHUNK_SIZE", 3) - bytes_source = tmp_path / "testfile" - bytes_source.write_text("abcdefgh") - - # call! - msg = r"Wrong status code from server \(expected=202, got=504\).*" - with pytest.raises(CraftError, match=msg): - ocireg.upload_blob(bytes_source, 8, "test-digest") - - -def test_ociregistry_upload_blob_bad_response_closing(tmp_path, responses): - """Bad response from the server when closing the upload.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - base_url = "https://fakereg.com/v2/test-image/" - - # fake the first initial response - pump_url_1 = base_url + "fakeurl-1" - responses.add( - responses.POST, - base_url + "blobs/uploads/", - status=202, - headers={"Location": pump_url_1, "Range": "0-0"}, - ) - - # and the intermediate one - pump_url_2 = base_url + "fakeurl-2" - responses.add(responses.PATCH, pump_url_1, status=202, headers={"Location": pump_url_2}) - - # finally, the closing url, crashing - responses.add(responses.PUT, base_url + "fakeurl-2&digest=test-digest", status=502) - - # prepare a fake content - bytes_source = tmp_path / "testfile" - bytes_source.write_text("abcdefgh") - - # call! - msg = r"Wrong status code from server \(expected=201, got=502\).*" - with pytest.raises(CraftError, match=msg): - ocireg.upload_blob(bytes_source, 8, "test-digest") - - -def test_ociregistry_upload_blob_bad_final_digest(tmp_path, responses): - """Bad digest from server after closing the upload.""" - ocireg = OCIRegistry("https://fakereg.com", "test-image") - base_url = "https://fakereg.com/v2/test-image/" - - # fake the first initial response - pump_url_1 = base_url + "fakeurl-1" - responses.add( - responses.POST, - base_url + "blobs/uploads/", - status=202, - headers={"Location": pump_url_1, "Range": "0-0"}, - ) - - # and the intermediate one - pump_url_2 = base_url + "fakeurl-2" - responses.add(responses.PATCH, pump_url_1, status=202, headers={"Location": pump_url_2}) - - # finally, the closing url, bad digest - responses.add( - responses.PUT, - base_url + "fakeurl-2&digest=test-digest", - status=201, - headers={"Docker-Content-Digest": "somethingelse"}, - ) - - # prepare a fake content - bytes_source = tmp_path / "testfile" - bytes_source.write_text("abcdefgh") - - # call! - msg = "Server error: the upload is corrupted" - with pytest.raises(CraftError, match=msg): - ocireg.upload_blob(bytes_source, 8, "test-digest") - - -# -- tests for the ImageHandler helpers and functionalities - - -def test_localdockerinterface_get_info_by_id_ok(responses, emitter): - """Get image info ok.""" - test_image_info = {"some": "stuff"} - responses.add( - responses.GET, - LocalDockerdInterface.dockerd_socket_baseurl + "/images/test-id/json", - json=test_image_info, - ) - ldi = LocalDockerdInterface() - resp = ldi.get_image_info_from_id("test-id") - assert resp == test_image_info - - emitter.assert_interactions(None) - - -def test_localdockerinterface_get_info_by_id_not_found(responses, emitter): - """Get image info for something that is not there.""" - # return 404, which means that the image was not found - responses.add( - responses.GET, - LocalDockerdInterface.dockerd_socket_baseurl + "/images/test-id/json", - status=404, - ) - ldi = LocalDockerdInterface() - resp = ldi.get_image_info_from_id("test-id") - assert resp is None - - emitter.assert_interactions(None) - - -def test_localdockerinterface_get_info_by_id_bad_response(responses, emitter): - """Docker answered badly when checking for the image.""" - # weird dockerd behaviour - responses.add( - responses.GET, - LocalDockerdInterface.dockerd_socket_baseurl + "/images/test-id/json", - status=500, - ) - ldi = LocalDockerdInterface() - resp = ldi.get_image_info_from_id("test-id") - assert resp is None - - emitter.assert_debug("Bad response when validating local image: 500") - - -def test_localdockerinterface_get_info_by_id_disconnected(emitter, responses): - """No daemon to talk to (see responses used as fixture but no listening).""" - ldi = LocalDockerdInterface() - resp = ldi.get_image_info_from_id("test-id") - assert resp is None - - emitter.assert_debug( - "Cannot connect to /var/run/docker.sock , please ensure dockerd is running." - ) - - -def test_localdockerinterface_get_info_by_digest_ok(responses, emitter): - """Get image info ok.""" - test_image_info_1 = {"some": "stuff", "RepoDigests": ["name @ sha256:test-digest", "other"]} - test_image_info_2 = {"some": "stuff", "RepoDigests": ["foo", "bar"]} - test_search_respoonse = [test_image_info_1, test_image_info_2] - responses.add( - responses.GET, - LocalDockerdInterface.dockerd_socket_baseurl + "/images/json", - json=test_search_respoonse, - ) - ldi = LocalDockerdInterface() - resp = ldi.get_image_info_from_digest("sha256:test-digest") - assert resp == test_image_info_1 - - emitter.assert_interactions(None) - - -def test_localdockerinterface_get_info_by_digest_not_found(responses, emitter): - """Get image info for something that is not there.""" - test_image_info_1 = {"some": "stuff", "RepoDigests": ["other"]} - test_image_info_2 = {"some": "stuff", "RepoDigests": ["foo", "bar"]} - test_search_respoonse = [test_image_info_1, test_image_info_2] - responses.add( - responses.GET, - LocalDockerdInterface.dockerd_socket_baseurl + "/images/json", - json=test_search_respoonse, - ) - ldi = LocalDockerdInterface() - resp = ldi.get_image_info_from_digest("sha256:test-digest") - assert resp is None - - emitter.assert_interactions(None) - - -def test_localdockerinterface_get_info_by_digest_none_digest(responses, emitter): - """Get image info for something that is not there.""" - test_image_info_1 = {"some": "stuff", "RepoDigests": None} - test_search_respoonse = [test_image_info_1] - responses.add( - responses.GET, - LocalDockerdInterface.dockerd_socket_baseurl + "/images/json", - json=test_search_respoonse, - ) - ldi = LocalDockerdInterface() - resp = ldi.get_image_info_from_digest("sha256:test-digest") - assert resp is None - - emitter.assert_interactions(None) - - -def test_localdockerinterface_get_info_by_digest_bad_response(responses, emitter): - """Docker answered badly when checking for the image.""" - # weird dockerd behaviour - responses.add( - responses.GET, - LocalDockerdInterface.dockerd_socket_baseurl + "/images/json", - status=500, - ) - ldi = LocalDockerdInterface() - resp = ldi.get_image_info_from_digest("sha256:test-digest") - assert resp is None - - emitter.assert_debug("Bad response when validating local image: 500") - - -def test_localdockerinterface_get_info_by_digest_disconnected(emitter, responses): - """No daemon to talk to (see responses used as fixture but no listening).""" - ldi = LocalDockerdInterface() - resp = ldi.get_image_info_from_digest("sha256:test-digest") - assert resp is None - - emitter.assert_debug( - "Cannot connect to /var/run/docker.sock , please ensure dockerd is running." - ) - - -def test_localdockerinterface_get_streamed_content(responses): - """Get the content streamed.""" - - class AuditableBufferedReader(io.BufferedReader): - """BufferedReader that records the size of each reading.""" - - _test_read_chunks = [] - - def read(self, size): - self._test_read_chunks.append(size) - return super().read(size) - - test_content = AuditableBufferedReader(io.BytesIO(b"123456789")) - responses.add( - responses.GET, - LocalDockerdInterface.dockerd_socket_baseurl + "/images/test-id/get", - body=test_content, - ) - ldi = LocalDockerdInterface() - resp = ldi.get_streamed_image_content("test-id") - assert test_content._test_read_chunks == [] - - chunk_size = 5 - streamed = resp.iter_content(chunk_size) - assert next(streamed) == b"12345" - assert test_content._test_read_chunks == [chunk_size] - assert next(streamed) == b"6789" - assert test_content._test_read_chunks == [chunk_size, chunk_size] - with pytest.raises(StopIteration): - next(streamed) - - -class FakeRegistry: - """A fake registry to mimic behaviour of the real one and record actions.""" - - def __init__(self, image_name=None): - self.image_name = image_name - self.stored_manifests = {} - self.stored_blobs = {} - - def is_manifest_already_uploaded(self, reference): - return reference in self.stored_manifests - - def upload_manifest(self, content, reference, multiple_manifest=False): - self.stored_manifests[reference] = (content, multiple_manifest) - - def get_manifest(self, reference): - return self.stored_manifests[reference] - - def is_blob_already_uploaded(self, reference): - return reference in self.stored_blobs - - def upload_blob(self, filepath, size, digest): - self.stored_blobs[digest] = (pathlib.Path(filepath).read_bytes(), size) - - -class FakeDockerd: - """A fake dockerd interface to mimic behaviour of the real one.""" - - def __init__(self, image_id, image_info, image_content): - self.image_info = image_info - self.image_content = image_content - self.used_id = image_id - - def get_streamed_image_content(self, image_id): - assert image_id == self.used_id - - class FakeResponse: - def __init__(self, content): - self.content = io.BytesIO(content) - - def iter_content(self, chunk_size): - while True: - chunk = self.content.read(chunk_size) - if not chunk: - break - yield chunk - - return FakeResponse(self.image_content) - - -def test_imagehandler_check_in_registry_yes(): - """Check if an image is in the registry and find it.""" - fake_registry = FakeRegistry() - fake_registry.stored_manifests["test-reference"] = ( - None, - "test-digest", - "test-manifest", - ) - - im = ImageHandler(fake_registry) - result = im.check_in_registry("test-reference") - assert result is True - - -def test_imagehandler_check_in_registry_no(): - """Check if an image is in the registry and don't find it.""" - fake_registry = FakeRegistry() - - im = ImageHandler(fake_registry) - result = im.check_in_registry("test-reference") - assert result is False - - -def test_imagehandler_extract_file_simple(tmp_path, emitter): - """Extract a file from the tarfile and gets its info.""" - # create a tar file with one file inside - test_content = b"test content for the sample file" - sample_file = tmp_path / "testfile.txt" - sample_file.write_bytes(test_content) - tar_filepath = tmp_path / "testfile.tar" - with tarfile.open(tar_filepath, "w") as tar: - tar.add(sample_file, "testfile.txt") - - im = ImageHandler("registry") - with tarfile.open(tar_filepath, "r") as tar: - tmp_filepath, size, digest = im._extract_file(tar, "testfile.txt") - - assert size == len(test_content) - assert digest == "sha256:" + hashlib.sha256(test_content).hexdigest() - assert pathlib.Path(tmp_filepath).read_bytes() == test_content - - emitter.assert_progress("Extracting file 'testfile.txt' from local tar (compress=False)") - - -def test_imagehandler_extract_file_compressed_ok(tmp_path, emitter): - """Extract a file from the tarfile and gets its info after compressed.""" - # create a tar file with one file inside - test_content = b"test content for the sample file" - sample_file = tmp_path / "testfile.txt" - sample_file.write_bytes(test_content) - tar_filepath = tmp_path / "testfile.tar" - with tarfile.open(tar_filepath, "w") as tar: - tar.add(sample_file, "testfile.txt") - - im = ImageHandler("registry") - with tarfile.open(tar_filepath, "r") as tar: - tmp_filepath, size, digest = im._extract_file(tar, "testfile.txt", compress=True) - - compressed_content = pathlib.Path(tmp_filepath).read_bytes() - assert size == len(compressed_content) - assert digest == "sha256:" + hashlib.sha256(compressed_content).hexdigest() - assert gzip.decompress(compressed_content) == test_content - - emitter.assert_progress("Extracting file 'testfile.txt' from local tar (compress=True)") - - -def test_imagehandler_extract_file_compressed_deterministic(tmp_path, emitter): - """Different compressions for the same file give the exact same data.""" - # create a tar file with one file inside - test_content = b"test content for the sample file" - sample_file = tmp_path / "testfile.txt" - sample_file.write_bytes(test_content) - tar_filepath = tmp_path / "testfile.tar" - with tarfile.open(tar_filepath, "w") as tar: - tar.add(sample_file, "testfile.txt") - - im = ImageHandler("registry") - with tarfile.open(tar_filepath, "r") as tar: - _, _, digest1 = im._extract_file(tar, "testfile.txt", compress=True) - _, _, digest2 = im._extract_file(tar, "testfile.txt", compress=True) - - assert digest1 == digest2 - - -def test_imagehandler_uploadblob_first_time(emitter, tmp_path): - """Upload a blob for the first time.""" - tmp_file = tmp_path / "somebinary.dat" - tmp_file.write_text("testcontent") - - fake_registry = FakeRegistry() - - im = ImageHandler(fake_registry) - im._upload_blob(str(tmp_file), 20, "superdigest") - - # check it was uploaded - assert fake_registry.stored_blobs["superdigest"] == (b"testcontent", 20) - - # verify the file is cleaned - assert not tmp_file.exists() - - emitter.assert_interactions(None) - - -def test_imagehandler_uploadblob_duplicated(emitter, tmp_path): - """Upload a blob that was already there.""" - tmp_file = tmp_path / "somebinary.dat" - tmp_file.write_text("testcontent") - - fake_registry = FakeRegistry() - # add the entry for the blob, the value is not important - fake_registry.stored_blobs["superdigest"] = None - - im = ImageHandler(fake_registry) - im._upload_blob(str(tmp_file), 20, "superdigest") - - # check it was NOT uploaded again - assert fake_registry.stored_blobs["superdigest"] is None - - # verify the file is cleaned - assert not tmp_file.exists() - - emitter.assert_progress("Blob was already uploaded") - - -@pytest.mark.skipif(sys.platform == "win32", reason="Windows not [yet] supported") -def test_imagehandler_uploadfromlocal_complete(emitter, tmp_path, responses, monkeypatch): - """Complete process of uploading a local image.""" - # fake an image in disk (a tar file with config, layers, and a manifest).""" - test_tar_image = tmp_path / "test-image.tar" - test_tar_config_content = b"fake config for the image" - test_tar_layer1_content = b"fake first layer content for the image" - test_tar_layer2_content = b"fake second layer content for the image" - test_manifest_content = json.dumps( - [ - { - "Config": const.JUJU_CONFIG_FILENAME, - "Layers": ["layer1.bin", "layer2.bin"], - } - ] - ).encode("ascii") - tar_file = tarfile.TarFile(test_tar_image, "w") - tar_content = [ - ("manifest.json", test_manifest_content), - (const.JUJU_CONFIG_FILENAME, test_tar_config_content), - ("layer1.bin", test_tar_layer1_content), - ("layer2.bin", test_tar_layer2_content), - ] - for name, content in tar_content: - ti = tarfile.TarInfo(name) - ti.size = len(content) - tar_file.addfile(ti, fileobj=io.BytesIO(content)) - tar_file.close() - - # prepare the image info - image_size = test_tar_image.stat().st_size - image_id = "test-image-id" - image_info = {"Size": image_size, "Id": image_id, "foobar": "etc"} - fakedockerd = FakeDockerd(image_id, image_info, test_tar_image.read_bytes()) - monkeypatch.setattr(registry, "LocalDockerdInterface", lambda: fakedockerd) - - # ensure two reads from that image, so we can properly test progress - image_read_from_dockerd_size_1 = int(image_size * 0.7) - image_read_from_dockerd_size_2 = image_size - image_read_from_dockerd_size_1 - monkeypatch.setattr(registry, "CHUNK_SIZE", image_read_from_dockerd_size_1) - - fake_registry = FakeRegistry() - im = ImageHandler(fake_registry) - main_call_result = im.upload_from_local(image_info) - - # check the uploaded blobs: first the config (as is), then the layers (compressed) - ( - uploaded_config, - uploaded_layer1, - uploaded_layer2, - ) = fake_registry.stored_blobs.items() - - (u_config_digest, (u_config_content, u_config_size)) = uploaded_config - assert u_config_content == test_tar_config_content - assert u_config_size == len(u_config_content) - assert u_config_digest == "sha256:" + hashlib.sha256(u_config_content).hexdigest() - - (u_layer1_digest, (u_layer1_content, u_layer1_size)) = uploaded_layer1 - assert gzip.decompress(u_layer1_content) == test_tar_layer1_content - assert u_layer1_size == len(u_layer1_content) - assert u_layer1_digest == "sha256:" + hashlib.sha256(u_layer1_content).hexdigest() - - (u_layer2_digest, (u_layer2_content, u_layer2_size)) = uploaded_layer2 - assert gzip.decompress(u_layer2_content) == test_tar_layer2_content - assert u_layer2_size == len(u_layer2_content) - assert u_layer2_digest == "sha256:" + hashlib.sha256(u_layer2_content).hexdigest() - - # check the uploaded manifest metadata and real content - (uploaded_manifest,) = fake_registry.stored_manifests.items() - (u_manifest_digest, (u_manifest_content, u_manifest_multiple)) = uploaded_manifest - assert ( - u_manifest_digest - == "sha256:" + hashlib.sha256(u_manifest_content.encode("utf8")).hexdigest() - ) - assert u_manifest_multiple is False - - # the response from the function we're testing is the final remote digest - assert main_call_result == u_manifest_digest - - u_manifest = json.loads(u_manifest_content) - assert u_manifest["mediaType"] == MANIFEST_V2_MIMETYPE - assert u_manifest["schemaVersion"] == 2 - - assert u_manifest["config"] == { - "digest": u_config_digest, - "mediaType": CONFIG_MIMETYPE, - "size": u_config_size, - } - - assert u_manifest["layers"] == [ - { - "digest": u_layer1_digest, - "mediaType": LAYER_MIMETYPE, - "size": u_layer1_size, - }, - { - "digest": u_layer2_digest, - "mediaType": LAYER_MIMETYPE, - "size": u_layer2_size, - }, - ] - - # check the output logs - emitter.assert_interactions( - [ - call("progress", f"Getting the image from the local repo; size={image_size}"), - call("progress_bar", "Reading image...", image_size), - call("advance", image_read_from_dockerd_size_1), - call("advance", image_read_from_dockerd_size_2), - call("progress", "Extracting file 'config.yaml' from local tar (compress=False)"), - call( - "progress", - f"Uploading config blob, size={u_config_size}, digest={u_config_digest}", - ), - call("progress", "Extracting file 'layer1.bin' from local tar (compress=True)"), - call( - "progress", - f"Uploading layer blob 1/2, size={u_layer1_size}, digest={u_layer1_digest}", - ), - call("progress", "Extracting file 'layer2.bin' from local tar (compress=True)"), - call( - "progress", - f"Uploading layer blob 2/2, size={u_layer2_size}, digest={u_layer2_digest}", - ), - ] - ) - - -@pytest.mark.skipif(sys.platform == "win32", reason="Windows not [yet] supported") -def test_imagehandler_uploadfromlocal_no_config(emitter, tmp_path, monkeypatch): - """Particular case of a manifest without config.""" - # fake an image in disk (a tar file with NO config, a layer, and a manifest).""" - test_tar_image = tmp_path / "test-image.tar" - test_tar_layer_content = b"fake layer content for the image" - test_manifest_content = json.dumps( - [ - { - "Layers": ["layer.bin"], - } - ] - ).encode("ascii") - tar_file = tarfile.TarFile(test_tar_image, "w") - tar_content = [ - ("manifest.json", test_manifest_content), - ("layer.bin", test_tar_layer_content), - ] - for name, content in tar_content: - ti = tarfile.TarInfo(name) - ti.size = len(content) - tar_file.addfile(ti, fileobj=io.BytesIO(content)) - tar_file.close() - - # return 200 with the image info - image_size = test_tar_image.stat().st_size - image_id = "test-image-id" - image_info = {"Size": image_size, "Id": image_id, "foobar": "etc"} - fakedockerd = FakeDockerd(image_id, image_info, test_tar_image.read_bytes()) - monkeypatch.setattr(registry, "LocalDockerdInterface", lambda: fakedockerd) - - fake_registry = FakeRegistry() - im = ImageHandler(fake_registry) - main_call_result = im.upload_from_local(image_info) - - # check the uploaded blob: just the compressed layer - (uploaded_layer,) = fake_registry.stored_blobs.items() - - (u_layer_digest, (u_layer_content, u_layer_size)) = uploaded_layer - assert gzip.decompress(u_layer_content) == test_tar_layer_content - assert u_layer_size == len(u_layer_content) - assert u_layer_digest == "sha256:" + hashlib.sha256(u_layer_content).hexdigest() - - # check the uploaded manifest metadata and real content - (uploaded_manifest,) = fake_registry.stored_manifests.items() - (u_manifest_digest, (u_manifest_content, u_manifest_multiple)) = uploaded_manifest - assert ( - u_manifest_digest - == "sha256:" + hashlib.sha256(u_manifest_content.encode("utf8")).hexdigest() - ) - assert u_manifest_multiple is False - - # the response from the function we're testing is the final remote digest - assert main_call_result == u_manifest_digest - - u_manifest = json.loads(u_manifest_content) - assert u_manifest["mediaType"] == MANIFEST_V2_MIMETYPE - assert u_manifest["schemaVersion"] == 2 - - assert "config" not in u_manifest - assert u_manifest["layers"] == [ - { - "digest": u_layer_digest, - "mediaType": LAYER_MIMETYPE, - "size": u_layer_size, - } - ] - - # check the output logs - emitter.assert_interactions( - [ - call("progress", f"Getting the image from the local repo; size={image_size}"), - call("progress_bar", "Reading image...", image_size), - call("advance", image_size), - call("progress", "Extracting file 'layer.bin' from local tar (compress=True)"), - call( - "progress", - f"Uploading layer blob 1/1, size={u_layer_size}, digest={u_layer_digest}", - ), - ] - ) From 74f79ec099b76967f56cf0fe4b09e1ba951e56b6 Mon Sep 17 00:00:00 2001 From: Alex Lowe Date: Tue, 17 Sep 2024 16:23:43 -0400 Subject: [PATCH 4/5] style(types): stricter type checking --- charmcraft/charm_builder.py | 4 +--- pyproject.toml | 10 +--------- tests/integration/commands/test_init.py | 10 ++++++---- tests/unit/models/test_config.py | 2 +- tests/unit/utils/test_cli.py | 2 +- 5 files changed, 10 insertions(+), 18 deletions(-) diff --git a/charmcraft/charm_builder.py b/charmcraft/charm_builder.py index aa279d6f2..9489317a0 100644 --- a/charmcraft/charm_builder.py +++ b/charmcraft/charm_builder.py @@ -60,7 +60,6 @@ def __init__( builddir: pathlib.Path, installdir: pathlib.Path, entrypoint: pathlib.Path, - allow_pip_binary: bool = None, binary_python_packages: list[str] | None = None, python_packages: list[str] | None = None, requirements: list[pathlib.Path] | None = None, @@ -69,7 +68,6 @@ def __init__( self.builddir = builddir self.installdir = installdir self.entrypoint = entrypoint - self.allow_pip_binary = allow_pip_binary self.binary_python_packages = binary_python_packages or [] self.python_packages = python_packages or [] self.requirement_paths = requirements or [] @@ -476,4 +474,4 @@ def main(): if __name__ == "__main__": with instrum.Timer("Full charm_builder.py main"): main() - instrum.dump(get_charm_builder_metrics_path()) + instrum.dump(get_charm_builder_metrics_path().as_posix()) diff --git a/pyproject.toml b/pyproject.toml index 7645ca2aa..257174d4b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -165,13 +165,12 @@ include = [ "tests/unit", "tests/integration", ] -analyzeUnannotatedFunctions = false +analyzeUnannotatedFunctions = true reportArgumentType = "warning" reportAttributeAccessIssue = "warning" reportIncompatibleVariableOverride = "warning" reportOptionalMemberAccess = "warning" - [tool.mypy] python_version = "3.10" packages = [ @@ -198,17 +197,10 @@ disallow_untyped_decorators = true # Ignore typing errors in most legacy packages until we fix them. module=[ "charmcraft.charm_builder", - "charmcraft.cmdbase", - "charmcraft.commands.extensions", - "charmcraft.commands.pack", - "charmcraft.commands.store", - "charmcraft.store.registry", "charmcraft.store.store", "charmcraft.extensions._utils", "charmcraft.linters", "charmcraft.models.charmcraft", - "charmcraft.package", - "charmcraft.providers", ] ignore_errors = true diff --git a/tests/integration/commands/test_init.py b/tests/integration/commands/test_init.py index a4f632809..2992417c7 100644 --- a/tests/integration/commands/test_init.py +++ b/tests/integration/commands/test_init.py @@ -29,12 +29,14 @@ import pytest_check import charmcraft -from charmcraft import errors +from charmcraft import application, errors from charmcraft.application import commands from charmcraft.utils import S_IXALL -with contextlib.suppress(ImportError): +try: import pwd +except ImportError: + pwd = None BASIC_INIT_FILES = frozenset( pathlib.Path(p) @@ -96,8 +98,8 @@ @pytest.fixture -def init_command(): - return commands.InitCommand({"app": charmcraft.application.APP_METADATA, "services": None}) +def init_command() -> commands.InitCommand: + return commands.InitCommand({"app": application.APP_METADATA, "services": None}) def create_namespace( diff --git a/tests/unit/models/test_config.py b/tests/unit/models/test_config.py index e929d848e..d4ccded5f 100644 --- a/tests/unit/models/test_config.py +++ b/tests/unit/models/test_config.py @@ -66,7 +66,7 @@ def test_empty_config(): def test_correct_option_type(option, type_): config = JujuConfig(options={"my-opt": option}) - assert isinstance(config.options["my-opt"], type_) + assert isinstance(config.options["my-opt"], type_) # pyright: ignore[reportOptionalSubscript] @pytest.mark.parametrize( diff --git a/tests/unit/utils/test_cli.py b/tests/unit/utils/test_cli.py index 971f36795..3b3e48748 100644 --- a/tests/unit/utils/test_cli.py +++ b/tests/unit/utils/test_cli.py @@ -279,4 +279,4 @@ def test_format_content_table(content): @pytest.mark.parametrize("fmt", ["yolo", 0]) def test_format_content_invalid(fmt): with pytest.raises(ValueError, match="^Unknown output format "): - format_content(None, fmt) + format_content(None, fmt) # pyright: ignore[reportCallIssue] From 008a14a74f6c58c04cf5ed460e6ba4e3c761e1c0 Mon Sep 17 00:00:00 2001 From: Alex Lowe Date: Tue, 17 Sep 2024 16:54:23 -0400 Subject: [PATCH 5/5] style(type): enable argument type linting --- charmcraft/application/commands/analyse.py | 4 ++-- charmcraft/application/commands/store.py | 4 ++-- charmcraft/extensions/_utils.py | 2 +- charmcraft/instrum.py | 7 ++++--- charmcraft/jujuignore.py | 2 +- charmcraft/models/project.py | 6 ++++-- charmcraft/parts/plugins/_charm.py | 4 +++- charmcraft/store/store.py | 4 ++-- charmcraft/utils/cli.py | 2 +- pyproject.toml | 1 - tests/factory.py | 7 +++++-- tests/integration/commands/test_init.py | 4 +--- tests/unit/models/test_metadata.py | 5 +++-- tests/unit/services/test_analysis.py | 4 +++- tests/unit/services/test_package.py | 8 +++++--- tests/unit/services/test_store.py | 4 +++- 16 files changed, 40 insertions(+), 28 deletions(-) diff --git a/charmcraft/application/commands/analyse.py b/charmcraft/application/commands/analyse.py index 8b160efe8..922b13fb6 100644 --- a/charmcraft/application/commands/analyse.py +++ b/charmcraft/application/commands/analyse.py @@ -69,13 +69,13 @@ def run(self, parsed_args: argparse.Namespace) -> int: return self._run_formatted(parsed_args.filepath, ignore=ignore) return self._run_streaming(parsed_args.filepath, ignore=ignore) - def _run_formatted(self, filepath: pathlib.Path, *, ignore=Container[str]) -> int: + def _run_formatted(self, filepath: pathlib.Path, *, ignore: Container[str]) -> int: """Run the command, formatting the output into JSON or similar at the end.""" results = list(self._services.analysis.lint_file(filepath)) emit.message(json.dumps(results, indent=4, default=pydantic_encoder)) return max(r.level for r in results).return_code - def _run_streaming(self, filepath: pathlib.Path, *, ignore=Container[str]) -> int: + def _run_streaming(self, filepath: pathlib.Path, *, ignore: Container[str]) -> int: """Run the command, printing linter results as we get them.""" max_level = lint.ResultLevel.OK with emit.progress_bar( diff --git a/charmcraft/application/commands/store.py b/charmcraft/application/commands/store.py index 5e76d80cf..a99ded360 100644 --- a/charmcraft/application/commands/store.py +++ b/charmcraft/application/commands/store.py @@ -259,7 +259,7 @@ def run(self, parsed_args): return human_msgs = [] - prog_info = {"logged": True} + prog_info: dict[str, Any] = {"logged": True} human_msgs.append(f"name: {macaroon_info['account']['display-name']}") prog_info["name"] = macaroon_info["account"]["display-name"] @@ -281,7 +281,7 @@ def run(self, parsed_args): for package_type, title in [("charm", "charms"), ("bundle", "bundles")]: if package_type in grouped: human_msgs.append(f"{title}:") - pkg_info = [] + pkg_info: list[dict[str, str]] = [] for item in grouped[package_type]: if item.name is not None: human_msgs.append(f"- name: {item.name}") diff --git a/charmcraft/extensions/_utils.py b/charmcraft/extensions/_utils.py index 3a816ad2b..bc0d08011 100644 --- a/charmcraft/extensions/_utils.py +++ b/charmcraft/extensions/_utils.py @@ -78,7 +78,7 @@ def _apply_extension( def _apply_extension_property( - existing_property: dict | list, extension_property: dict | list + existing_property: dict | list | None, extension_property: dict | list ) -> dict | list: if existing_property: # If the property is not scalar, merge them diff --git a/charmcraft/instrum.py b/charmcraft/instrum.py index e83bb8578..a3164d62f 100644 --- a/charmcraft/instrum.py +++ b/charmcraft/instrum.py @@ -1,4 +1,4 @@ -# Copyright 2022 Canonical Ltd. +# Copyright 2022,2024 Canonical Ltd. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -17,6 +17,7 @@ """Provide utilities to measure performance in different parts of the app.""" import json +import pathlib import uuid from time import time from typing import Any @@ -34,7 +35,7 @@ def __init__(self): # ancestors list when a measure starts (last item is direct parent); the # first value is special, None, to reflect the "root", the rest are # measurement ids - self.parents = [None] # start with a unique "root" + self.parents: list[str | None] = [None] # start with a unique "root" # simple dict to hold measurements information; the key is the measurement # id and each value holds all it info @@ -74,7 +75,7 @@ def dump(self, filename: str) -> None: with open(filename, "w") as fh: json.dump(measurements, fh, indent=4) - def merge_from(self, filename: str) -> None: + def merge_from(self, filename: str | pathlib.Path) -> None: """Merge measurements from a file to the current ongoing structure.""" with open(filename) as fh: to_merge = json.load(fh) diff --git a/charmcraft/jujuignore.py b/charmcraft/jujuignore.py index d715859b3..fec99aea5 100644 --- a/charmcraft/jujuignore.py +++ b/charmcraft/jujuignore.py @@ -126,7 +126,7 @@ def __init__( orig_rule: str, invert: bool, only_dirs: bool, - regex: typing.Pattern, + regex: re.Pattern[str] | str, ): self.line_num = line_num self.orig_rule = orig_rule diff --git a/charmcraft/models/project.py b/charmcraft/models/project.py index 9930b612a..f1352b894 100644 --- a/charmcraft/models/project.py +++ b/charmcraft/models/project.py @@ -15,6 +15,8 @@ # For further info, check https://github.com/canonical/charmcraft """Project-related models for Charmcraft.""" +from __future__ import annotations + import abc import datetime import pathlib @@ -433,7 +435,7 @@ def started_at(self) -> datetime.datetime: return self._started_at @classmethod - def unmarshal(cls, data: dict[str, Any]): + def unmarshal(cls, data: dict[str, Any]) -> CharmcraftProject: """Create a Charmcraft project from a dictionary of data.""" if cls is not CharmcraftProject: return cls.model_validate(data) @@ -447,7 +449,7 @@ def unmarshal(cls, data: dict[str, Any]): raise ValueError(f"field type cannot be {project_type!r}") @classmethod - def from_yaml_file(cls, path: pathlib.Path) -> Self: + def from_yaml_file(cls, path: pathlib.Path) -> CharmcraftProject: """Instantiate this model from a YAML file. For use with craft-application. diff --git a/charmcraft/parts/plugins/_charm.py b/charmcraft/parts/plugins/_charm.py index fcfa4b249..209c2a864 100644 --- a/charmcraft/parts/plugins/_charm.py +++ b/charmcraft/parts/plugins/_charm.py @@ -22,6 +22,7 @@ from contextlib import suppress from typing import Literal, cast +import craft_parts import overrides import pydantic from craft_parts import Step, callbacks, plugins @@ -346,9 +347,10 @@ def _get_legacy_dependencies_parameters(self) -> list[str]: return parameters - def post_build_callback(self, step_info): + def post_build_callback(self, step_info: craft_parts.StepInfo) -> Literal[False]: """Collect metrics left by charm_builder.py.""" instrum.merge_from(env.get_charm_builder_metrics_path()) + return False def _get_os_special_priority_paths(self) -> str | None: """Return a str of PATH for special OS.""" diff --git a/charmcraft/store/store.py b/charmcraft/store/store.py index 2e8517734..c599e5c85 100644 --- a/charmcraft/store/store.py +++ b/charmcraft/store/store.py @@ -85,7 +85,7 @@ def _build_errors(item): def _build_revision(item: dict[str, Any]) -> Revision: """Build a Revision from a response item.""" - bases = [(None if base is None else Base(**base)) for base in item["bases"]] + bases = [Base(**base) for base in item["bases"] if base is not None] return Revision( revision=item["revision"], version=item["version"], @@ -387,7 +387,7 @@ def list_releases(self, name: str) -> tuple[list[Release], list[Channel], list[R channel=item["channel"], expires_at=expires_at, resources=resources, - base=base, + base=base, # pyright: ignore[reportArgumentType] ) ) diff --git a/charmcraft/utils/cli.py b/charmcraft/utils/cli.py index 1f1044ec3..539ca6024 100644 --- a/charmcraft/utils/cli.py +++ b/charmcraft/utils/cli.py @@ -187,7 +187,7 @@ def format_content(content: dict[str, str], fmt: Literal[OutputFormat.TABLE, "ta @overload def format_content( - content: str | (numbers.Real | (list | dict)), fmt: OutputFormat | (str | None) + content: str | (numbers.Real | (list | dict)) | None, fmt: OutputFormat | (str | None) ) -> str: ... diff --git a/pyproject.toml b/pyproject.toml index 257174d4b..48cd7bf5b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -166,7 +166,6 @@ include = [ "tests/integration", ] analyzeUnannotatedFunctions = true -reportArgumentType = "warning" reportAttributeAccessIssue = "warning" reportIncompatibleVariableOverride = "warning" reportOptionalMemberAccess = "warning" diff --git a/tests/factory.py b/tests/factory.py index a99babc1e..3270b2b96 100644 --- a/tests/factory.py +++ b/tests/factory.py @@ -18,11 +18,14 @@ import pathlib import textwrap +from typing import cast from charmcraft.utils import create_importable_name, get_lib_info -def create_lib_filepath(charm_name, lib_name, api=0, patch=1, lib_id="test-lib-id"): +def create_lib_filepath( + charm_name: str, lib_name: str, api: int = 0, patch: int = 1, lib_id: str = "test-lib-id" +) -> tuple[str, str]: """Helper to create the structures on disk for a given lib.""" charm_name = create_importable_name(charm_name) base_dir = pathlib.Path("lib") @@ -46,4 +49,4 @@ def create_lib_filepath(charm_name, lib_name, api=0, patch=1, lib_id="test-lib-i # use get_lib_info to get the hash of the file, as the used hash is WITHOUT the metadata # files (no point in duplicating that logic here) libdata = get_lib_info(lib_path=lib_file) - return content, libdata.content_hash + return content, cast(str, libdata.content_hash) diff --git a/tests/integration/commands/test_init.py b/tests/integration/commands/test_init.py index 2992417c7..c6f42d645 100644 --- a/tests/integration/commands/test_init.py +++ b/tests/integration/commands/test_init.py @@ -15,7 +15,6 @@ # For further info, check https://github.com/canonical/charmcraft """Tests for init command.""" import argparse -import contextlib import os import pathlib import re @@ -28,7 +27,6 @@ import pytest import pytest_check -import charmcraft from charmcraft import application, errors from charmcraft.application import commands from charmcraft.utils import S_IXALL @@ -105,7 +103,7 @@ def init_command() -> commands.InitCommand: def create_namespace( *, name="my-charm", - author="J Doe", + author: str | None = "J Doe", force=False, profile=commands.init.DEFAULT_PROFILE, project_dir: pathlib.Path | None = None, diff --git a/tests/unit/models/test_metadata.py b/tests/unit/models/test_metadata.py index 63fdeaa50..43350300e 100644 --- a/tests/unit/models/test_metadata.py +++ b/tests/unit/models/test_metadata.py @@ -15,6 +15,7 @@ # For further info, check https://github.com/canonical/charmcraft """Tests for metadata models.""" import json +from typing import cast import pytest @@ -80,7 +81,7 @@ ], ) def test_charm_metadata_from_charm_success(charm_dict, expected): - charm = project.CharmcraftProject.unmarshal(charm_dict) + charm = cast(project.Charm, project.CharmcraftProject.unmarshal(charm_dict)) assert json.loads(json.dumps(metadata.CharmMetadata.from_charm(charm).marshal())) == expected @@ -92,7 +93,7 @@ def test_charm_metadata_from_charm_success(charm_dict, expected): ], ) def test_bundle_metadata_from_bundle(bundle_dict, expected): - bundle = project.Bundle.unmarshal(BASIC_BUNDLE_DICT) + bundle = cast(project.Bundle, project.Bundle.unmarshal(BASIC_BUNDLE_DICT)) assert ( json.loads(json.dumps(metadata.BundleMetadata.from_bundle(bundle).marshal())) == expected diff --git a/tests/unit/services/test_analysis.py b/tests/unit/services/test_analysis.py index d3c3d9eb4..cde932c46 100644 --- a/tests/unit/services/test_analysis.py +++ b/tests/unit/services/test_analysis.py @@ -99,7 +99,9 @@ def mock_zip_file(monkeypatch): @pytest.fixture def analysis_service(): - return analysis.AnalysisService(app=application.APP_METADATA, services=None) + return analysis.AnalysisService( + app=application.APP_METADATA, services=None # pyright: ignore[reportArgumentType] + ) @pytest.mark.parametrize( diff --git a/tests/unit/services/test_package.py b/tests/unit/services/test_package.py index 9ac506243..5de845534 100644 --- a/tests/unit/services/test_package.py +++ b/tests/unit/services/test_package.py @@ -37,9 +37,11 @@ charmcraft_started_at="1970-01-01T00:00:00+00:00", bases=[SIMPLE_BUILD_BASE], ) -MANIFEST_WITH_ATTRIBUTE = models.Manifest( - **SIMPLE_MANIFEST.marshal(), - analysis={"attributes": [models.Attribute(name="boop", result="success")]}, +MANIFEST_WITH_ATTRIBUTE = models.Manifest.unmarshal( + { + **SIMPLE_MANIFEST.marshal(), + "analysis": {"attributes": [models.Attribute(name="boop", result="success")]}, + }, ) diff --git a/tests/unit/services/test_store.py b/tests/unit/services/test_store.py index 8407d6ce4..178a05867 100644 --- a/tests/unit/services/test_store.py +++ b/tests/unit/services/test_store.py @@ -42,7 +42,9 @@ def store(service_factory) -> services.StoreService: @pytest.fixture(scope="module") def reusable_store(): - store = services.StoreService(app=application.APP_METADATA, services=None) + store = services.StoreService( + app=application.APP_METADATA, services=None # pyright: ignore[reportArgumentType] + ) store.client = mock.Mock(spec_set=craft_store.StoreClient) return store