fix: migrate all octokit API calls to .rest namespace for probot v14 compatibility#949
Conversation
…ogic Co-authored-by: decyjphr <57544838+decyjphr@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/safe-settings/sessions/97b10121-f26e-44c0-86e6-3ede047fe176
Co-authored-by: decyjphr <57544838+decyjphr@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/safe-settings/sessions/97b10121-f26e-44c0-86e6-3ede047fe176
…compatibility and revert Dockerfile ENV HOST Co-authored-by: decyjphr <57544838+decyjphr@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/safe-settings/sessions/179c9d77-8ca0-4098-9017-8a255df170f9
There was a problem hiding this comment.
Pull request overview
This PR updates the app’s GitHub API usage to be compatible with Probot v14 / @octokit/plugin-rest-endpoint-methods v17 by migrating endpoint calls from top-level namespaces (e.g., octokit.repos.*) to the .rest.* namespace, and adjusts unit test mocks accordingly.
Changes:
- Migrated Octokit REST endpoint calls across the app to
octokit.rest.*(runtime compatibility fix for Probot v14). - Updated unit tests to mock
github.rest.*namespaces instead of top-level namespaces. - Improved functional-test workflow diagnostics (capture container ID, print logs, make curl retries stricter) and removed a Dockerfile
HOSTenv var.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| index.js | Migrates checks, pulls, and apps API calls to context.octokit.rest.* / github.rest.*. |
| lib/configManager.js | Migrates getContent to context.octokit.rest.repos.getContent. |
| lib/settings.js | Migrates repos, checks, issues, and git calls to this.github.rest.*. |
| lib/plugins/archive.js | Migrates repo get/update calls and nop endpoints to github.rest.repos.*. |
| lib/plugins/autolinks.js | Migrates autolinks calls + nop endpoints to github.rest.repos.*. |
| lib/plugins/branches.js | Migrates branch protection calls + nop endpoints to github.rest.repos.*. |
| lib/plugins/collaborators.js | Migrates collaborator/invitation calls + nop endpoints to github.rest.repos.*. |
| lib/plugins/custom_properties.js | Migrates custom properties endpoints to github.rest.repos.*. |
| lib/plugins/labels.js | Migrates label endpoints and repo existence check to github.rest.*. |
| lib/plugins/milestones.js | Migrates milestones endpoints to github.rest.issues.*. |
| lib/plugins/repository.js | Migrates repo management endpoints to github.rest.repos.* (but introduces/retains nop-mode issues noted in comments). |
| lib/plugins/teams.js | Migrates team endpoints to github.rest.* and listTeams pagination to use github.rest.repos.listTeams. |
| lib/plugins/validator.js | Migrates topics endpoints to github.rest.repos.*. |
| test/unit/lib/settings.test.js | Updates octokit mocks/spies to rest.repos.getContent. |
| test/unit/lib/plugins/*.test.js | Updates plugin test mocks/expectations to github.rest.*. |
| Dockerfile | Removes ENV HOST=0.0.0.0. |
| .github/workflows/create-pre-release.yml | Captures container ID, prints logs, and makes curl retries fail-fast. |
Comments suppressed due to low confidence (1)
lib/plugins/repository.js:308
- Same issue as above: this
NopCommandinvocation is missing therepoargument and will cause nop-mode execution to fail. Ensure the second argument isthis.repo, followed by the endpoint options and action string.
} else {
this.log.debug(`Disabling Dependabot security updates for owner: ${repoData.owner.login} and repo ${repoData.name}`)
if (this.nop) {
resArray.push((new NopCommand(this.constructor.name, this.github.rest.repos.disableAutomatedSecurityFixes.endpoint({
owner: repoData.owner.login,
repo: repoData.name
}), 'Disabling Dependabot security updates')))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot I noticed that unit tests had one error but it was not propogated so the CI workflow passed. We should fix the code and the workflow. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…teams test error propagation Co-authored-by: decyjphr <57544838+decyjphr@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/safe-settings/sessions/d9420b15-2cca-40a3-a30b-869df73487f1
... Fixed in e473adf. The root cause was that the Three changes to fix the code and workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
lib/plugins/branches.js:61
params.branchis a string (the branch name), soparams.branch.namewill always beundefined. This makes the generatedresults.msginaccurate (and any downstream reporting/NOP output will reference an undefined branch). Use the branch name directly (e.g.,params.branchor the localbranch.name) when formatting this message.
const mergeDeep = new MergeDeep(this.log, this.github, ignorableFields)
const changes = mergeDeep.compareDeep({ branch: { protection: this.reformatAndReturnBranchProtection(result.data) } }, { branch: { protection: Overrides.removeOverrides(overrides, branch.protection, result.data) } })
const results = { msg: `Followings changes will be applied to the branch protection for ${params.branch.name} branch`, additions: changes.additions, modifications: changes.modifications, deletions: changes.deletions }
this.log.debug(`Result of compareDeep = ${results}`)
lib/configManager.js:29
- The inner
.catch()ongetContent()logs and then returnsundefined, but the code below unconditionally readsresponse.data, which will throw aTypeErrorand bypass the intendede.status === 404handling. Prefer letting the outertry/catchhandle errors (remove the inner.catch()), or rethrow/return a sentinel value and guard before accessingresponse.data.
const repo = { owner: this.context.repo().owner, repo: env.ADMIN_REPO }
const params = Object.assign(repo, { path: filePath, ref: this.ref })
const response = await this.context.octokit.rest.repos.getContent(params).catch(e => {
this.log.error(`Error getting settings ${e}`)
})
// Ignore in case path is a folder
// - https://developer.github.com/v3/repos/contents/#response-if-content-is-a-directory
if (Array.isArray(response.data)) {
return null
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Probot v14 uses
@octokit/plugin-rest-endpoint-methodsv17, which only exposes API endpoint namespaces under.rest.*— they are no longer mounted at the root of the octokit instance. Every call likeoctokit.repos.*,octokit.issues.*,octokit.apps.*, etc. was silentlyundefinedat runtime.The
info()fix from the problem statement (usinggithub.rest.apps.listInstallations) is the correct pattern. This PR applies the same fix consistently across the entire codebase.Changes
Source files (84 call sites across 13 files)
index.js—context.octokit.checks.*,context.octokit.pulls.*,github.apps.*lib/configManager.js—this.context.octokit.repos.getContentlib/settings.js—this.github.repos.*,this.github.checks.*,this.github.issues.*,this.github.git.*lib/plugins/archive.js—this.github.repos.*lib/plugins/autolinks.js—this.github.repos.*lib/plugins/branches.js—this.github.repos.*lib/plugins/collaborators.js—this.github.repos.*lib/plugins/custom_properties.js—this.github.repos.*lib/plugins/labels.js—this.github.repos.*,this.github.issues.*lib/plugins/milestones.js—this.github.issues.*lib/plugins/repository.js—this.github.repos.*lib/plugins/teams.js—this.github.repos.*,this.github.teams.*lib/plugins/validator.js—this.github.repos.*Test files
All test mocks updated to match:
github.rest.repos.*/github.rest.issues.*/github.rest.teams.*instead of top-level namespaces.Dockerfile
Removed
ENV HOST=0.0.0.0that was added as a workaround in a previous session.Pattern
paginate(),request(), and raw-stringrequest.endpoint()calls are unaffected — those remain on the root instance.