-
Notifications
You must be signed in to change notification settings - Fork 7
Use test-actions as the standard place to test the automations #99
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
Changes from all commits
4e038d1
3a50c1c
487680a
da50b0b
bf5facb
38c4734
1ea196b
7f9a562
cad6b69
ad4e772
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| # Testing the automations | ||
|
|
||
| Test in [`learningequality/test-actions`](https://github.com/learningequality/test-actions). The bot | ||
| app is installed there and the secrets resolve, so there is nothing to set up. | ||
|
|
||
| It holds a copy of `automation-template.yml` at `.github/workflows/automation.yml`, making it a | ||
| consumer like any other. Every automation runs there against the reusable workflows on `main`. | ||
|
|
||
| ## Testing an automation | ||
|
|
||
| Trigger the event you want to test: open a pull request, comment on an issue, add a label, etc. Then | ||
| check the run in the Actions tab. | ||
|
|
||
| Jobs that do not match the event are reported as skipped, which is normal. If everything is skipped, | ||
| the `if:` condition on each job did not match, so check the event first. | ||
|
|
||
| ### Automations that need an outside contributor | ||
|
|
||
| Some automations run only when `is-contributor` is true, meaning the author is not a member of the | ||
| organization. An org member cannot trigger them, so they skip. These are `review-requested`, | ||
| `pull-request-label`, `contributor-pr-reply`, `contributor-issue-comment`, and | ||
| `update-pr-spreadsheet`, plus `holiday-message` when enabled. | ||
|
|
||
| Use a second GitHub account that is not in the organization. It needs no permissions, secrets, or | ||
| app. To open a pull request, fork `test-actions` and open one back to it. `pull_request_target` then | ||
| runs in `test-actions` with `test-actions`' secrets rather than the fork's. For an issue or comment, | ||
| no fork is needed. | ||
|
|
||
| Keep that account out of the organization. Adding it makes these automations skip again. | ||
|
|
||
| `update-pr-spreadsheet` writes to a test sheet rather than the production sheet because | ||
| `CONTRIBUTIONS_SPREADSHEET_ID` and `CONTRIBUTIONS_SHEET_NAME` are set as repository secrets there. A | ||
| repository secret takes precedence over an organization secret with the same name. | ||
|
|
||
| ## Testing the sync workflow | ||
|
|
||
| Run `Sync automation template` from the Actions tab with `only` set to `test-actions`. This limits | ||
| the run to one repo, so it cannot open a pull request anywhere else. Leave `dry_run` on to see what | ||
| it would do, or turn it off to let it open a pull request. | ||
|
|
||
| Without `only`, the workflow considers every consumer in the organization, which is how it runs on | ||
| its schedule. | ||
|
|
||
| To see it propose a change, modify `.github/workflows/automation.yml` in `test-actions` so that it | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Resolved — addressed in the current code. suggestion: Closing the test sync PR without reverting the drift leaves
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed in cad6b69 to say merge or revert the drift. One correction: the pull request is not reopened. Closing it unmerged gives |
||
| differs from the template, then run it with `only: test-actions`. | ||
|
|
||
| ## Testing a change to a reusable workflow | ||
|
|
||
| Point a caller at your branch, for example: | ||
|
|
||
| `uses: learningequality/.github/.github/workflows/automation.yml@my-branch` | ||
|
|
||
| Put that caller at a path other than `.github/workflows/automation.yml`. The sync reads exactly that | ||
| path, so a branch-pinned caller there looks like drift and gets a pull request on every run. | ||
|
|
||
| ## Clearing up | ||
|
|
||
| Close test pull requests and delete their branches. Leave `.github/workflows/automation.yml` in | ||
| place, since the repo is only a consumer while that file exists. | ||
|
|
||
| If you drifted the copy to make the sync propose something, either merge the sync pull request or | ||
| revert the drift. Closing it without merging leaves test-actions out of sync, so the workflow | ||
| reports it as declined and stops proposing updates until the template changes. | ||
|
|
||
| ## Testing in a separate organization | ||
|
|
||
| Changing the bot app, using different secret values, or testing discovery across several repos | ||
| requires an organization you control. See | ||
| [testing in a separate organization](./testing-in-a-separate-org.md). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # Testing in a separate organization | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Resolved — addressed in the current code. suggestion: This page is beyond #98's deliverables, which list the sandbox org as out of scope. #98 asks only for
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The page was requested by the issue author during implementation, so #98 is now updated to cover it rather than the page being dropped. |
||
|
|
||
| Most testing belongs in `learningequality/test-actions`, where the app and secrets are already in | ||
| place. See [testing the automations](./testing-automations.md). | ||
|
|
||
| A separate organization is worth the setup when: | ||
|
|
||
| * You are changing the bot app itself, its permissions, or its installation scope. Those changes | ||
| cannot be tested on the production app. | ||
| * You need different secret values, such as another Slack webhook, without changing what everyone | ||
| else testing in `test-actions` sees. | ||
| * You are changing discovery. `--only` covers one repo, while an organization lets you arrange | ||
| several repos: in sync, drifted, archived, and a fork. | ||
|
|
||
| ## Setting one up | ||
|
|
||
| 1. Create an organization, or use one you own. | ||
| 2. Register a GitHub App in it with repository permissions for contents, issues, and pull requests | ||
| set to read and write, and metadata set to read. Install it on the repos you will test with. | ||
| 3. Generate a private key for the app. Set `LE_BOT_APP_ID` to the app's ID and `LE_BOT_PRIVATE_KEY` | ||
| to the contents of the key file, at the organization or repository level. | ||
| 4. Add `SLACK_WEBHOOK_URL` and `SLACK_COMMUNITY_NOTIFICATIONS_WEBHOOK_URL` if you want to test the | ||
| Slack automations. Without them, those steps fail, but the rest still runs. | ||
| 5. For the spreadsheet, create a Google Cloud project, enable the Google Sheets API, and create a | ||
| service account. On the service account, open the Keys tab, add a key, and choose JSON. Set | ||
| `GH_UPLOADER_GCP_SA_CREDENTIALS` to the contents of the file. | ||
| 6. Create a Google Sheet and share it with the service account's `client_email` as an editor. That | ||
| sharing is the only permission that matters, since no IAM role is involved. Set | ||
| `CONTRIBUTIONS_SPREADSHEET_ID` to the ID in the sheet's URL and `CONTRIBUTIONS_SHEET_NAME` to the | ||
| tab name. | ||
| 7. Copy `automation-template.yml` into a repo there as `.github/workflows/automation.yml`. | ||
|
|
||
| ## Running against it | ||
|
|
||
| Set `SYNC_ORG` to the organization: | ||
|
|
||
| ```bash | ||
| SYNC_ORG=<org> GITHUB_TOKEN=<token> node scripts/sync-automation-template.js --dry-run | ||
| ``` | ||
|
|
||
| `SYNC_ORG` and `--only` compose, so a run can still be narrowed to one repo within the organization. | ||
|
|
||
| Installation is per repo and separate from the app's permissions. A repo the app is not installed on | ||
| never appears because discovery only sees what the token can see. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,9 @@ | |
| * drifted from automation-template.yml. | ||
| * | ||
| * Usage: | ||
| * node scripts/sync-automation-template.js open or update pull requests | ||
| * node scripts/sync-automation-template.js --dry-run report only, change nothing | ||
| * node scripts/sync-automation-template.js open or update pull requests | ||
| * node scripts/sync-automation-template.js --dry-run report only, change nothing | ||
| * node scripts/sync-automation-template.js --only=repo limit the run to one repo | ||
| * | ||
| * Requires GITHUB_TOKEN with contents:write and pull-requests:write on each | ||
| * consumer repo. It never commits to a default branch and never merges. | ||
|
|
@@ -136,32 +137,52 @@ async function readCopy(api, repo, ref) { | |
| * skipped because Actions do not run on them, and forks because their copy | ||
| * belongs to the upstream repo. | ||
| */ | ||
| async function findConsumers(api) { | ||
| async function findConsumers(api, only) { | ||
| const repos = []; | ||
| for (let page = 1; ; page += 1) { | ||
| if (only) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Resolved — addressed in the current code. suggestion: An excluded
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in cad6b69. A named repo that is archived, a fork, or without the marker now says which, and exits 1. Live: |
||
| const r = await api('GET', `/repos/${ORG}/${only}`); | ||
| if (!r.ok) throw new Error(`could not read ${only} (${detail(r)})`); | ||
| repos.push(r.data); | ||
| } | ||
| for (let page = 1; !only; page += 1) { | ||
| const r = await api('GET', `/orgs/${ORG}/repos?per_page=100&type=all&page=${page}`); | ||
| if (!r.ok) throw new Error(`could not list the org's repos (${detail(r)})`); | ||
| repos.push(...r.data); | ||
| if (r.data.length < 100) break; | ||
| } | ||
|
|
||
| // Skipping a named repo would report no consumers and exit 0, which reads as | ||
| // "in sync". Say why instead. | ||
| const skip = (repo, why) => { | ||
| if (only) throw new Error(`${repo.name} is not a consumer: ${why}`); | ||
| }; | ||
|
|
||
| const consumers = []; | ||
| for (const repo of repos) { | ||
| if (repo.archived || repo.fork) continue; | ||
| if (repo.archived || repo.fork) { | ||
| skip(repo, repo.archived ? 'archived' : 'a fork'); | ||
| continue; | ||
| } | ||
| let copy; | ||
| try { | ||
| copy = await readCopy(api, repo.name, repo.default_branch); | ||
| } catch (err) { | ||
| copy = { error: err.message }; | ||
| } | ||
| if (copy.missing) continue; | ||
| if (copy.missing) { | ||
| skip(repo, `no ${TARGET_PATH}`); | ||
| continue; | ||
| } | ||
| if (copy.error) { | ||
| consumers.push({ repo: repo.name, base: repo.default_branch, unreadable: copy.error }); | ||
| continue; | ||
| } | ||
| // This repo's own reusable automation.yml sits at the same path and does not | ||
| // call the shared workflow, so this excludes it without a special case. | ||
| if (!copy.content.includes(CONSUMER_MARKER)) continue; | ||
| if (!copy.content.includes(CONSUMER_MARKER)) { | ||
| skip(repo, 'its automation.yml does not call the shared workflow'); | ||
| continue; | ||
| } | ||
| consumers.push({ repo: repo.name, base: repo.default_branch }); | ||
| } | ||
| return consumers; | ||
|
|
@@ -272,7 +293,7 @@ async function syncRepo(api, consumer, template, { dryRun }) { | |
| } | ||
|
|
||
| async function run(api, template, options) { | ||
| const consumers = await findConsumers(api); | ||
| const consumers = await findConsumers(api, options.only); | ||
|
|
||
| const results = []; | ||
| for (const consumer of consumers) { | ||
|
|
@@ -299,17 +320,44 @@ function report(results) { | |
| return problems.length; | ||
| } | ||
|
|
||
| // Both spellings, because an --only nobody parses is a full run against every | ||
| // consumer, which is the opposite of what it asks for. | ||
| function parseArgs(argv) { | ||
| const i = argv.findIndex((a) => a === '--only' || a.startsWith('--only=')); | ||
| let only; | ||
| if (i !== -1) { | ||
| only = argv[i] === '--only' ? argv[i + 1] : argv[i].slice('--only='.length); | ||
| if (!only || only.startsWith('--')) throw new Error('--only needs a repo name'); | ||
| } | ||
| return { dryRun: argv.includes('--dry-run'), only }; | ||
| } | ||
|
|
||
| async function main() { | ||
| const token = process.env.GITHUB_TOKEN; | ||
| if (!token) { | ||
| console.error('GITHUB_TOKEN is not set.'); | ||
| process.exit(1); | ||
| } | ||
| const template = fs.readFileSync(TEMPLATE_PATH, 'utf8'); | ||
| const results = await run(httpApi(token), template, { dryRun: process.argv.includes('--dry-run') }); | ||
| const results = await run(httpApi(token), template, parseArgs(process.argv.slice(2))); | ||
| process.exit(report(results) ? 1 : 0); | ||
| } | ||
|
|
||
| if (require.main === module) main(); | ||
| if (require.main === module) { | ||
| main().catch((err) => { | ||
| console.error(err.message); | ||
| process.exit(1); | ||
| }); | ||
| } | ||
|
|
||
| module.exports = { run, syncRepo, classifyDrift, findConsumers, report, PROBLEM_STATES, BRANCH, TARGET_PATH }; | ||
| module.exports = { | ||
| run, | ||
| syncRepo, | ||
| classifyDrift, | ||
| findConsumers, | ||
| parseArgs, | ||
| report, | ||
| PROBLEM_STATES, | ||
| BRANCH, | ||
| TARGET_PATH, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,15 @@ | ||
| const test = require('node:test'); | ||
| const assert = require('node:assert/strict'); | ||
|
|
||
| const { run, classifyDrift, findConsumers, report, PROBLEM_STATES, BRANCH } = require('./sync-automation-template'); | ||
| const { | ||
| run, | ||
| classifyDrift, | ||
| findConsumers, | ||
| parseArgs, | ||
| report, | ||
| PROBLEM_STATES, | ||
| BRANCH, | ||
| } = require('./sync-automation-template'); | ||
|
|
||
| const USES = 'jobs:\n automation:\n uses: learningequality/.github/.github/workflows/automation.yml@main\n'; | ||
| const TEMPLATE = `name: Automation\non: {}\n${USES}`; | ||
|
|
@@ -375,6 +383,68 @@ test('a stale branch is reset to base when no pull request is open', async () => | |
| assert.equal(reset.body.force, true); | ||
| }); | ||
|
|
||
| test('both spellings of --only are parsed, because an ignored one runs against everything', () => { | ||
| assert.equal(parseArgs(['--only=demo']).only, 'demo'); | ||
| assert.equal(parseArgs(['--only', 'demo']).only, 'demo'); | ||
| assert.equal(parseArgs(['--dry-run', '--only', 'demo']).dryRun, true); | ||
| assert.equal(parseArgs(['--dry-run']).only, undefined); | ||
| }); | ||
|
|
||
| test('--only without a repo name is an error, not a full run', () => { | ||
| assert.throws(() => parseArgs(['--only']), /needs a repo name/); | ||
| assert.throws(() => parseArgs(['--only=']), /needs a repo name/); | ||
| assert.throws(() => parseArgs(['--only', '--dry-run']), /needs a repo name/); | ||
| }); | ||
|
|
||
| test('a named repo that is not a consumer says why, rather than reporting none', async () => { | ||
| const cases = [ | ||
| [repo({ archived: true }), /archived/], | ||
| [repo({ fork: true }), /a fork/], | ||
| ]; | ||
| for (const [r, expected] of cases) { | ||
| const routes = [...baseRoutes(TEMPLATE), ['GET =/repos/learningequality/demo', ok(r)]]; | ||
| await assert.rejects(() => run(makeApi(routes), TEMPLATE, { only: 'demo' }), expected); | ||
| } | ||
| }); | ||
|
|
||
| test('a named repo without the marker says why', async () => { | ||
| const routes = [ | ||
| ...baseRoutes(TEMPLATE), | ||
| ['GET =/repos/learningequality/demo', ok(repo())], | ||
| ['GET contents/.github/workflows/automation.yml', ok({ sha: 'x', content: encode('name: other\n') })], | ||
| ]; | ||
| await assert.rejects(() => run(makeApi(routes), TEMPLATE, { only: 'demo' }), /does not call the shared workflow/); | ||
| }); | ||
|
|
||
| test('only limits the run to that repo, and never lists the org', async () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Resolved — addressed in the current code. praise: Asserting no call touches |
||
| const calls = []; | ||
| const others = [repo({ name: 'demo' }), repo({ name: 'production-repo' })]; | ||
| const routes = [...baseRoutes(TEMPLATE, others), ['GET =/repos/learningequality/demo', ok(repo())]]; | ||
| const results = await run(makeApi(routes, calls), TEMPLATE, { only: 'demo' }); | ||
| assert.deepEqual( | ||
| results.map((r) => r.repo), | ||
| ['demo'] | ||
| ); | ||
| assert.ok( | ||
| !calls.some((c) => c.url.includes('/orgs/')), | ||
| 'the org listing is the path to every other repo, so it must not be walked' | ||
| ); | ||
| }); | ||
|
|
||
| test('without only, every repo in the listing is considered', async () => { | ||
| const others = [repo({ name: 'demo' }), repo({ name: 'production-repo' })]; | ||
| const results = await run(makeApi(baseRoutes(TEMPLATE, others)), TEMPLATE, {}); | ||
| assert.deepEqual( | ||
| results.map((r) => r.repo), | ||
| ['demo', 'production-repo'] | ||
| ); | ||
| }); | ||
|
|
||
| test('only reports an error when the repo cannot be read', async () => { | ||
| const routes = [...baseRoutes(TEMPLATE), ['GET =/repos/learningequality/missing', fail(404, 'Not Found')]]; | ||
| await assert.rejects(() => run(makeApi(routes), TEMPLATE, { only: 'missing' }), /could not read missing/); | ||
| }); | ||
|
|
||
| test('findConsumers pages through the org listing', async () => { | ||
| const calls = []; | ||
| const many = Array.from({ length: 100 }, (_, i) => repo({ name: `r${i}` })); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Resolved — addressed in the current code.
nitpick: This link-text rewording is unrelated to #98. Please drop it from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated so both internal doc links read the same way.