Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add min version in TAV matrix #3531

Merged
merged 19 commits into from
Aug 3, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 34 additions & 35 deletions .ci/tav.json
david-luna marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,39 +1,38 @@
{
"// todo": "We want versions=['20','19','18','16','14','12','10','8'], but versions*modules needs to be <256 for the GH Actions jobs limit",
"versions": [ "20", "18", "16", "14", "12", "10", "8" ],
"versions": [ 20, 19, 18, 16, 14, 12, 10, 8 ],
"modules": [
david-luna marked this conversation as resolved.
Show resolved Hide resolved
"@apollo/server",
"@aws-sdk/client-s3",
"@elastic/elasticsearch",
"@hapi/hapi",
"@opentelemetry/api",
"@opentelemetry/sdk-metrics",
"apollo-server-express",
"aws-sdk",
"cassandra-driver",
"elasticsearch",
"express",
"express-queue",
"fastify",
"finalhandler",
"generic-pool",
"graphql",
"ioredis",
"knex",
"memcached",
"mongodb",
"mongodb-core",
"mysql",
"mysql2",
"next",
"pg",
"redis",
"restify",
"tedious",
"undici",
"ws",
"@koa/router,koa-router",
"handlebars,pug",
"bluebird,got"
{"name": "@apollo/server", "minVersion": 14 },
{"name": "@aws-sdk/client-s3", "minVersion": 14 },
{"name": "@elastic/elasticsearch", "minVersion": 10 },
{"name": "@hapi/hapi", "minVersion": 8 },
{"name": "@opentelemetry/api", "minVersion": 14 },
{"name": "@opentelemetry/sdk-metrics", "minVersion": 14 },
{"name": "apollo-server-express", "minVersion": 8 },
{"name": "aws-sdk", "minVersion": 8 },
{"name": "cassandra-driver", "minVersion": 8 },
{"name": "elasticsearch", "minVersion": 8 },
{"name": "express", "minVersion": 8 },
{"name": "express-queue", "minVersion": 8 },
{"name": "fastify", "minVersion": 8 },
{"name": "finalhandler", "minVersion": 8 },
{"name": "generic-pool", "minVersion": 8 },
{"name": "graphql", "minVersion": 8 },
{"name": "ioredis", "minVersion": 8 },
{"name": "knex", "minVersion": 8 },
{"name": "memcached", "minVersion": 8 },
{"name": "mongodb", "minVersion": 8 },
{"name": "mongodb-core", "minVersion": 8 },
{"name": "mysql", "minVersion": 8 },
{"name": "mysql2", "minVersion": 8 },
{"name": "next", "minVersion": 14 },
{"name": "pg", "minVersion": 8 },
{"name": "redis", "minVersion": 8 },
{"name": "restify", "minVersion": 14 },
{"name": "tedious", "minVersion": 8 },
{"name": "undici", "minVersion": 8 },
{"name": "ws", "minVersion": 8 },
{"name": "@koa/router,koa-router", "minVersion": 8 },
{"name": "handlebars,pug", "minVersion": 8 },
{"name": "bluebird,got", "minVersion": 8 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, I guess with all these minVersion: 8s, it means there won't be a large reduction in the number of jobs. Still probably worth keeping.

(Not part of this PR, but I wonder if we could consider dropping testing for versions of these packages that are, say, older than 2y and not of the latest major version. That might enable raising the minVersion here.)

Copy link
Member Author

@david-luna david-luna Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can leverage the multiple package option we have, here we are testing 2 packages in the same job

{"name": "bluebird,got", "minVersion": 8 }

This is actually what I want to do for @aws-sdk/client-* to test all of them at once for each version. There might be other groups we could make perhaps @opentelemetry/* or mysql? Of course some test review is needed to see compatibility. Also I think the runTestFixtures util introduced in the ESM support PR would come handy

we could consider dropping testing for versions of these packages that are, say, older than 2y
I agree and also I think it could be something to motivate users to update

]
}
84 changes: 65 additions & 19 deletions .github/workflows/tav-command.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ jobs:
permissions:
pull-requests: write
outputs:
versions: ${{ steps.transform.outputs.versions }}
modules: ${{ steps.transform.outputs.modules }}
permutations: ${{ steps.transform.outputs.permutations }}
steps:
- name: Is comment allowed?
uses: actions/github-script@v6
Expand Down Expand Up @@ -55,32 +54,80 @@ jobs:

let modules, versions
try {
const matrix = JSON.parse(fs.readFileSync('./.ci/tav.json'))
const matrix = JSON.parse(fs.readFileSync('./.ci/tav.json'));
versions = matrix.versions
modules = matrix.modules
} catch (err) {
core.setFailed(`Error loading './.ci/tav.json': ${err}`)
return
}

const getPermutations = (mods, vers) => {
const permutations = []
for (const mod of mods) {
for (const nv of vers) {
if (mod.minVersion && nv >= mod.minVersion) {
permutations.push(`${mod.name} ${nv}`)
}
}
}
return permutations
}

const comment = context.payload.review.body
if (comment !== '/test tav') {
const regex = /\/test tav ([^\s]+)(\s*)([^\s]*)/
const match = comment.match(regex)
if (!match) {
core.setFailed(`Incorrect comment, please use /test tav(\\s(module1,...,moduleN)?(\\s)?(node1,...,nodeN)?)?'`)
return
if (comment === '/test tav') {
const permutations = getPermutations(modules, versions)
if (permutations.length > 256) {
core.setFailed(`Matrix size (${permutations.length}) is bigger than the limit (256)`)
} else {
core.setOutput('permutations', permutations)
}
if (match[1]) {
if (match[1] !== 'all') {
modules = match[1].split(',')
return
}

const regex = /\/test tav ([^\s]+)(\s*)([^\s]*)/
const match = comment.match(regex)
if (!match) {
core.setFailed(`Incorrect comment, please use /test tav(\\s(module1,...,moduleN)?(\\s)?(node1,...,nodeN)?)?'`)
return
}

const resolvedModules = []
const resolvedVersions = []
let inputNames, inputVersions
if (match[1]) {
if (match[1] === 'all') {
resolvedModules.push(...modules)
} else {
inputNames = match[1].split(',')
for (const name of inputNames) {
const mod = modules.find((m) => m.name === name)
if (mod) {
resolvedModules.push(mod)
} else {
core.setFailed(`Incorrect module name ${name}, please review it'`)
david-luna marked this conversation as resolved.
Show resolved Hide resolved
return
}
}
}
if (match[3]) {
versions = match[3].split(',')
}
if (match[3]) {
inputVersions = match[3].split(',').map(Number)
if (inputVersions.some((v) => isNaN(v))) {
core.setFailed(`Incorrect versions list ${match[3]}, please review it'`)
david-luna marked this conversation as resolved.
Show resolved Hide resolved
return
}
resolvedVersions.push(...inputVersions)
} else {
resolvedVersions.push(...versions)
}

const permutations = getPermutations(resolvedModules, resolvedVersions)
if (permutations.length > 256) {
core.setFailed(`Matrix size (${permutations.length}) is bigger than the limit (256)`)
return
}
core.setOutput('modules', modules)
core.setOutput('versions', versions)
core.setOutput('permutations', permutations)

test-tav:
needs: command-validation
Expand All @@ -90,14 +137,13 @@ jobs:
max-parallel: 15
fail-fast: false
matrix:
node: ${{ fromJSON(needs.command-validation.outputs.versions) }}
module: ${{ fromJSON(needs.command-validation.outputs.modules) }}
module_and_node: ${{ fromJSON(needs.command-validation.outputs.permutations) }}
steps:

- uses: actions/checkout@v3
with:
ref: ${{ github.event.pull_request.head.sha }}

- run: .ci/scripts/test.sh -b "release" -t "${{ matrix.module }}" "${{ matrix.node }}"
- run: .ci/scripts/test.sh -b "release" -t ${{ matrix.module_and_node }}
env:
ELASTIC_APM_CONTEXT_MANAGER: ''
23 changes: 16 additions & 7 deletions .github/workflows/tav.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ jobs:
runs-on: ubuntu-latest
timeout-minutes: 5
outputs:
versions: ${{ steps.transform.outputs.versions }}
modules: ${{ steps.transform.outputs.modules }}
permutations: ${{ steps.transform.outputs.permutations }}
steps:

- uses: actions/checkout@v3
Expand All @@ -43,8 +42,19 @@ jobs:
core.setFailed(`Error loading './.ci/tav.json': ${err}`)
return
}
core.setOutput('modules', matrix.modules)
core.setOutput('versions', matrix.versions)
const permutations = []
for (const mod of matrix.modules) {
for (const nv of matrix.versions) {
if (mod.minVersion && nv >= mod.minVersion) {
permutations.push(`${mod.name} ${nv}`)
}
}
}
if (permutations.length > 256) {
core.setFailed(`Matrix size (${permutations.length}) is bigger than the limit (256)`)
return
}
core.setOutput('permutations', permutations)

test-tav:
needs: prepare-matrix
Expand All @@ -57,10 +67,9 @@ jobs:
# A job matrix limit is 256. We do some grouping of TAV modules to
# stay under that limit.
# https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration
node: ${{ fromJSON(needs.prepare-matrix.outputs.versions) }}
module: ${{ fromJSON(needs.prepare-matrix.outputs.modules) }}
module_and_node: ${{ fromJSON(needs.prepare-matrix.outputs.permutations) }}
steps:
- uses: actions/checkout@v3
- run: .ci/scripts/test.sh -b "release" -t "${{ matrix.module }}" "${{ matrix.node }}"
- run: .ci/scripts/test.sh -b "release" -t ${{ matrix.module_and_node }}
env:
ELASTIC_APM_CONTEXT_MANAGER: ''
24 changes: 17 additions & 7 deletions dev-utils/lint-tav-json.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,33 +35,43 @@ function main(argv) {
const tavYmlPaths = glob.sync('**/.tav.yml', {
ignore: ['**/node_modules/**'],
});
// console.log('tavYmlPaths:', tavYmlPaths)
// console.log('tavYmlPaths:', tavYmlPaths);
tavYmlPaths.forEach((p) => {
const tavCfg = yaml.load(fs.readFileSync(p, 'utf8'));
Object.keys(tavCfg).forEach((k) => {
const v = tavCfg[k];
moduleNamesFromYaml.add(v.name || k);
});
});
// console.log('moduleNamesFromYaml: ', moduleNamesFromYaml)
// console.log('moduleNamesFromYaml: ', moduleNamesFromYaml);

// Find module names in ".ci/tav.json".
const moduleNamesFromJson = new Set();
const tavJson = JSON.parse(fs.readFileSync(path.join(TOP, TAV_JSON_PATH)));
tavJson.modules.forEach((m) => {
m.split(',').forEach((moduleName) => {
m.name.split(',').forEach((moduleName) => {
moduleNamesFromJson.add(moduleName);
});
});
// console.log('moduleNamesFromJson: ', moduleNamesFromJson)
const matrix = [];
for (const mod of tavJson.modules) {
mod.name.split(',').forEach((moduleName) => {
moduleNamesFromJson.add(moduleName);
});
for (const nv of tavJson.versions) {
if (mod.minVersion && nv >= mod.minVersion) {
matrix.push(`${mod.name} ${nv}`);
}
}
}
// console.log('moduleNamesFromJson: ', moduleNamesFromJson);

// Matrix 256 limit.
const matrixSize = tavJson.versions.length * tavJson.modules.length;
if (matrixSize > 256) {
if (matrix.length > 256) {
console.error(
'lint-tav-json: #versions * #modules from "%s" is >256, which exceeds the GH Actions workflow matrix limit: %d',
TAV_JSON_PATH,
matrixSize,
matrix.length,
david-luna marked this conversation as resolved.
Show resolved Hide resolved
);
numErrors += 1;
}
Expand Down
Loading