Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 16 additions & 2 deletions landoscript/src/landoscript/actions/version_bump.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import json
import logging
import os.path
import typing
Expand All @@ -17,10 +18,11 @@

log = logging.getLogger(__name__)

# A list of files that this action is allowed to operate on.
ALLOWED_BUMP_FILES = (
"browser/config/version.txt",
"browser/config/version_display.txt",
"browser/extensions/newtab/manifest.json",
Comment thread
JohanLorenzo marked this conversation as resolved.
"browser/extensions/webcompat/manifest.json",
"config/milestone.txt",
"mobile/android/version.txt",
"mail/config/version.txt",
Expand All @@ -34,6 +36,12 @@ class VersionBumpInfo:
files: list[str]


def parse_manifest_version(orig_contents: str) -> BaseVersion:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we're not using a parser to write there's no good reason to use it to read either IMO. If we care about the risk of updating the wrong substring in the file the previously reviewed version of this is preferable. If we're using simple string replacement, this is unnecessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I'm not sure I follow. get_cur_and_next_version() parses both the current and the next version no matter if it's a plain text or not. So the existing implementation is already parses the data to read but not to write.

The extra layer here just extracts the content of manifest["version"] for this check to happen:

cur, next_ = get_cur_and_next_version(file, orig, next_version, munge_next_version)
if next_ < cur:
log.warning(f"{file}: Version bumping skipped due to conflicting values: (next version {next_} is < current version {cur})")
continue
elif next_ == cur:
log.info(f"{file}: Version bumping skipped due to unchanged values")
continue

I'm not sure what I should change here. Could you expand on it @bhearsum?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're right, my bad. This is slightly different and more involved than what we do currently, but not categorically different.

"""Extract and parse the version field from a JSON manifest."""
manifest = json.loads(orig_contents)
return BaseVersion.parse(manifest["version"])


async def run(
github_client: GithubClient,
public_artifact_dir: str,
Expand Down Expand Up @@ -87,7 +95,8 @@ async def run(

modified = orig.replace(str(cur), str(next_))
if orig == modified:
raise LandoscriptError("file not modified, this should be impossible")
log.warning(f"{file}: version replacement produced no change, skipping")
continue

log.info(f"{file}: successfully bumped! new contents are:")
log_file_contents(modified)
Expand Down Expand Up @@ -119,6 +128,11 @@ def extract_path(diff_text):


def get_cur_and_next_version(filename, orig_contents, next_version, munge_next_version):
if filename.endswith(".json"):
cur = parse_manifest_version(orig_contents)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I might suggest a more detailed check on what we're parsing here rather than just looking for ".json", but this is also fine for now.

next_ = BaseVersion.parse(next_version)
return cur, next_

VersionClass: BaseVersion = find_what_version_parser_to_use(filename)
lines = [line for line in orig_contents.splitlines() if line and not line.startswith("#")]
cur = VersionClass.parse(lines[-1])
Expand Down
94 changes: 94 additions & 0 deletions landoscript/tests/test_version_bump.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from landoscript.errors import LandoscriptError
from landoscript.script import async_main
from landoscript.actions.version_bump import ALLOWED_BUMP_FILES
import json as _json
from landoscript.util.version import _VERSION_CLASS_PER_BEGINNING_OF_PATH

from .conftest import (
Expand Down Expand Up @@ -394,4 +395,97 @@ def test_no_overlaps_in_version_classes():

def test_all_bump_files_have_version_class():
for bump_file in ALLOWED_BUMP_FILES:
# JSON manifests use BaseVersion directly and bypass the version class lookup
if bump_file.endswith(".json"):
continue
assert any([bump_file.startswith(path) for path in _VERSION_CLASS_PER_BEGINNING_OF_PATH])


NEWTAB_MANIFEST = "browser/extensions/newtab/manifest.json"
WEBCOMPAT_MANIFEST = "browser/extensions/webcompat/manifest.json"
MANIFEST_FILE = NEWTAB_MANIFEST


def _manifest(version):
return _json.dumps({"manifest_version": 2, "name": "New Tab", "version": version}, indent=2)


@pytest.mark.asyncio
@pytest.mark.parametrize(
"initial_version,expected_version",
[
pytest.param("151.0.0", "151.1.0", id="initial_after_merge_day"),
pytest.param("151.1.0", "151.2.0", id="normal_minor_bump"),
],
)
async def test_json_manifest_bump(aioresponses, github_installation_responses, context, initial_version, expected_version):
payload = {
"actions": ["version_bump"],
"lando_repo": "repo_name",
"version_bump_info": {
"files": [MANIFEST_FILE],
"next_version": expected_version,
},
}
setup_github_graphql_responses(aioresponses, get_files_payload({MANIFEST_FILE: _manifest(initial_version)}))
await run_test(
aioresponses,
github_installation_responses,
context,
payload,
["version_bump"],
assert_func=lambda req: assert_success(
req,
["Automatic version bump", "NO BUG", "a=release", "CLOSED TREE"],
{MANIFEST_FILE: f' "version": "{initial_version}"'},
{MANIFEST_FILE: f' "version": "{expected_version}"'},
),
)


@pytest.mark.asyncio
async def test_json_manifest_file_not_found(aioresponses, github_installation_responses, context):
payload = {
"actions": ["version_bump"],
"lando_repo": "repo_name",
"version_bump_info": {
"files": [MANIFEST_FILE],
"next_version": "151.1.0",
},
}
setup_github_graphql_responses(aioresponses, get_files_payload({MANIFEST_FILE: None}))
await run_test(
aioresponses,
github_installation_responses,
context,
payload,
["version_bump"],
err=LandoscriptError,
errmsg="does not exist",
)


@pytest.mark.asyncio
async def test_webcompat_manifest_bump(aioresponses, github_installation_responses, context):
payload = {
"actions": ["version_bump"],
"lando_repo": "repo_name",
"version_bump_info": {
"files": [WEBCOMPAT_MANIFEST],
"next_version": "151.1.0",
},
}
setup_github_graphql_responses(aioresponses, get_files_payload({WEBCOMPAT_MANIFEST: _manifest("151.0.0")}))
await run_test(
aioresponses,
github_installation_responses,
context,
payload,
["version_bump"],
assert_func=lambda req: assert_success(
req,
["Automatic version bump", "NO BUG", "a=release", "CLOSED TREE"],
{WEBCOMPAT_MANIFEST: ' "version": "151.0.0"'},
{WEBCOMPAT_MANIFEST: ' "version": "151.1.0"'},
),
)
33 changes: 33 additions & 0 deletions landoscript/tests/test_version_bump_json_manifest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import json

import pytest

from landoscript.actions.version_bump import parse_manifest_version


def _manifest(version, name="New Tab"):
return json.dumps({"manifest_version": 2, "name": name, "version": version}, indent=2)


@pytest.mark.parametrize(
"version",
["151.0.0", "151.1.0", "151.2.0"],
)
def test_parse_manifest_version(version):
result = parse_manifest_version(_manifest(version))
assert str(result) == version


def test_parse_manifest_version_ignores_other_fields():
contents = '{"manifest_version": 2, "version": "151.0.0", "strict_min_version": "140.0"}'
result = parse_manifest_version(contents)
assert str(result) == "151.0.0"


@pytest.mark.parametrize(
"manifest_name",
["New Tab", "Web Compat"],
)
def test_parse_manifest_version_works_for_newtab_and_webcompat(manifest_name):
result = parse_manifest_version(_manifest("151.0.0", name=manifest_name))
assert str(result) == "151.0.0"