From 87031f37da067f0223f8d5d0481f62f696d74557 Mon Sep 17 00:00:00 2001 From: elkal98 Date: Sun, 19 Oct 2025 15:49:49 +0300 Subject: [PATCH 1/2] Bug 1968280 - Draft: Landoscript should fetch file modes for newly added or files being removed. --- .../actions/android_l10n_import.py | 6 +- .../landoscript/actions/android_l10n_sync.py | 6 +- .../src/landoscript/actions/l10n_bump.py | 2 +- .../src/landoscript/actions/merge_day.py | 6 +- .../src/landoscript/actions/version_bump.py | 2 +- landoscript/src/landoscript/util/diffs.py | 6 +- landoscript/tests/conftest.py | 6 +- .../pytest_scriptworker_client/__init__.py | 6 ++ .../src/scriptworker_client/github_client.py | 25 ++++-- .../tests/test_github_client.py | 90 +++++++++++++++---- .../src/treescript/github/versionmanip.py | 2 +- 11 files changed, 115 insertions(+), 42 deletions(-) diff --git a/landoscript/src/landoscript/actions/android_l10n_import.py b/landoscript/src/landoscript/actions/android_l10n_import.py index 2d8ab183c..54251bb43 100644 --- a/landoscript/src/landoscript/actions/android_l10n_import.py +++ b/landoscript/src/landoscript/actions/android_l10n_import.py @@ -47,7 +47,7 @@ async def run( async with GithubClient(github_config, l10n_owner, l10n_repo) as l10n_github_client: toml_files = [info.toml_path for info in android_l10n_import_info.toml_info] # we always take the tip of the default branch when importing new strings - toml_contents = await l10n_github_client.get_files(toml_files) + toml_contents = await l10n_github_client.get_files(toml_files, mode=None) l10n_files: list[L10nFile] = [] missing = [fn for fn, contents in toml_contents.items() if contents is None] @@ -75,7 +75,7 @@ async def run( # fetch l10n_files from android-l10n src_files = [f.src_name for f in l10n_files] log.info(f"fetching updated files from l10n repository: {src_files}") - new_files = await l10n_github_client.get_files(src_files) + new_files = await l10n_github_client.get_files(src_files, mode=None) # we also need to update the toml files with locale metadata. we've # already fetched them above; so just add their contents by hand new_files.update(toml_contents) @@ -88,7 +88,7 @@ async def run( dst_files.append(f"{toml_info.dest_path}/l10n.toml") log.info(f"fetching original files from l10n repository: {dst_files}") - orig_files = await github_client.get_files(dst_files, branch=to_branch) + orig_files = await github_client.get_files(dst_files, branch=to_branch, mode=None) files_to_diff = [] for l10n_file in l10n_files: diff --git a/landoscript/src/landoscript/actions/android_l10n_sync.py b/landoscript/src/landoscript/actions/android_l10n_sync.py index 4df6fb711..a92161a09 100644 --- a/landoscript/src/landoscript/actions/android_l10n_sync.py +++ b/landoscript/src/landoscript/actions/android_l10n_sync.py @@ -39,7 +39,7 @@ async def run(github_client: GithubClient, public_artifact_dir: str, android_l10 from_branch = android_l10n_sync_info.from_branch toml_files = [info.toml_path for info in android_l10n_sync_info.toml_info] - toml_contents = await github_client.get_files(toml_files, branch=from_branch) + toml_contents = await github_client.get_files(toml_files, branch=from_branch, mode=None) l10n_files: list[L10nFile] = [] missing = [fn for fn, contents in toml_contents.items() if contents is None] @@ -67,7 +67,7 @@ async def run(github_client: GithubClient, public_artifact_dir: str, android_l10 # fetch l10n_files from `from_branch` in the gecko repo src_files = [f.src_name for f in l10n_files] log.info(f"fetching updated files from l10n repository: {src_files}") - new_files = await github_client.get_files(src_files, branch=from_branch) + new_files = await github_client.get_files(src_files, branch=from_branch, mode=None) # we also need to update the toml files with locale metadata. we've # already fetched them above; so just add their contents by hand new_files.update(toml_contents) @@ -80,7 +80,7 @@ async def run(github_client: GithubClient, public_artifact_dir: str, android_l10 dst_files.append(toml_info.toml_path) log.info(f"fetching original files from l10n repository: {dst_files}") - orig_files = await github_client.get_files(dst_files, branch=to_branch) + orig_files = await github_client.get_files(dst_files, branch=to_branch, mode=None) files_to_diff = [] for l10n_file in l10n_files: diff --git a/landoscript/src/landoscript/actions/l10n_bump.py b/landoscript/src/landoscript/actions/l10n_bump.py index 36a5905b2..7741991bb 100644 --- a/landoscript/src/landoscript/actions/l10n_bump.py +++ b/landoscript/src/landoscript/actions/l10n_bump.py @@ -66,7 +66,7 @@ async def run( files = [bump_config.path, *platform_config_files] try: log.info(f"fetching bump files from github: {files}") - orig_files = await github_client.get_files(files, branch) + orig_files = await github_client.get_files(files, branch, mode=None) except TransportError as e: raise LandoscriptError("couldn't retrieve bump files from github") from e diff --git a/landoscript/src/landoscript/actions/merge_day.py b/landoscript/src/landoscript/actions/merge_day.py index ba502d282..b8d60c7a2 100644 --- a/landoscript/src/landoscript/actions/merge_day.py +++ b/landoscript/src/landoscript/actions/merge_day.py @@ -161,7 +161,7 @@ async def run( for r in regex_replacements: needed_files.append(r[0]) - orig_contents = await github_client.get_files(needed_files, bump_branch) + orig_contents = await github_client.get_files(needed_files, bump_branch, mode=None) # At the moment, there are no known cases of needing to replace with # a suffix...so we simply don't handle that here! new_contents = process_replacements(bump_version, replacements, regex_replacements, orig_contents) @@ -171,7 +171,7 @@ async def run( files_to_diff.append((fn, str(orig_contents[fn]), new_contents[fn])) log.info("Touching clobber file") - orig_clobber_file = (await github_client.get_files("CLOBBER", bump_branch))["CLOBBER"] + orig_clobber_file = (await github_client.get_files("CLOBBER", bump_branch, mode=None))["CLOBBER"] if orig_clobber_file is None: raise LandoscriptError("Couldn't find CLOBBER file in repository!") @@ -206,7 +206,7 @@ async def run( async def get_version(github_client: GithubClient, version_file: str, branch: str): - resp = await github_client.get_files(version_file, branch) + resp = await github_client.get_files(version_file, branch, mode=None) contents = resp[version_file] if contents is None: raise LandoscriptError(f"Couldn't find {version_file} in repository!") diff --git a/landoscript/src/landoscript/actions/version_bump.py b/landoscript/src/landoscript/actions/version_bump.py index d8f60641a..365fe6dca 100644 --- a/landoscript/src/landoscript/actions/version_bump.py +++ b/landoscript/src/landoscript/actions/version_bump.py @@ -60,7 +60,7 @@ async def run( try: log.info("fetching bump files from github") - orig_files = await github_client.get_files(version_bump_info.files, branch) + orig_files = await github_client.get_files(version_bump_info.files, branch, mode=None) except TransportError as e: raise LandoscriptError("couldn't retrieve bump files from github") from e diff --git a/landoscript/src/landoscript/util/diffs.py b/landoscript/src/landoscript/util/diffs.py index d922bf105..cb73314b4 100644 --- a/landoscript/src/landoscript/util/diffs.py +++ b/landoscript/src/landoscript/util/diffs.py @@ -3,7 +3,7 @@ NO_NEWLINE_SUFFIX = "\\ No newline at end of file" -def diff_contents(orig, modified, file): +def diff_contents(orig, modified, file, mode=None): """Create a git-style unified diff of `orig` and `modified` with the filename `file`.""" add_remove_metadata = "" if orig: @@ -14,7 +14,7 @@ def diff_contents(orig, modified, file): # orig does not exist yet; ie: it will be added orig_contents = "" fromfile = "/dev/null" - add_remove_metadata = "new file mode 100644\n" + add_remove_metadata = f"new file mode {mode or 100644}\n" if modified: # modified exists already modified_contents = modified.splitlines() @@ -23,7 +23,7 @@ def diff_contents(orig, modified, file): # modified does not exist yet; ie: it will be added modified_contents = "" tofile = "/dev/null" - add_remove_metadata = "deleted file mode 100644\n" + add_remove_metadata = f"deleted file mode {mode or 100644}\n" diff_lines = [line for line in unified_diff(orig_contents, modified_contents, fromfile=fromfile, tofile=tofile, lineterm="")] if not diff_lines: diff --git a/landoscript/tests/conftest.py b/landoscript/tests/conftest.py index c552fb437..d2f82d463 100644 --- a/landoscript/tests/conftest.py +++ b/landoscript/tests/conftest.py @@ -219,7 +219,7 @@ def assert_tag_response(req, tag_info, target_revision): assert revisions.pop() == target_revision -def assert_add_commit_response(action, commit_msg_strings, initial_values, expected_bumps): +def assert_add_commit_response(action, commit_msg_strings, initial_values, expected_bumps, mode=None): # ensure metadata is correct assert action["author"] == "Release Engineering Landoscript " # we don't actually verify the value here; it's not worth the trouble of mocking @@ -261,12 +261,12 @@ def assert_add_commit_response(action, commit_msg_strings, initial_values, expec # the diff. if not before: # addition - assert "new file mode 100644" in diff + assert f"new file mode {mode or 100644}" in diff if "\n" not in after and f"+{after}" in diff: break elif not after: # removal - assert "deleted file mode 100644" in diff + assert f"deleted file mode {mode or 100644}" in diff if not "\n" in before and f"-{before}" in diff: break else: diff --git a/scriptworker_client/packages/pytest-scriptworker-client/src/pytest_scriptworker_client/__init__.py b/scriptworker_client/packages/pytest-scriptworker-client/src/pytest_scriptworker_client/__init__.py index 620fdd841..91af97775 100644 --- a/scriptworker_client/packages/pytest-scriptworker-client/src/pytest_scriptworker_client/__init__.py +++ b/scriptworker_client/packages/pytest-scriptworker-client/src/pytest_scriptworker_client/__init__.py @@ -58,6 +58,12 @@ def get_files_payload(file_contents={}): hash_ = "a" + hashlib.md5(file.encode()).hexdigest() if contents is None: payload["data"]["repository"][hash_] = None + elif isinstance(contents, dict): + payload["data"]["repository"][hash_] = { + "mode": contents.get("mode"), + "text": contents.get("text") + } + else: payload["data"]["repository"][hash_] = {"text": contents} else: diff --git a/scriptworker_client/src/scriptworker_client/github_client.py b/scriptworker_client/src/scriptworker_client/github_client.py index 444e980d3..87d3f1e69 100644 --- a/scriptworker_client/src/scriptworker_client/github_client.py +++ b/scriptworker_client/src/scriptworker_client/github_client.py @@ -93,7 +93,7 @@ async def _execute(): await retry_async(_execute, attempts=3, retry_exceptions=(TransportQueryError,), sleeptime_kwargs={"delay_factor": 0}) - async def get_files(self, files: Union[str, List[str]], branch: Optional[str] = None, files_per_request: int = 200) -> Dict[str, Union[str, None]]: + async def get_files(self, files: Union[str, List[str]], branch: Optional[str] = None, files_per_request: int = 200, mode: Optional[str] = None) -> Dict[str, Union[str, Dict[str, Optional[str]]]]: """Get the contents of the specified files. Args: @@ -127,10 +127,18 @@ async def get_files(self, files: Union[str, List[str]], branch: Optional[str] = dedent( """ $name: object(expression: "$branch:$file") { - ... on Blob { - text - } + ... on Tree { + entries { + name + mode + object { + ... on Blob { + text + } + } + } } + } """ ) ) @@ -140,7 +148,7 @@ async def get_files(self, files: Union[str, List[str]], branch: Optional[str] = # file paths to their hashes, and use the hashes as the key names # in the query. aliases = {} - ret: Dict[str, Union[str, None]] = {} + ret: Dict[str, Union[str, Dict[str, Optional[str]]]] = {} # yields the starting index for each batch for i in range(0, len(files), files_per_request): @@ -164,8 +172,11 @@ async def get_files(self, files: Union[str, List[str]], branch: Optional[str] = name = aliases[k] if v is None: ret[name] = None - else: - ret[name] = v["text"] + else: + if mode: + ret[name] = {"mode": v.get("mode"), "text": v.get("text")} + else: + ret[name] = v.get("text") return ret diff --git a/scriptworker_client/tests/test_github_client.py b/scriptworker_client/tests/test_github_client.py index 76b8995fa..cee6f2d8a 100644 --- a/scriptworker_client/tests/test_github_client.py +++ b/scriptworker_client/tests/test_github_client.py @@ -109,7 +109,7 @@ async def test_get_files(aioresponses, github_client): aioresponses.post(GITHUB_GRAPHQL_ENDPOINT, status=200, payload={"data": {"repository": {aliases[k]: {"text": v} for k, v in expected.items()}}}) - result = await github_client.get_files(files, branch) + result = await github_client.get_files(files, branch, mode=None) assert result == expected aioresponses.assert_called() @@ -123,13 +123,29 @@ async def test_get_files(aioresponses, github_client): query getFileContents {{ repository(owner: "{github_client.owner}", name: "{github_client.repo}") {{ {aliases[str(files[0])]}: object(expression: "{branch}:{files[0]}") {{ - ... on Blob {{ - text + ... on Tree {{ + entries {{ + name + mode + object {{ + ... on Blob {{ + text + }} + }} + }} }} }} {aliases[str(files[1])]}: object(expression: "{branch}:{files[1]}") {{ - ... on Blob {{ - text + ... on Tree {{ + entries {{ + name + mode + object {{ + ... on Blob {{ + text + }} + }} + }} }} }} }} @@ -152,7 +168,7 @@ async def test_get_files_multiple_requests(aioresponses, github_client): aioresponses.post(GITHUB_GRAPHQL_ENDPOINT, status=200, payload={"data": {"repository": {aliases["README.md"]: {"text": expected["README.md"]}}}}) aioresponses.post(GITHUB_GRAPHQL_ENDPOINT, status=200, payload={"data": {"repository": {aliases["version.txt"]: {"text": expected["version.txt"]}}}}) - result = await github_client.get_files(files, branch, files_per_request=1) + result = await github_client.get_files(files, branch, files_per_request=1, mode=None) assert result == expected aioresponses.assert_called() @@ -167,8 +183,16 @@ async def test_get_files_multiple_requests(aioresponses, github_client): query getFileContents {{ repository(owner: "{github_client.owner}", name: "{github_client.repo}") {{ {aliases["README.md"]}: object(expression: "{branch}:README.md") {{ - ... on Blob {{ - text + ... on Tree {{ + entries {{ + name + mode + object {{ + ... on Blob {{ + text + }} + }} + }} }} }} }} @@ -182,8 +206,16 @@ async def test_get_files_multiple_requests(aioresponses, github_client): query getFileContents {{ repository(owner: "{github_client.owner}", name: "{github_client.repo}") {{ {aliases["version.txt"]}: object(expression: "{branch}:version.txt") {{ - ... on Blob {{ - text + ... on Tree {{ + entries {{ + name + mode + object {{ + ... on Blob {{ + text + }} + }} + }} }} }} }} @@ -210,7 +242,7 @@ async def test_get_files_with_missing(aioresponses, github_client): payload={"data": {"repository": {aliases["README.md"]: {"text": "Hello!"}, aliases["version.txt"]: {"text": "109.1.0"}, aliases["missing.txt"]: None}}}, ) - result = await github_client.get_files(files, branch) + result = await github_client.get_files(files, branch, mode=None) assert result == expected aioresponses.assert_called() @@ -224,18 +256,42 @@ async def test_get_files_with_missing(aioresponses, github_client): query getFileContents {{ repository(owner: "{github_client.owner}", name: "{github_client.repo}") {{ {aliases[str(files[0])]}: object(expression: "{branch}:{files[0]}") {{ - ... on Blob {{ - text + ... on Tree {{ + entries {{ + name + mode + object {{ + ... on Blob {{ + text + }} + }} + }} }} }} {aliases[str(files[1])]}: object(expression: "{branch}:{files[1]}") {{ - ... on Blob {{ - text + ... on Tree {{ + entries {{ + name + mode + object {{ + ... on Blob {{ + text + }} + }} + }} }} }} {aliases[str(files[2])]}: object(expression: "{branch}:{files[2]}") {{ - ... on Blob {{ - text + ... on Tree {{ + entries {{ + name + mode + object {{ + ... on Blob {{ + text + }} + }} + }} }} }} }} diff --git a/treescript/src/treescript/github/versionmanip.py b/treescript/src/treescript/github/versionmanip.py index e880132ee..955312698 100644 --- a/treescript/src/treescript/github/versionmanip.py +++ b/treescript/src/treescript/github/versionmanip.py @@ -125,7 +125,7 @@ async def do_bump_version(client: GithubClient, files: List[str], next_version: """ changes = {} diff = [] - file_contents = await client.get_files(files, branch=branch) + file_contents = await client.get_files(files, branch=branch, mode=None) for file_ in files: if file_ not in ALLOWED_BUMP_FILES: raise TaskVerificationError("{} is not in version bump whitelist".format(file_)) From 58bd0ba448da8a853e43a6bff885837179e17f9d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 19 Oct 2025 13:31:59 +0000 Subject: [PATCH 2/2] style: pre-commit.ci auto fixes [...] --- .../src/pytest_scriptworker_client/__init__.py | 5 +---- .../src/scriptworker_client/github_client.py | 6 ++++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/scriptworker_client/packages/pytest-scriptworker-client/src/pytest_scriptworker_client/__init__.py b/scriptworker_client/packages/pytest-scriptworker-client/src/pytest_scriptworker_client/__init__.py index 91af97775..dedb46c1d 100644 --- a/scriptworker_client/packages/pytest-scriptworker-client/src/pytest_scriptworker_client/__init__.py +++ b/scriptworker_client/packages/pytest-scriptworker-client/src/pytest_scriptworker_client/__init__.py @@ -59,10 +59,7 @@ def get_files_payload(file_contents={}): if contents is None: payload["data"]["repository"][hash_] = None elif isinstance(contents, dict): - payload["data"]["repository"][hash_] = { - "mode": contents.get("mode"), - "text": contents.get("text") - } + payload["data"]["repository"][hash_] = {"mode": contents.get("mode"), "text": contents.get("text")} else: payload["data"]["repository"][hash_] = {"text": contents} diff --git a/scriptworker_client/src/scriptworker_client/github_client.py b/scriptworker_client/src/scriptworker_client/github_client.py index 87d3f1e69..795a25728 100644 --- a/scriptworker_client/src/scriptworker_client/github_client.py +++ b/scriptworker_client/src/scriptworker_client/github_client.py @@ -93,7 +93,9 @@ async def _execute(): await retry_async(_execute, attempts=3, retry_exceptions=(TransportQueryError,), sleeptime_kwargs={"delay_factor": 0}) - async def get_files(self, files: Union[str, List[str]], branch: Optional[str] = None, files_per_request: int = 200, mode: Optional[str] = None) -> Dict[str, Union[str, Dict[str, Optional[str]]]]: + async def get_files( + self, files: Union[str, List[str]], branch: Optional[str] = None, files_per_request: int = 200, mode: Optional[str] = None + ) -> Dict[str, Union[str, Dict[str, Optional[str]]]]: """Get the contents of the specified files. Args: @@ -172,7 +174,7 @@ async def get_files(self, files: Union[str, List[str]], branch: Optional[str] = name = aliases[k] if v is None: ret[name] = None - else: + else: if mode: ret[name] = {"mode": v.get("mode"), "text": v.get("text")} else: