Skip to content

Commit

Permalink
Duplicate countries to be in lowercase for x-country header update.
Browse files Browse the repository at this point in the history
  • Loading branch information
goodov committed Dec 18, 2024
1 parent 7970395 commit 480bd97
Show file tree
Hide file tree
Showing 7 changed files with 198 additions and 6 deletions.
2 changes: 2 additions & 0 deletions .github/workflows/generate-test-seed.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ name: Generate Test Seed

on:
pull_request:
types: [opened, synchronize, reopened, labeled]
paths:
- '.github/workflows/generate-test-seed.yml'
- 'studies/**'

jobs:
build:
if: github.event.action != 'labeled' || github.event.label.name == 'generate-seed'
runs-on: ubuntu-latest
env:
ACTION_RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}'
Expand Down
6 changes: 4 additions & 2 deletions src/finch_tracker/tracker_lib.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as os from 'os';
import { describe, expect, test } from '@jest/globals';
import JSON5 from 'json5';
import path from 'path';
import { asPosix } from '../base/path_utils';
import { StudyPriority } from '../core/study_processor';
import { ItemAction, makeSummary, summaryToJson } from '../core/summary';
import { Study, Study_Channel, Study_Platform } from '../proto/generated/study';
Expand All @@ -18,11 +19,12 @@ import { storeDataToDirectory } from './tracker_lib';
function readDirectory(dir: string): Record<string, any> {
const files = fs
.readdirSync(dir, { recursive: true, encoding: 'utf-8' })
.sort();
.sort()
.map(asPosix);
const result: Record<string, string> = {};

for (const file of files) {
const filePath = path.join(dir, file);
const filePath = `${dir}/${file}`;
if (!file.endsWith('.json5')) {
continue;
}
Expand Down
29 changes: 27 additions & 2 deletions src/seed_tools/utils/studies_to_seed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { execSync } from 'child_process';
import { promises as fs } from 'fs';
import * as path from 'path';
import { wsPath } from 'src/base/path_utils';
import { asPosix, wsPath } from 'src/base/path_utils';
import {
Study_ActivationType,
Study_Consistency,
Expand Down Expand Up @@ -36,6 +36,10 @@ export async function readStudiesToSeed(
layers: [],
};

// Keep country filters in both uppercase and lowercase. This is a workaround
// to support x-country header returning uppercase values.
duplicateCountriesAsLowercase(variationsSeed);

errors.push(
...seed_validation.getSeedErrors(variationsSeed, studyFileBaseNameMap),
);
Expand Down Expand Up @@ -72,7 +76,7 @@ async function readStudiesAtRevision(
errors: string[];
}> {
const basePath = wsPath('//');
studiesDir = path.relative(basePath, studiesDir);
studiesDir = asPosix(path.relative(basePath, studiesDir));

// Validate revision format.
if (!/^[a-z0-9]+$/.test(revision) && revision !== 'HEAD') {
Expand Down Expand Up @@ -190,3 +194,24 @@ function setStudyDefaultParameters(study: Study) {
study.consistency = Study_Consistency.PERMANENT;
}
}

function duplicateCountriesAsLowercase(variationsSeed: VariationsSeed) {
// For now duplicate the country filters to lowercase.
const duplicate = (countries: string[]) => {
const countrySet = new Set(countries);
const allCountries = [...countries];
for (const country of countries) {
if (!countrySet.has(country.toLowerCase())) {
allCountries.push(country.toLowerCase());
}
}
return allCountries;
};
for (const study of variationsSeed.study) {
if (study.filter === undefined) {
continue;
}
study.filter.country = duplicate(study.filter.country);
study.filter.exclude_country = duplicate(study.filter.exclude_country);
}
}
25 changes: 25 additions & 0 deletions src/seed_tools/utils/study_validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export function getStudyErrors(study: Study, fileBaseName: string): string[] {
checkVersionRange,
checkOsVersionRange,
checkChannelAndPlatform,
checkCountry,
];
for (const validator of validators) {
try {
Expand Down Expand Up @@ -343,3 +344,27 @@ export function validateName(
}
return true;
}

function checkCountry(study: Study): string[] {
const errors: string[] = [];
if (study.filter === undefined) {
return errors;
}

// check if country in uppercase also exists in lowercase
const checkCountriesCase = (countries: string[]) => {
const countrySet = new Set(countries);
for (const country of countries) {
if (!countrySet.has(country.toLowerCase())) {
errors.push(
`Country ${country} in lowercase is required for study ${study.name}`,
);
}
}
};

checkCountriesCase(study.filter.country);
checkCountriesCase(study.filter.exclude_country);

return errors;
}
25 changes: 23 additions & 2 deletions src/test/data/perf_seeds/legacy_seed/expected_seed.json
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,18 @@
"MX",
"BR",
"AR",
"IN"
"IN",
"ca",
"de",
"fr",
"gb",
"us",
"at",
"es",
"mx",
"br",
"ar",
"in"
]
}
},
Expand Down Expand Up @@ -637,7 +648,17 @@
"MX",
"BR",
"AR",
"IN"
"IN",
"ca",
"de",
"fr",
"gb",
"at",
"es",
"mx",
"br",
"ar",
"in"
]
}
},
Expand Down
63 changes: 63 additions & 0 deletions src/test/data/valid_seeds/country_case/expected_seed.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
{
"serial_number": "1",
"study": [
{
"name": "TestStudy",
"consistency": "PERMANENT",
"experiment": [
{
"name": "Enabled",
"probability_weight": 100,
"feature_association": {
"enable_feature": [
"TestFeature"
]
}
}
],
"filter": {
"min_version": "92.1.30.57",
"channel": [
"CANARY"
],
"platform": [
"PLATFORM_WINDOWS"
],
"country": [
"US",
"us"
]
},
"activation_type": "ACTIVATE_ON_STARTUP"
},
{
"name": "TestStudy",
"consistency": "PERMANENT",
"experiment": [
{
"name": "Enabled",
"probability_weight": 100,
"feature_association": {
"enable_feature": [
"TestFeature"
]
}
}
],
"filter": {
"min_version": "92.1.30.57",
"channel": [
"BETA"
],
"platform": [
"PLATFORM_WINDOWS"
],
"country": [
"us"
]
},
"activation_type": "ACTIVATE_ON_STARTUP"
}
],
"version": "1"
}
54 changes: 54 additions & 0 deletions src/test/data/valid_seeds/country_case/studies/TestStudy.json5
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
[
{
name: 'TestStudy',
experiment: [
{
name: 'Enabled',
probability_weight: 100,
feature_association: {
enable_feature: [
'TestFeature',
],
},
},
],
filter: {
min_version: '92.1.30.57',
channel: [
'NIGHTLY',
],
platform: [
'WINDOWS',
],
country: [
'US',
],
},
},
{
name: 'TestStudy',
experiment: [
{
name: 'Enabled',
probability_weight: 100,
feature_association: {
enable_feature: [
'TestFeature',
],
},
},
],
filter: {
min_version: '92.1.30.57',
channel: [
'BETA',
],
platform: [
'WINDOWS',
],
country: [
'us',
],
},
},
]

0 comments on commit 480bd97

Please sign in to comment.