fix(branches): incorporate PR #1017 log/label fixes + mitigate integration appId harness - #1046
Open
decyjphr wants to merge 1 commit into
Open
fix(branches): incorporate PR #1017 log/label fixes + mitigate integration appId harness#1046decyjphr wants to merge 1 commit into
decyjphr wants to merge 1 commit into
Conversation
…ation appId harness Incorporate PR #1017 (absent from this branch), adapted to this branch's `this.github.repos` convention: - Branch-protection diff message read `params.branch.name` (always undefined, since `params.branch` is already the branch string) -> use `params.branch`, and JSON.stringify the results in the debug log. - NOP update path (protection already exists) was mislabeled 'Add Branch Protection' -> 'Update Branch Protection' (debug 'Updating'); the 404/add path keeps its 'Add' label. - Add NOP-mode unit tests asserting the update label when protection exists, the add label on 404, and that the diff message names the real branch. Integration harness: `createProbot` in probot 13 only reads overrides/defaults/env, so the old `{ id, cert, githubToken }` args were dropped, making @octokit/auth-app throw "appId option is required". Pass dummy credentials via `overrides` (token auth) and stub the startup `/app/installations` call so the app loads under nock.disableNetConnect(). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c735bbe7-feb9-472f-827c-d56ddfe7fe6a
Contributor
There was a problem hiding this comment.
Pull request overview
This PR backports and adapts PR #1017’s branch-protection logging / dry-run (NOP) labeling fixes to this branch’s this.github.repos.* Octokit usage, and updates the integration test harness so Probot can instantiate under Probot 13 without failing on missing appId credentials.
Changes:
- Fix branch-protection diff messaging to reference the actual branch string and improve debug output formatting.
- Distinguish NOP-mode branch-protection “update” vs “add” actions via clearer
NopCommandlabels and logs. - Update the integration harness to pass credentials via
createProbot({ overrides: ... })and stub the startup/app/installationscall to work undernock.disableNetConnect().
Show a summary per file
| File | Description |
|---|---|
| lib/plugins/branches.js | Corrects branch name usage in protection diff messages; improves debug log formatting; fixes NOP-mode update labeling/logging. |
| test/unit/lib/plugins/branches.test.js | Adds NOP-mode unit coverage asserting correct labels and that diff messages include the real branch name. |
| test/integration/common.js | Updates Probot 13 harness initialization via overrides and stubs the app info() installations request to prevent startup failures under nock. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
PR #1017 fixes two long-standing cosmetic-but-confusing bugs in the branch-protection plugin's log and dry-run output, and those fixes were not present in this branch. This brings them in (adapted to this branch's
this.github.reposconvention rather than the PR'sthis.github.rest.repos). It also unblocks the integration test harness, which currently cannot even instantiate Probot.What changed
Branch-protection log/label fixes (
lib/plugins/branches.js)params.branch.name, butparams.branchis already the branch string, so every branch-protection diff in logs and dry-run reports said "for undefined branch". Fixed to useparams.branch, and the adjacent debug log nowJSON.stringifys the results object instead of interpolating[object Object].getBranchProtectionsucceeds (protection already exists, so this is an update) pushed aNopCommandlabeledAdd Branch Protection, identical to the real 404/add path. Plans could not tell "create protection" from "change protection". The update path now saysUpdate Branch Protection(debug "Updating"); the 404 path keeps itsAddlabel.in nop modeunit block asserting the update label when protection exists, the add label on 404, and that the diff message names the actual branch (neverundefined).Integration harness appId mitigation (
test/integration/common.js)createProbotonly readsoverrides/defaults/env, so the old positional{ id, cert, githubToken }args were silently dropped, leaving no credentials and making@octokit/auth-appthrowappId option is requiredbefore any test ran. The harness now passes dummy credentials viaoverrides(agithubToken, which selects token auth and avoids the app-auth JWT/installation-token calls the nock scopes don't mock).info()on load, which lists app installations. That startup call is stubbed with an empty list so the app loads cleanly undernock.disableNetConnect()without interfering with the per-test scopes.Notes for reviewers
this.github.repos.*(notthis.github.rest.repos.*), so the incorporated code differs textually from PR fix(branches): correct branch name in protection log and nop label #1017 while preserving its intent.appId option is requirederror is fully gone). Some legacy per-plugin integration tests still fail for a separate, pre-existing reason: they predate this branch's org-level admin-config loading and expect aninstallationin the event payload. Fully greening them is a larger harness/fixture rewrite and is intentionally out of scope here.Testing
gh apidrift-setup steps, and enterprise app-installation apply timeouts) and unrelated to this change.