Skip to content

fix: migrate all octokit API calls to .rest namespace for probot v14 compatibility#949

Merged
decyjphr merged 5 commits intomain-enterprisefrom
copilot/fix-github-actions-workflow
Mar 23, 2026
Merged

fix: migrate all octokit API calls to .rest namespace for probot v14 compatibility#949
decyjphr merged 5 commits intomain-enterprisefrom
copilot/fix-github-actions-workflow

Conversation

Copy link
Contributor

Copilot AI commented Mar 23, 2026

Summary

Probot v14 uses @octokit/plugin-rest-endpoint-methods v17, which only exposes API endpoint namespaces under .rest.* — they are no longer mounted at the root of the octokit instance. Every call like octokit.repos.*, octokit.issues.*, octokit.apps.*, etc. was silently undefined at runtime.

The info() fix from the problem statement (using github.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.jscontext.octokit.checks.*, context.octokit.pulls.*, github.apps.*
  • lib/configManager.jsthis.context.octokit.repos.getContent
  • lib/settings.jsthis.github.repos.*, this.github.checks.*, this.github.issues.*, this.github.git.*
  • lib/plugins/archive.jsthis.github.repos.*
  • lib/plugins/autolinks.jsthis.github.repos.*
  • lib/plugins/branches.jsthis.github.repos.*
  • lib/plugins/collaborators.jsthis.github.repos.*
  • lib/plugins/custom_properties.jsthis.github.repos.*
  • lib/plugins/labels.jsthis.github.repos.*, this.github.issues.*
  • lib/plugins/milestones.jsthis.github.issues.*
  • lib/plugins/repository.jsthis.github.repos.*
  • lib/plugins/teams.jsthis.github.repos.*, this.github.teams.*
  • lib/plugins/validator.jsthis.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.0 that was added as a workaround in a previous session.

Pattern

// Before (probot v13 / plugin-rest-endpoint-methods ≤v13)
github.apps.listInstallations.endpoint.merge({ per_page: 100 })
this.github.repos.getContent(params)

// After (probot v14 / plugin-rest-endpoint-methods v17)
github.rest.apps.listInstallations.endpoint.merge({ per_page: 100 })
this.github.rest.repos.getContent(params)

paginate(), request(), and raw-string request.endpoint() calls are unaffected — those remain on the root instance.

Copilot AI and others added 3 commits March 23, 2026 13:19
…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
Copilot AI requested a review from decyjphr March 23, 2026 14:25
@decyjphr decyjphr marked this pull request as ready for review March 23, 2026 15:41
Copilot AI review requested due to automatic review settings March 23, 2026 15:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 HOST env 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 NopCommand invocation is missing the repo argument and will cause nop-mode execution to fail. Ensure the second argument is this.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.

@decyjphr
Copy link
Collaborator

@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.

PASS test/unit/lib/plugins/teams.test.js
  ● Console

    console.error
      Unexpected error when fetching for security manager teams org bkeepers = TypeError: this.github.request.endpoint is not a function

      46 |         this.log.debug(`${this.repo.owner} Org does not have Security manager teams set up ${e}`)
      47 |       } else {
    > 48 |         this.log.error(
         |                  ^
      49 |         `Unexpected error when fetching for security manager teams org ${this.repo.owner} = ${e}`
      50 |         )
      51 |       }

      at Teams.error [as checkSecurityManager] (lib/plugins/teams.js:48:18)
      at checkSecurityManager (lib/plugins/teams.js:10:19)
      at Object.<anonymous> (test/unit/lib/plugins/teams.test.js:63:7)

decyjphr and others added 2 commits March 23, 2026 11:54
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
Copy link
Contributor Author

Copilot AI commented Mar 23, 2026

@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.

...

Fixed in e473adf. The root cause was that the request mock in teams.test.js didn't have an endpoint property, so this.github.request.endpoint(...) inside the checkSecurityManager debug log threw a TypeError. This was caught by the catch block and logged via console.error (since log.error = console.error), but the test still passed because the sync completed successfully.

Three changes to fix the code and workflow:

  1. Added endpoint: jest.fn().mockReturnValue({}) to the request mock in teams.test.js
  2. Fixed the paginate mock to handle string-path calls (used by checkSecurityManager for the security-managers API)
  3. Added test/setup.js (wired via setupFilesAfterEnv in package.json) that throws on unexpected console.error calls in tests — this ensures future similar issues fail the test suite immediately

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.branch is a string (the branch name), so params.branch.name will always be undefined. This makes the generated results.msg inaccurate (and any downstream reporting/NOP output will reference an undefined branch). Use the branch name directly (e.g., params.branch or the local branch.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() on getContent() logs and then returns undefined, but the code below unconditionally reads response.data, which will throw a TypeError and bypass the intended e.status === 404 handling. Prefer letting the outer try/catch handle errors (remove the inner .catch()), or rethrow/return a sentinel value and guard before accessing response.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.

Copy link
Collaborator

@decyjphr decyjphr left a comment

Choose a reason for hiding this comment

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

ok

@decyjphr decyjphr merged commit 16feaad into main-enterprise Mar 23, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants