Handle multiple config changes in a PR or Push event and process them as a batch #888
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the synchronization logic for repository and sub-organization settings by consolidating duplicate code into batch processing functions. The main goal is to handle multiple configuration changes from PR or push events more efficiently by processing them as a batch rather than individually.
Key changes:
- Introduced
syncSelectedReposmethod to handle batching of repository and sub-organization synchronization - Consolidated duplicate sync logic by creating a unified
syncSelectedSettingsfunction - Updated pull request file change detection to use GitHub's pull request files API instead of commit comparison
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/settings.js | Added new syncSelectedRepos batch processing method and extracted checkAndProcessRepo helper |
| index.js | Refactored sync functions to use batch processing and updated PR file detection logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| params = Object.assign(context.repo(), { pull_number: pull_request.number }) | ||
|
|
||
| const changes = await context.octokit.pulls.listFiles(params) |
There was a problem hiding this comment.
[nitpick] The variable params is being reused for different purposes. Consider using a more descriptive variable name like pullRequestParams to improve code clarity and avoid confusion with the previous check run parameters.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| })) | ||
| } | ||
|
|
||
| const subOrgChanges = getChangedSubOrgConfigName(files) |
There was a problem hiding this comment.
Inconsistent property naming: getChangedSubOrgConfigName returns objects with name property while getAllChangedSubOrgConfigs returns objects with repo property. This inconsistency will cause issues when both are processed by the same batch method.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| // guarding against null value from upstream libary that is | ||
| // causing a 404 and the check to stall | ||
| // from issue: https://github.com/github/safe-settings/issues/185#issuecomment-1075240374 | ||
| if (check_suite.before === '0000000000000000000000000000000000000000') { |
There was a problem hiding this comment.
this seems like it would cause a regression of #185
why remove it?
There was a problem hiding this comment.
Because we have changed the API call to a different one which does not require the before and after commits.
2 participants
This pull request refactors the way repository and sub-organization settings are synchronized, improving clarity and maintainability by consolidating logic and introducing a new method for handling multiple changes at once. The changes also update how file changes are detected in pull requests, aligning with GitHub's API best practices.