feat(nimbus): Support multiple FML paths per app; fix focus-ios fetch…

…ing (#9805)

Because

- focus-ios had its nimbus.fml.yaml file move recently (see
mozilla-mobile/focus-ios#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/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"

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]
 

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,

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,17 +29,18 @@ 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]:
     """Make several reqeusts to a paginated API resource and yield each page of results."""
     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

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)),
         ],
     )

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",