Skip to content

Bug 1968280 - Draft: Landoscript should fetch file modes for newly added or files being removed. #1292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
elkal98 wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Bug 1968280 - Draft: Landoscript should fetch file modes for newly added or files being removed. #1292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87031f3
Bug 1968280 - Draft: Landoscript should fetch file modes for newly ad…
elkal98 Oct 19, 2025
58bd0ba
style: pre-commit.ci auto fixes [...]
pre-commit-ci[bot] Oct 19, 2025
1dd6ee8
Merge branch 'mozilla-releng:master' into landoscript-enhancement
elkal98 Nov 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions landoscript/src/landoscript/actions/android_l10n_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@bhearsum bhearsum Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you've updated get_files as noted below, the l10n_github_client.get_files calls in this function and android_l10n_sync.py will need to be updated to set fetch_mode=True.

You will also need changes to actually pass the modes along to diff_contents for each file.

l10n_files: list[L10nFile] = []

missing = [fn for fn, contents in toml_contents.items() if contents is None]
Expand Down Expand Up @@ -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)
Expand All @@ -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:
Expand Down
6 changes: 3 additions & 3 deletions landoscript/src/landoscript/actions/android_l10n_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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)
Expand All @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion landoscript/src/landoscript/actions/l10n_bump.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@bhearsum bhearsum Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific change should not be needed (see my comment elsewhere about the interface of get_files. You will need to changes to this and all other get_files calls to deal with the fact that it will now be returning dicts for each file, rather than strings.

except TransportError as e:
raise LandoscriptError("couldn't retrieve bump files from github") from e

Expand Down
6 changes: 3 additions & 3 deletions landoscript/src/landoscript/actions/merge_day.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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!")

Expand Down Expand Up @@ -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!")
Expand Down
2 changes: 1 addition & 1 deletion landoscript/src/landoscript/actions/version_bump.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 3 additions & 3 deletions landoscript/src/landoscript/util/diffs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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"
Copy link
Contributor

@bhearsum bhearsum Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having a default here, this function should raise an error if mode is not set when a file is being newly added or removed.

if modified:
# modified exists already
modified_contents = modified.splitlines()
Expand All @@ -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:
Expand Down
6 changes: 3 additions & 3 deletions landoscript/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <release+landoscript@mozilla.com>"
# we don't actually verify the value here; it's not worth the trouble of mocking
Expand Down Expand Up @@ -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:
Expand Down
3 changes: 3 additions & 0 deletions ...ker_client/packages/pytest-scriptworker-client/src/pytest_scriptworker_client/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ 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:
Expand Down
25 changes: 19 additions & 6 deletions scriptworker_client/src/scriptworker_client/github_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) -> 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
Copy link
Contributor

@bhearsum bhearsum Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not quite right - we want a boolean argument called something like fetch_mode that defaults to False. When True it should:

  • Change the query to fetch file modes
  • Return file modes in the return value

) -> Dict[str, Union[str, Dict[str, Optional[str]]]]:
"""Get the contents of the specified files.

Args:
Expand Down Expand Up @@ -127,10 +129,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
}
}
}
}
}
"""
)
)
Expand All @@ -140,7 +150,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):
Expand All @@ -165,7 +175,10 @@ async def get_files(self, files: Union[str, List[str]], branch: Optional[str] =
if v is None:
ret[name] = None
else:
ret[name] = v["text"]
if mode:
ret[name] = {"mode": v.get("mode"), "text": v.get("text")}
else:
ret[name] = v.get("text")
Copy link
Contributor

@bhearsum bhearsum Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update the value for each file to always be a dict; that will make things easier for callers, and make the function more comprehensible (especially the type signatures).


return ret

Expand Down
90 changes: 73 additions & 17 deletions scriptworker_client/tests/test_github_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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
}}
}}
}}
}}
}}
}}
Expand All @@ -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()
Expand All @@ -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
}}
}}
}}
}}
}}
}}
Expand All @@ -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
}}
}}
}}
}}
}}
}}
Expand All @@ -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()
Expand All @@ -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
}}
}}
}}
}}
}}
}}
Expand Down
2 changes: 1 addition & 1 deletion treescript/src/treescript/github/versionmanip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_))
Expand Down