From 4431d1a702a8b1dfc93f3b34d255f90f36cc4428 Mon Sep 17 00:00:00 2001 From: Barret Rennie Date: Wed, 22 Nov 2023 13:10:29 -0500 Subject: [PATCH] feat(nimbus): Support multiple FML paths per app; fix focus-ios fetching (#9805) Because - focus-ios had its nimbus.fml.yaml file move recently (see https://github.com/mozilla-mobile/focus-ios/pull/3917) This commit - updates our fetch logic to support multiple potential FML file locations; and - updates focus_ios.fml_path to support the old and new paths. Fixes #9804 --- .../experimenter/features/manifests/apps.yaml | 4 +- experimenter/manifesttool/appconfig.py | 2 +- experimenter/manifesttool/fetch.py | 14 +- experimenter/manifesttool/github_api.py | 31 +++- experimenter/manifesttool/nimbus_cli.py | 15 +- experimenter/manifesttool/tests/test_fetch.py | 159 +++++++++++++++++- .../manifesttool/tests/test_github_api.py | 29 +++- .../manifesttool/tests/test_nimbus_cli.py | 14 +- 8 files changed, 241 insertions(+), 27 deletions(-) diff --git a/experimenter/experimenter/features/manifests/apps.yaml b/experimenter/experimenter/features/manifests/apps.yaml index ecdcd84ddf..6277bd5a39 100644 --- a/experimenter/experimenter/features/manifests/apps.yaml +++ b/experimenter/experimenter/features/manifests/apps.yaml @@ -56,7 +56,9 @@ focus_ios: repo: type: "github" name: "mozilla-mobile/focus-ios" - fml_path: "nimbus.fml.yaml" + fml_path: + - "focus-ios/nimbus.fml.yaml" + - "nimbus.fml.yaml" release_discovery: version_file: type: "plist" diff --git a/experimenter/manifesttool/appconfig.py b/experimenter/manifesttool/appconfig.py index 3c7002580b..ce1c15f9f9 100644 --- a/experimenter/manifesttool/appconfig.py +++ b/experimenter/manifesttool/appconfig.py @@ -128,7 +128,7 @@ class AppConfig(BaseModel): slug: str repo: Repository - fml_path: Optional[str] + fml_path: Optional[Union[str, list[str]]] experimenter_yaml_path: Optional[str] release_discovery: Optional[ReleaseDiscovery] diff --git a/experimenter/manifesttool/fetch.py b/experimenter/manifesttool/fetch.py index c4717604cd..17ffd19ceb 100644 --- a/experimenter/manifesttool/fetch.py +++ b/experimenter/manifesttool/fetch.py @@ -67,7 +67,18 @@ def fetch_fml_app( else: print(f"fetch: {app_name} at {ref}") - channels = nimbus_cli.get_channels(app_config, ref.target) + if isinstance(app_config.fml_path, str): + fml_path = app_config.fml_path + elif isinstance(app_config.fml_path, list): + for fml_path in app_config.fml_path: + if github_api.file_exists(app_config.repo.name, fml_path, ref.target): + break + else: + raise Exception( + f"Could not find a feature manifest for {app_name} at {ref}" + ) + + channels = nimbus_cli.get_channels(app_config, fml_path, ref.target) print(f"fetch: {app_name}: channels are {', '.join(channels)}") if not channels: @@ -82,6 +93,7 @@ def fetch_fml_app( nimbus_cli.download_single_file( manifest_dir, app_config, + fml_path, channel, ref.target, version, diff --git a/experimenter/manifesttool/github_api.py b/experimenter/manifesttool/github_api.py index ea664c56e5..c9b2e2b0b6 100644 --- a/experimenter/manifesttool/github_api.py +++ b/experimenter/manifesttool/github_api.py @@ -18,7 +18,9 @@ GITHUB_API_HEADERS["Authorization"] = f"Bearer {bearer_token}" -def api_request(path: str, **kwargs: dict[str, Any]) -> Any: +def api_request( + path: str, *, raise_for_status: bool = True, **kwargs: dict[str, Any] +) -> requests.Response: """Make a request to the GitHub API.""" url = f"{GITHUB_API_URL}/{path}" rsp = requests.get(url, headers=GITHUB_API_HEADERS, **kwargs) @@ -27,9 +29,10 @@ def api_request(path: str, **kwargs: dict[str, Any]) -> Any: if rsp.headers.get("X-RateLimit-Remaining") == "0": raise Exception(f"Could not fetch {url}: GitHub API rate limit exceeded") - rsp.raise_for_status() + if raise_for_status: + rsp.raise_for_status() - return rsp.json() + return rsp def paginated_api_request(path: str, per_page: int = 100) -> Generator[Any, None, None]: @@ -37,7 +40,7 @@ def paginated_api_request(path: str, per_page: int = 100) -> Generator[Any, None page = 1 while True: - results = api_request(path, params={"page": page, "per_page": per_page}) + results = api_request(path, params={"page": page, "per_page": per_page}).json() # When there are no more results, the API returns an empty list. if results: @@ -58,7 +61,7 @@ def resolve_branch(repo: str, branch: str) -> Ref: branch: The name of the branch. """ - rsp = api_request(f"repos/{repo}/branches/{branch}") + rsp = api_request(f"repos/{repo}/branches/{branch}").json() return Ref(branch, rsp["commit"]["sha"]) @@ -131,3 +134,21 @@ def fetch_file( download.to_path(url, download_path) return None + + +def file_exists(repo: str, file_path: str, rev: str) -> bool: + """Return whether or not a file with the given path exists in the repo at + the given revision. + """ + rsp = api_request( + f"repos/{repo}/contents/{file_path}", + raise_for_status=False, + params={"ref": rev}, + ) + + if rsp.status_code == 404: + return False + + rsp.raise_for_status() + + return True diff --git a/experimenter/manifesttool/nimbus_cli.py b/experimenter/manifesttool/nimbus_cli.py index 006d1ba049..adb64814e3 100644 --- a/experimenter/manifesttool/nimbus_cli.py +++ b/experimenter/manifesttool/nimbus_cli.py @@ -4,6 +4,7 @@ from pathlib import Path from typing import Optional +from manifesttool import github_api from manifesttool.appconfig import AppConfig, RepositoryType from manifesttool.version import Version @@ -23,7 +24,7 @@ def nimbus_cli(args: list[str], *, output: bool = False): ) -def get_channels(app_config: AppConfig, ref: str) -> list[str]: +def get_channels(app_config: AppConfig, fml_path: str, ref: str) -> list[str]: """Get the list of channels supported by the application.""" assert app_config.repo.type == RepositoryType.GITHUB output = nimbus_cli( @@ -34,7 +35,7 @@ def get_channels(app_config: AppConfig, ref: str) -> list[str]: "--json", "--ref", ref, - f"@{app_config.repo.name}/{app_config.fml_path}", + f"@{app_config.repo.name}/{fml_path}", ], output=True, ) @@ -53,12 +54,18 @@ def get_channels(app_config: AppConfig, ref: str) -> list[str]: def download_single_file( manifest_dir: Path, app_config: AppConfig, + fml_path: str, channel: str, ref: str, version: Optional[Version], ): - """Download the single-file FML manifest for the app on the specified channel.""" + """Download the single-file FML manifest for the app on the specified + channel. + + If the AppConfig provides multiple FML paths, they will be tried in order. + """ assert app_config.repo.type == RepositoryType.GITHUB + nimbus_cli( [ "fml", @@ -68,7 +75,7 @@ def download_single_file( channel, "--ref", ref, - f"@{app_config.repo.name}/{app_config.fml_path}", + f"@{app_config.repo.name}/{fml_path}", str(_get_fml_path(manifest_dir, app_config, channel, version)), ], ) diff --git a/experimenter/manifesttool/tests/test_fetch.py b/experimenter/manifesttool/tests/test_fetch.py index 23ecd48ad2..c439527e38 100644 --- a/experimenter/manifesttool/tests/test_fetch.py +++ b/experimenter/manifesttool/tests/test_fetch.py @@ -9,6 +9,7 @@ import responses import yaml +from parameterized import parameterized import manifesttool from manifesttool.appconfig import ( @@ -94,6 +95,7 @@ def generate_fml(app_config: AppConfig, channel: str) -> dict[str, Any]: def mock_download_single_file( manifest_dir: Path, app_config: AppConfig, + fml_path: str, channel: str, ref: str, version: Optional[Version], @@ -225,8 +227,22 @@ def test_fetch_fml(self, get_channels, download_single_file, resolve_branch): get_channels.asset_called_with(FML_APP_CONFIG, "ref") download_single_file.assert_has_calls( [ - call(manifest_dir, FML_APP_CONFIG, "release", "ref", None), - call(manifest_dir, FML_APP_CONFIG, "beta", "ref", None), + call( + manifest_dir, + FML_APP_CONFIG, + FML_APP_CONFIG.fml_path, + "release", + "ref", + None, + ), + call( + manifest_dir, + FML_APP_CONFIG, + FML_APP_CONFIG.fml_path, + "beta", + "ref", + None, + ), ] ) @@ -284,10 +300,13 @@ def test_fetch_fml_ref( self.assertIsNone(result.exc) resolve_branch.assert_not_called() - get_channels.assert_called_with(FML_APP_CONFIG, ref.target) + get_channels.assert_called_with( + FML_APP_CONFIG, FML_APP_CONFIG.fml_path, ref.target + ) download_single_file.assert_called_with( manifest_dir, FML_APP_CONFIG, + FML_APP_CONFIG.fml_path, "release", ref.target, None, @@ -344,11 +363,27 @@ def test_fetch_fml_ref_version( self.assertEqual(result, FetchResult("app", ref, version)) resolve_branch.assert_not_called() - get_channels.assert_called_with(FML_APP_CONFIG, ref.target) + get_channels.assert_called_with( + FML_APP_CONFIG, FML_APP_CONFIG.fml_path, ref.target + ) download_single_file.assert_has_calls( [ - call(manifest_dir, FML_APP_CONFIG, "release", ref.target, version), - call(manifest_dir, FML_APP_CONFIG, "beta", ref.target, version), + call( + manifest_dir, + FML_APP_CONFIG, + FML_APP_CONFIG.fml_path, + "release", + ref.target, + version, + ), + call( + manifest_dir, + FML_APP_CONFIG, + FML_APP_CONFIG.fml_path, + "beta", + ref.target, + version, + ), ] ) @@ -363,6 +398,118 @@ def test_fetch_fml_ref_version( self.assertIn("v1.2.3", str(experimenter_manifest_path)) self.assertTrue(experimenter_manifest_path.exists()) + @parameterized.expand( + [ + (["nimbus.fml.yaml", "app/nimbus.fml.yaml"], "nimbus.fml.yaml"), + (["nimbus.fml.yaml", "app/nimbus.fml.yaml"], "app/nimbus.fml.yaml"), + ] + ) + @patch.object( + manifesttool.fetch.github_api, "resolve_branch", lambda *args: Ref("main", "foo") + ) + @patch.object( + manifesttool.fetch.nimbus_cli, + "get_channels", + side_effect=lambda *args: ["channel"], + ) + @patch.object( + manifesttool.fetch.nimbus_cli, + "download_single_file", + side_effect=mock_download_single_file, + ) + def test_fetch_fml_multiple_fml( + self, + fml_paths: list[str], + correct_fml_path: str, + download_single_file, + get_channels, + ): + app_config = AppConfig( + slug="fml-app", + repo=Repository( + type=RepositoryType.GITHUB, + name="fml-repo", + ), + fml_path=fml_paths, + ) + + def mock_file_exists(repo, file, rev): + return file == correct_fml_path + + with ( + TemporaryDirectory() as tmp, + patch.object( + manifesttool.fetch.github_api, "file_exists", side_effect=mock_file_exists + ) as file_exists, + ): + manifest_dir = Path(tmp) + manifest_dir.joinpath("fml-app").mkdir() + + result = fetch_fml_app(manifest_dir, "fml_app", app_config) + self.assertEqual( + result, + FetchResult( + app_name="fml_app", + ref=Ref("main", "foo"), + version=None, + ), + ) + + # Files are resolved in order, so file_exists() should be called index+1 times. + index = fml_paths.index(correct_fml_path) + self.assertEqual(file_exists.call_count, index + 1) + file_exists.assert_has_calls( + [call("fml-repo", fml_path, "foo") for fml_path in fml_paths[: index + 1]] + ) + + get_channels.assert_called_once_with(app_config, correct_fml_path, "foo") + download_single_file.assert_called_once_with( + manifest_dir, + app_config, + correct_fml_path, + "channel", + "foo", + None, + ) + + @patch.object( + manifesttool.fetch.github_api, "resolve_branch", lambda *args: Ref("main", "foo") + ) + @patch.object( + manifesttool.fetch.github_api, "file_exists", side_effect=lambda *args: False + ) + @patch.object(manifesttool.fetch.nimbus_cli, "get_channels") + def test_fetch_fml_missing_fml(self, get_channels, file_exists): + app_config = AppConfig( + slug="fml-app", + repo=Repository( + type=RepositoryType.GITHUB, + name="fml-repo", + ), + fml_path=[ + "does-not-exist.fml.yaml", + "neither-does-this.fml.yaml", + ], + ) + + with TemporaryDirectory() as tmp: + manifest_dir = Path(tmp) + manifest_dir.joinpath("fml-app").mkdir() + + result = fetch_fml_app(manifest_dir, "fml_app", app_config) + + self.assertEqual(file_exists.call_count, 2) + file_exists.assert_has_calls( + [call("fml-repo", fml_path, "foo") for fml_path in app_config.fml_path] + ) + + self.assertIsNotNone(result.exc) + self.assertEqual( + str(result.exc), "Could not find a feature manifest for fml_app at main (foo)" + ) + + get_channels.assert_not_called() + @patch.object( manifesttool.fetch.github_api, "resolve_branch", diff --git a/experimenter/manifesttool/tests/test_github_api.py b/experimenter/manifesttool/tests/test_github_api.py index dfd4a57b4d..9c567ad2c8 100644 --- a/experimenter/manifesttool/tests/test_github_api.py +++ b/experimenter/manifesttool/tests/test_github_api.py @@ -7,7 +7,7 @@ from responses import matchers from manifesttool import github_api -from manifesttool.github_api import GITHUB_RAW_URL +from manifesttool.github_api import GITHUB_RAW_URL, GITHUB_API_URL from manifesttool.repository import Ref @@ -34,7 +34,7 @@ class GitHubApiTests(TestCase): def test_resolve_branch(self): """Testing resolve_branch.""" responses.get( - "https://api.github.com/repos/owner/repo/branches/main", + f"{GITHUB_API_URL}/repos/owner/repo/branches/main", json={ "commit": { "sha": "0" * 40, @@ -50,7 +50,7 @@ def test_resolve_branch(self): def test_api_limit(self): """Testing detection of API rate limit errors.""" responses.get( - "https://api.github.com/repos/owner/repo/branches/main", + f"{GITHUB_API_URL}/repos/owner/repo/branches/main", headers={ "X-RateLimit-Remaining": "0", }, @@ -64,7 +64,7 @@ def test_api_limit(self): def test_get_branches(self): """Testing get_branches and the underlying paginated_api_request.""" rsps = _add_paginated_responses( - "https://api.github.com/repos/owner/repo/branches", + f"{GITHUB_API_URL}/repos/owner/repo/branches", { "1": { "json": [ @@ -110,7 +110,7 @@ def test_get_branches(self): def test_get_tags(self): """Testing get_tags.""" rsps = _add_paginated_responses( - "https://api.github.com/repos/owner/repo/tags", + f"{GITHUB_API_URL}/repos/owner/repo/tags", { "1": { "json": [ @@ -180,3 +180,22 @@ def test_fetch_file_download(self): contents = github_api.fetch_file("repo", "file/path.txt", "ref") self.assertEqual(contents, "hello, world\n") + + @responses.activate + def test_file_exists(self): + """Testing github_api.file_exists.""" + responses.get( + f"{GITHUB_API_URL}/repos/repo/contents/file.txt", + status=404, + body="404", + match=[matchers.query_param_matcher({"ref": "foo"})], + ) + + responses.get( + f"{GITHUB_API_URL}/repos/repo/contents/file.txt", + json={}, + match=[matchers.query_param_matcher({"ref": "bar"})], + ) + + self.assertFalse(github_api.file_exists("repo", "file.txt", "foo")) + self.assertTrue(github_api.file_exists("repo", "file.txt", "bar")) diff --git a/experimenter/manifesttool/tests/test_nimbus_cli.py b/experimenter/manifesttool/tests/test_nimbus_cli.py index d2127478a7..01e575521d 100644 --- a/experimenter/manifesttool/tests/test_nimbus_cli.py +++ b/experimenter/manifesttool/tests/test_nimbus_cli.py @@ -2,7 +2,7 @@ from pathlib import Path from tempfile import TemporaryDirectory from unittest import TestCase -from unittest.mock import patch +from unittest.mock import call, patch from parameterized import parameterized @@ -31,7 +31,8 @@ class NimbusCliTests(TestCase): def test_get_channels(self, mock_cli): """Testing get_channels calls nimbus-cli with correct arguments.""" self.assertEqual( - nimbus_cli.get_channels(APP_CONFIG, "0" * 40), ["staging", "prod"] + nimbus_cli.get_channels(APP_CONFIG, APP_CONFIG.fml_path, "0" * 40), + ["staging", "prod"], ) mock_cli.assert_called_with( [ @@ -54,7 +55,7 @@ def test_get_channels(self, mock_cli): def test_get_channels_invalid(self): "Testing get_channels handling of invalid JSON." with self.assertRaises(json.decoder.JSONDecodeError): - nimbus_cli.get_channels(APP_CONFIG, "channel") + nimbus_cli.get_channels(APP_CONFIG, APP_CONFIG.fml_path, "channel") @parameterized.expand( [ @@ -73,7 +74,12 @@ def test_download_single_file(self, version, fml_path): manifest_dir = Path(tmp) manifest_dir.joinpath("slug").mkdir() nimbus_cli.download_single_file( - manifest_dir, APP_CONFIG, "channel", "0" * 40, version + manifest_dir, + APP_CONFIG, + APP_CONFIG.fml_path, + "channel", + "0" * 40, + version, ) check_call.assert_called_with(