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

feat: Add support for checking all changes of PR #767

Open
wants to merge 2 commits into
base: main-enterprise
Choose a base branch
from

Conversation

stokkie90
Copy link

@stokkie90 stokkie90 commented Feb 28, 2025

This Adds support for checking entire PR context instead of only the last commit.
Also fixes a bunch of errors making sure it uses the correct Repo when looping over the list. When setting RepoConfig it Assigned the object to the last one in the loop of Repositories. Resulting in wrong comparison.

Log of the RepoConfig error:

DEBUG (event): Processing repo {"visibility":"internal","name":"sdlc-test-changelog","org":"Org-123"}
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): Process normally... Not a SubOrg config change or SubOrg config was changed and this repo is part of it. {"owner":"Org-123","repo":"sdlc-test-changelog"} suborg config undefined
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): Processing Merged repoConfig {"visibility":"internal","name":"sdlc-test-changelog","org":"Org-123"}
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): sdlc-poc not in restricted repos admin,.github,safe-settings
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): Processing repo {"visibility":"internal","name":"sdlc-poc","org":"Org-123"}
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): Process normally... Not a SubOrg config change or SubOrg config was changed and this repo is part of it. {"owner":"Org-123","repo":"sdlc-poc"} suborg config undefined
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): Processing Merged repoConfig {"visibility":"internal","name":"sdlc-poc","org":"Org-123"}
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): data-analysis-eda not in restricted repos admin,.github,safe-settings
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): Processing repo {"visibility":"internal","name":"data-analysis-eda","org":"Org-123"}
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): Process normally... Not a SubOrg config change or SubOrg config was changed and this repo is part of it. {"owner":"Org-123","repo":"data-analysis-eda"} suborg config undefined
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): Processing Merged repoConfig {"visibility":"internal","name":"data-analysis-eda","org":"Org-123"}
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
    
 #### FROM HERE IT ONLY USED `test-repo-1`   
DEBUG (octokit): GitHub request: GET https://api.github.com/repos/Org-123/spec-api-Org-123-deep-integration - 200
DEBUG (event): Repo Org-123/spec-api-Org-123-deep-integration is not archived, proceed as usual.
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): found a matching repoconfig for this repo {"visibility":"internal","name":"data-analysis-eda","org":"Org-123"}
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): suborg config for spec-api-Org-123-deep-integration  is undefined
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): consolidated config is {"restrictedRepos":["admin",".github","safe-settings"],"repository":{"visibility":"internal","name":"data-analysis-eda","org":"Org-123"}}
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): Syncing Repo data-analysis-eda
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (octokit): GitHub request: GET https://api.github.com/repos/Org-123/api-store - 200
DEBUG (event): Repo Org-123/api-store is not archived, proceed as usual.
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): found a matching repoconfig for this repo {"visibility":"internal","name":"data-analysis-eda","org":"Org-123"}
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
DEBUG (event): suborg config for api-store  is undefined
    id: "1e8b1e00-f5c9-11ef-8072-0096effcb449"
    
    
# Found matching config will result always in `data-analysis-eda` being used
    ```

This Adds support for checking entire PR context instead of only the last commit.
Also fixes a bunch of errors making sure it uses the correct Repo when looping over the list. When setting `RepoConfig` it Assigned the object to the last one in the loop of Repositories. Resulting in wrong comparision.
@stokkie90 stokkie90 marked this pull request as ready for review March 3, 2025 16:22
@Copilot Copilot bot review requested due to automatic review settings March 3, 2025 16:22

Choose a reason for hiding this comment

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

PR Overview

This PR adds support for checking an entire pull request’s changes rather than only the last commit and fixes a bug with repo configuration assignment. Key changes include:

  • Introducing a new environment flag (PR_USE_BASE_SHA) to control base SHA selection in check runs.
  • Fixing the assignment of RepoConfig to avoid unintended mutation by using a new object copy.
  • Updating debug logging in rulesets and settings to improve clarity during processing.

Reviewed Changes

File Description
index.js Adjusts SHA selection logic based on the new PR_USE_BASE_SHA flag for check runs.
lib/env.js Introduces the new environment variable PR_USE_BASE_SHA with a default value of 'false'.
lib/plugins/rulesets.js Updates logging to use the repository owner for clarity when fetching rulesets.
lib/settings.js Adds debug logs at the start of sync routines, fixes the RepoConfig assignment to avoid mutation, and refines the filtering of results in nop mode.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

lib/settings.js:306

  • Creating a new object for repoConfig prevents mutating the original configuration, which avoids side effects in later processing. Please verify that all downstream consumers correctly handle the new object reference.
repoConfig = Object.assign({}, repoConfig, { name: repo.repo, org: repo.owner })

lib/settings.js:867

  • [nitpick] Defaulting to an empty array when res is not an array might discard valid result data; confirm that res is always expected to be an array in nop mode.
const results = Array.isArray(res) ? res.flat(3).filter(r => r) : [];

lib/plugins/rulesets.js:31

  • [nitpick] Using 'this.repo.owner' for logging improves clarity regarding the organization context; ensure that this change is consistent with how the organization is referenced throughout the code.
this.log.debug(`Getting all rulesets for the org ${this.repo.owner}`)
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.

1 participant