feat(nimbus): Include fetch summary in update_external_resources PR (#9816)

Because

- fetching feature manifests can fail (see #9804 for an example);
- the fetch command does not hard fail when an individual fetch fails;
and
- we are generating a summary of all the fetches

This commit

- updates the fetch command to hard fail if there are fetch failures and
no
  successes (i.e., in the case where it would not open or update a PR);
- adds a flag to the fetch command to write the summary to a file; and
- opens and updates PRs with a summary of the fetches in the PR
description

Fixes #9815

.circleci/config.yml

@@ -508,7 +508,8 @@ jobs:
             git checkout main
             git pull origin main
             cp .env.sample .env
-            env GITHUB_BEARER_TOKEN="${GH_TOKEN}" make fetch_external_resources
+            env GITHUB_BEARER_TOKEN="${GH_TOKEN}" make fetch_external_resources FETCH_ARGS="--summary fetch-summary.txt"
+            mv ./experimenter/fetch-summary.txt /tmp/fetch-summary.txt
             if python3 ./experimenter/bin/should-pr.py
               then
                 git checkout -B external-config
@@ -517,7 +518,8 @@ jobs:
                 if (($((git diff external-config origin/external-config || git diff HEAD~1) | wc -c) > 0))
                   then
                     git push origin external-config -f
-                    gh pr create -t "chore(nimbus): Update External Configs" -b "" --base main --head external-config --repo mozilla/experimenter || echo "PR already exists, skipping"
+                    gh pr create -t "chore(nimbus): Update External Configs" -F /tmp/fetch-summary.txt --base main --head external-config --repo mozilla/experimenter || \
+                      gh pr edit external-config -F /tmp/fetch-summary.txt
                   else
                     echo "Changes already committed, skipping"
                 fi

Makefile

@@ -85,7 +85,7 @@ jetstream_config:
 	rm -Rf experimenter/experimenter/outcomes/metric-hub-main/.script/
 
 feature_manifests: build_dev
-	$(COMPOSE) run experimenter /experimenter/bin/manifest-tool.py fetch
+	$(COMPOSE) run experimenter /experimenter/bin/manifest-tool.py fetch $(FETCH_ARGS)
 
 install_nimbus_cli:  ## Install Nimbus client
 	mkdir -p $(CLI_DIR)

experimenter/manifesttool/cli.py

@@ -1,5 +1,7 @@
+import sys
 from dataclasses import dataclass
 from pathlib import Path
+from typing import Optional
 
 import click
 
@@ -40,7 +42,10 @@ def main(ctx: click.Context, *, manifest_dir: Path):
 
 @main.command("fetch")
 @click.pass_context
-def fetch(ctx: click.Context):
+@click.option(
+    "--summary", "summary_filename", type=Path, help="Write a summary to this file."
+)
+def fetch(ctx: click.Context, *, summary_filename: Optional[Path]):
     """Fetch the FML manifests and generate experimenter.yaml files."""
     context = ctx.find_object(Context)
 
@@ -67,4 +72,20 @@ def fetch(ctx: click.Context):
 
             ref_cache.write_to_file(ref_cache_path)
 
-    summarize_results(results)
+    summary_file = sys.stdout
+    if summary_filename:
+        summary_file = summary_filename.open("w")
+
+    success_count, cache_count, fail_count = summarize_results(results, summary_file)
+
+    if summary_filename:
+        summary_file.close()
+
+    # If we have any successful results, we'll exit normally. In CI, the PR will
+    # include the generated summary, which will contain any failures.
+    #
+    # However, if all we have are failures and cache hits, we won't produce a
+    # PR. Therefore we should exit(1) and cause the task to fail so it will be
+    # noticed.
+    if fail_count > 0 and success_count == 0:
+        raise SystemExit(1)

experimenter/manifesttool/fetch.py

@@ -2,7 +2,7 @@ import sys
 from dataclasses import dataclass
 from pathlib import Path
 from traceback import print_exception
-from typing import Optional
+from typing import Optional, TextIO
 
 import yaml
 from mozilla_nimbus_schemas import FeatureManifest
@@ -252,7 +252,13 @@ def fetch_releases(
     return results
 
 
-def summarize_results(results: list[FetchResult]):
+def summarize_results(results: list[FetchResult], file: TextIO) -> (int, int, int):
+    """Print out a summary of the results to the given file.
+
+    Returns:
+        A 3-tuple of the number of successes, the number of cache hits, and the
+        number of failures.
+    """
     successes = []
     failures = []
     cached = []
@@ -265,23 +271,28 @@ def summarize_results(results: list[FetchResult]):
         else:
             successes.append(result)
 
-    print("\n\nSUMMARY:\n")
+    if file == sys.stdout:
+        print("\n\n")
+
+    print("SUMMARY:\n", file=file)
 
     if successes:
-        print("SUCCESS:\n")
+        print("SUCCESS:\n", file=file)
         for result in successes:
-            print(result)
+            print(result, file=file)
 
-        print()
+        print(file=file)
 
     if cached:
-        print("CACHED:\n")
+        print("CACHED:\n", file=file)
         for result in cached:
-            print(result)
+            print(result, file=file)
 
-        print()
+        print(file=file)
 
     if failures:
-        print("FAILURES:\n")
+        print("FAILURES:\n", file=file)
         for result in failures:
-            print(result)
+            print(result, file=file)
+
+    return (len(successes), len(cached), len(failures))

experimenter/manifesttool/tests/test_cli.py

@@ -95,7 +95,7 @@ class CliTests(TestCase):
         with cli_runner(app_config=FML_APP_CONFIG) as runner:
             result = runner.invoke(main, ["--manifest-dir", ".", "fetch"])
 
-        self.assertEqual(result.exit_code, 0, result.exception or result.stdout)
+        self.assertEqual(result.exit_code, 1, result.exception or result.stdout)
 
         self.assertIn(
             "SUMMARY:\n\nFAILURES:\n\nfml_app at main version None\nConnection error\n",
@@ -145,7 +145,7 @@ class CliTests(TestCase):
         with cli_runner(app_config=LEGACY_APP_CONFIG) as runner:
             result = runner.invoke(main, ["--manifest-dir", ".", "fetch"])
 
-        self.assertEqual(result.exit_code, 0, result.exception or result.stdout)
+        self.assertEqual(result.exit_code, 1, result.exception or result.stdout)
 
         self.assertIn(
             "SUMMARY:\n\nFAILURES:\n\nlegacy_app at tip version None\nConnection error\n",
@@ -210,3 +210,59 @@ class CliTests(TestCase):
             app_config,
             cache,
         )
+
+    @patch.object(
+        manifesttool.cli,
+        "fetch_fml_app",
+        lambda *args: FetchResult(
+            "fml_app", ref=Ref("main", "quux"), version=None, exc=Exception("oh no")
+        ),
+    )
+    @patch.object(
+        manifesttool.fetch,
+        "discover_tagged_releases",
+        lambda *args: {
+            Version(1): Ref("foo", "bar"),
+            Version(2): Ref("baz", "qux"),
+        },
+    )
+    @patch.object(manifesttool.fetch, "fetch_fml_app", lambda *args: mock_fetch(*args))
+    @patch.object(
+        manifesttool.fetch.RefCache,
+        "load_or_create",
+        lambda *args: RefCache(__root__={"foo": "bar"}),
+    )
+    def test_fetch_summary_filename(self):
+        app_config = AppConfig(
+            slug="fml-app",
+            repo=Repository(
+                type=RepositoryType.GITHUB,
+                name="fml-repo",
+            ),
+            fml_path="nimbus.fml.yaml",
+            release_discovery=ReleaseDiscovery(
+                version_file=VersionFile.create_plain_text("version.txt"),
+                strategies=[DiscoveryStrategy.create_tagged(branch_re="")],
+            ),
+        )
+
+        with cli_runner(app_config=app_config) as runner:
+            result = runner.invoke(
+                main, ["--manifest-dir", ".", "fetch", "--summary", "summary.txt"]
+            )
+            self.assertEqual(result.exit_code, 0, result.exception or result.stdout)
+
+            with Path("summary.txt").open() as f:
+                summary = f.read()
+
+        self.assertEqual(
+            summary,
+            "SUMMARY:\n\n"
+            "SUCCESS:\n\n"
+            "fml_app at baz (qux) version 2.0.0\n\n"
+            "CACHED:\n\n"
+            "fml_app at foo (bar) version 1.0.0 (cached)\n\n"
+            "FAILURES:\n\n"
+            "fml_app at main (quux) version None\n"
+            "oh no\n\n",
+        )

experimenter/manifesttool/tests/test_fetch.py

@@ -1,5 +1,4 @@
 import json
-from contextlib import redirect_stdout
 from io import StringIO
 from pathlib import Path
 from tempfile import TemporaryDirectory
@@ -1002,31 +1001,30 @@ class FetchTests(TestCase):
         )
 
     def test_summarize_results(self):
-        stdout_buffer = StringIO()
+        buffer = StringIO()
 
-        with redirect_stdout(stdout_buffer):
-            summarize_results(
-                [
-                    FetchResult(
-                        "app-1",
-                        Ref("a", "foo"),
-                        None,
-                    ),
-                    FetchResult("app-2", Ref("b", "bar"), Version(1, 2, 3)),
-                    FetchResult("app-3", Ref("c", "baz"), None, exc=Exception("oh no")),
-                    FetchResult("app-4", Ref("d", "qux"), Version(4, 5, 6), cached=True),
-                    FetchResult(
-                        "app-5",
-                        Ref("e", "quux"),
-                        Version(7, 8, 9),
-                        exc=Exception("rats!"),
-                    ),
-                ]
-            )
+        summarize_results(
+            [
+                FetchResult(
+                    "app-1",
+                    Ref("a", "foo"),
+                    None,
+                ),
+                FetchResult("app-2", Ref("b", "bar"), Version(1, 2, 3)),
+                FetchResult("app-3", Ref("c", "baz"), None, exc=Exception("oh no")),
+                FetchResult("app-4", Ref("d", "qux"), Version(4, 5, 6), cached=True),
+                FetchResult(
+                    "app-5",
+                    Ref("e", "quux"),
+                    Version(7, 8, 9),
+                    exc=Exception("rats!"),
+                ),
+            ],
+            buffer,
+        )
 
         self.assertEqual(
-            stdout_buffer.getvalue(),
-            "\n\n"
+            buffer.getvalue(),
             "SUMMARY:\n\n"
             "SUCCESS:\n\n"
             "app-1 at a (foo) version None\n"