Skip to content

Commit 4e93c8a

Browse files
authored
feat(nimbus): fetch targeting contexts directly from each firefox app source code (#14936)
Because - We plan to add automated validation for Experimenter targeting configs by checking JEXL field references against the upstream targeting context definitions in the Desktop, Fenix, and iOS source code - Manifesttool already fetches app manifests per ref/version, but it was not downloading the targeting context source files that external config updates depend on. This commit - Adds `targeting_files` to app config and configures it for firefox-desktop, fenix, and ios - Stores the files by version in each app's feature manifest as well as an unversioned copy in the root Fixes #14922
1 parent ac1eac9 commit 4e93c8a

File tree

5 files changed

+152
-1
lines changed

5 files changed

+152
-1
lines changed

experimenter/experimenter/features/manifests/apps.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ fenix:
1818
- "main"
1919
- "beta"
2020
- "release"
21+
targeting_files:
22+
- "mobile/android/fenix/app/src/main/java/org/mozilla/fenix/experiments/RecordedNimbusContext.kt"
2123

2224
firefox_ios:
2325
slug: "ios"
@@ -42,6 +44,8 @@ firefox_ios:
4244
branch_re: 'release/v(?P<major>\d+)(?:\.(?P<minor>\d+))?'
4345
tag_re: 'firefox-v(?P<major>\d+)\.(?P<minor>\d+)'
4446
- type: "branched"
47+
targeting_files:
48+
- "firefox-ios/Client/Experiments/RecordedNimbusContext.swift"
4549

4650
monitor_cirrus:
4751
slug: "monitor-web"
@@ -85,6 +89,8 @@ firefox_desktop:
8589
- "esr115"
8690
- "esr128"
8791
- "esr140"
92+
targeting_files:
93+
- "toolkit/components/nimbus/lib/TargetingContextRecorder.sys.mjs"
8894

8995
experimenter_cirrus:
9096
slug: "experimenter"

experimenter/manifesttool/appconfig.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ class AppConfig(BaseModel):
150150
fml_path: str | list[str] | None = None
151151
experimenter_yaml_path: str | None = None
152152
release_discovery: ReleaseDiscovery | None = None
153+
targeting_files: list[str] | None = None
153154

154155
@model_validator(mode="before")
155156
@classmethod

experimenter/manifesttool/fetch.py

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,24 @@ def __str__(self): # pragma: no cover
3535
return as_str
3636

3737

38+
def fetch_targeting_files(
39+
save_path: Path,
40+
logging_msg: str,
41+
app_config: AppConfig,
42+
ref: Ref,
43+
) -> None:
44+
targeting_files_path = app_config.targeting_files
45+
if targeting_files_path:
46+
print(logging_msg)
47+
48+
github_api.fetch_file(
49+
app_config.repo.name,
50+
targeting_files_path[0],
51+
ref.target,
52+
save_path / Path(targeting_files_path[0]).name,
53+
)
54+
55+
3856
def fetch_fml_app(
3957
manifest_dir: Path,
4058
app_name: str,
@@ -107,6 +125,13 @@ def fetch_fml_app(
107125
version,
108126
)
109127

128+
fetch_targeting_files(
129+
manifest_dir / app_config.slug / f"v{version}",
130+
f"fetch: {app_name} at {ref} version {version} downloading targeting files",
131+
app_config,
132+
ref,
133+
)
134+
110135
print(f"fetch: {app_name}: generate experimenter.yaml")
111136
# The single-file fml file for each channel will generate the same
112137
# experimenter.yaml, so we can pick any here.
@@ -169,6 +194,13 @@ def fetch_legacy_app(
169194
manifest_path,
170195
)
171196

197+
fetch_targeting_files(
198+
manifest_dir / app_config.slug / f"v{version}",
199+
f"fetch: {app_name} at {ref} version {version} downloading targeting files",
200+
app_config,
201+
ref,
202+
)
203+
172204
with manifest_path.open() as f:
173205
raw_manifest = yaml.safe_load(f)
174206
manifest = DesktopFeatureManifest.parse_obj(raw_manifest)
@@ -252,6 +284,12 @@ def fetch_releases(
252284

253285
results.append(result)
254286

287+
fetch_targeting_files(
288+
manifest_dir / app_config.slug,
289+
f"fetch: {app_name} at {ref.name} downloading targeting files ",
290+
app_config,
291+
ref,
292+
)
255293
return results
256294

257295

experimenter/manifesttool/github_api.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,9 @@ def _get_refs(repo: str, kind: str) -> list[Ref]:
9696

9797

9898
@overload
99-
def fetch_file(repo: str, file_path: str, rev: str) -> str: ... # pragma: no cover
99+
def fetch_file(
100+
repo: str, file_path: str, rev: str
101+
) -> Optional[str]: ... # pragma: no cover
100102

101103

102104
@overload

experimenter/manifesttool/tests/test_fetch.py

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ def mock_download_single_file(
101101
version: Optional[Version],
102102
):
103103
"""A mock version of `nimbus fml -- single file`."""
104+
(manifest_dir / app_config.slug).mkdir(exist_ok=True)
104105
filename = _get_fml_path(manifest_dir, app_config, channel, version)
105106
with filename.open("w") as f:
106107
yaml.dump(generate_fml(app_config, channel), f)
@@ -880,6 +881,42 @@ def test_fetch_legacy_unresolved_ref(self):
880881
):
881882
fetch_legacy_app(manifest_dir, "repo", LEGACY_APP_CONFIG, Ref("foo"))
882883

884+
@patch.object(
885+
manifesttool.fetch.github_api,
886+
"fetch_file",
887+
side_effect=make_mock_fetch_file(
888+
paths_by_ref={
889+
"foo": {
890+
"experimenter.yaml": LEGACY_MANIFEST,
891+
"targeting_files.txt": {"context": "data"},
892+
}
893+
}
894+
),
895+
)
896+
def test_fetch_legacy_targeting_files(self, fetch_file):
897+
targeting_files_path = "targeting_files.txt"
898+
app_config = AppConfig(
899+
slug="legacy-app",
900+
repo=Repository(
901+
type=RepositoryType.GITHUB,
902+
name="legacy-repo",
903+
default_branch="tip",
904+
),
905+
experimenter_yaml_path="experimenter.yaml",
906+
targeting_files=[targeting_files_path],
907+
)
908+
909+
with TemporaryDirectory() as tmp:
910+
manifest_dir = Path(tmp)
911+
manifest_dir.joinpath("legacy-app").mkdir()
912+
fetch_legacy_app(
913+
manifest_dir, "app", app_config, Ref("tip", "foo"), Version(1, 0, 0)
914+
)
915+
916+
self.assertTrue(
917+
(manifest_dir / "legacy-app" / "v1.0.0" / "targeting_files.txt").exists()
918+
)
919+
883920
def test_fetch_releases_unsupported_apps(self):
884921
"""Testing fetch_releases with unsupported apps."""
885922
app_config = AppConfig(
@@ -1082,6 +1119,73 @@ def test_fetch_releases_cached(self, fetch_fml_app):
10821119
),
10831120
)
10841121

1122+
@patch.object(
1123+
manifesttool.fetch,
1124+
"discover_branched_releases",
1125+
lambda *args: {
1126+
Version(1): Ref("branch", "foo"),
1127+
Version(1, 2, 3): Ref("tag", "bar"),
1128+
},
1129+
)
1130+
@patch.object(
1131+
manifesttool.fetch.github_api,
1132+
"fetch_file",
1133+
side_effect=make_mock_fetch_file(
1134+
paths_by_ref={
1135+
"foo": {"targeting-contexts.yaml": {"context": "v1.0.0"}},
1136+
"bar": {"targeting-contexts.yaml": {"context": "v1.2.3"}},
1137+
}
1138+
),
1139+
)
1140+
@patch.object(
1141+
manifesttool.fetch.nimbus_cli,
1142+
"download_single_file",
1143+
side_effect=mock_download_single_file,
1144+
)
1145+
@patch.object(
1146+
manifesttool.fetch.nimbus_cli,
1147+
"get_channels",
1148+
side_effect=lambda *args: ["release", "beta"],
1149+
)
1150+
def test_fetch_releases_targeting_contexts(
1151+
self,
1152+
get_channels,
1153+
download_single_file,
1154+
fetch_file,
1155+
):
1156+
app_config = AppConfig(
1157+
slug="fml-app",
1158+
repo=Repository(
1159+
type=RepositoryType.GITHUB,
1160+
name="fml-repo",
1161+
),
1162+
fml_path="nimbus.fml.yaml",
1163+
release_discovery=ReleaseDiscovery(
1164+
version_file=VersionFile.create_plain_text("version.txt"),
1165+
strategies=[DiscoveryStrategy.create_branched()],
1166+
),
1167+
targeting_files=["targeting-contexts.yaml"],
1168+
)
1169+
1170+
cache = RefCache()
1171+
1172+
with TemporaryDirectory() as tmp:
1173+
manifest_dir = Path(tmp)
1174+
1175+
fetch_releases(manifest_dir, "fml_app", app_config, cache)
1176+
1177+
self.assertTrue(
1178+
(manifest_dir / "fml-app" / "v1.0.0" / "targeting-contexts.yaml").exists()
1179+
)
1180+
self.assertTrue(
1181+
(manifest_dir / "fml-app" / "v1.2.3" / "targeting-contexts.yaml").exists()
1182+
)
1183+
self.assertTrue(
1184+
(manifest_dir / "fml-app" / "targeting-contexts.yaml").exists()
1185+
)
1186+
1187+
self.assertEqual(fetch_file.call_count, 3)
1188+
10851189
def test_summarize_results(self):
10861190
buffer = StringIO()
10871191

0 commit comments

Comments
 (0)