Feat: modify workflow updateContributor to be trigger by a repository dispatch event and extract contrbuton of all public repo of frontendmu #248
Conversation
…l public repo of frontendmu
WalkthroughThe pull request introduces modifications to three main files: a GitHub Actions workflow, a JavaScript script for updating contributors, and a new configuration file. The workflow file now includes an additional event trigger, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
packages/frontendmu-data/scripts/update-contributors.js (4)
43-44: Externalize excluded contributors and repositoriesThe
excludedContributorsandexcludedRepositoriesarrays are hardcoded within the script. For better maintainability and flexibility, consider moving them to a configuration file or allowing them to be passed as parameters.
85-85: Remove unnecessary console log statementThe
console.log(contributorsData)statement seems to be for debugging purposes. If it's not needed in production, consider removing it to keep the console output clean.
89-90: Use consistent Git user configurationThe Git user configuration is set to "GitHub Action" and "action@github.com". If this script is run locally or in different environments, the commit attribution may not be accurate.
Consider parameterizing the Git user configuration or setting it to values that reflect the actual user or environment.
// Use environment variables or default to these values const gitUserName = process.env.GIT_USER_NAME || "GitHub Action"; const gitUserEmail = process.env.GIT_USER_EMAIL || "action@github.com"; execSync(`git config user.name "${gitUserName}"`); execSync(`git config user.email "${gitUserEmail}"`);
Line range hint
109-113: Ensure deep equality when comparing contributorsUsing
JSON.stringifyfor comparison may not account for differences in the order of properties or array elements. This could lead to unnecessary commits even when there are no actual changes.Consider sorting the contributors before comparing or using a deep equality check:
function sortContributors(contributors) { return contributors.sort((a, b) => a.username.localeCompare(b.username)); } const existingContributors = sortContributors(getExistingContributors()); const newContributors = sortContributors(updatedContributors); if (JSON.stringify(newContributors) !== JSON.stringify(existingContributors)) { // Proceed to update and commit }Alternatively, use a deep equality library like
lodash.isEqualfor comparison.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/update-contributors.yml (1 hunks)
- packages/frontendmu-data/scripts/update-contributors.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
.github/workflows/update-contributors.yml (1)
7-8: Approve new trigger, but suggest improvementsThe addition of the
repository_dispatchevent trigger is a good improvement that adds flexibility to the workflow. However, there are a few points to consider:
- The current
ifconditiongithub.event.pull_request.merged == truein the job might prevent the workflow from running onrepository_dispatchevents. Consider modifying it to accommodate both triggers:if: (github.event_name == 'pull_request_target' && github.event.pull_request.merged == true) || github.event_name == 'repository_dispatch'
You might want to consider passing additional data with the
repository_dispatchevent. This can be done by adding aclient_payloadto the event and accessing it in the workflow.The PR title mentions modifying the workflow to extract contributions from all public repos, but this change is not reflected in the workflow file. Ensure that the necessary changes to implement this functionality are included in the PR.
To verify the impact of the new trigger, you can run the following command:
This will help identify any additional files that might need to be updated to support the new functionality mentioned in the PR title.
✅ Verification successful
Approve new trigger with verification confirmed
The addition of the
repository_dispatchevent trigger is a beneficial enhancement that allows the workflow to be initiated both manually and programmatically without disrupting the existingpull_request_targettrigger.Upon reviewing the associated
update-contributors.jsscripts:
Compatibility: The scripts are designed to update the
contributors.jsonfile and handle commits independently of how they are triggered. Therefore, they are compatible with the newrepository_dispatchevent.No Additional Changes Needed: Since the scripts do not rely on specific event data and are invoked by the workflow, no modifications to the scripts are necessary to support the new trigger.
Recommendations:
Modify the
ifCondition: Ensure the workflow'sifcondition accommodates both the existing and new triggers. For example:if: (github.event_name == 'pull_request_target' && github.event.pull_request.merged == true) || github.event_name == 'repository_dispatch'Documentation: Update any relevant documentation to reflect the addition of the
repository_dispatchtrigger, ensuring team members are aware of the new capability.Validate Workflow Execution: After merging, perform a test dispatch to confirm that the workflow executes as expected with the new trigger.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any other files that might need updating for the new functionality # Search for files that might be related to updating contributors rg -l 'update.*contributors' --type js --type tsLength of output: 166
Script:
#!/bin/bash # Description: Display the contents of the identified update-contributors.js files for review cat packages/frontendmu-astro/automation/update-contributors.js cat packages/frontendmu-data/scripts/update-contributors.jsLength of output: 6690
packages/frontendmu-data/scripts/update-contributors.js (1)
4-4: Updated the 'owner' variable to 'frontendmu'The 'owner' variable has been correctly updated to "frontendmu", ensuring that the script fetches repositories under the correct GitHub user.
| const allPublicRepositoriesList = await fetch( | ||
| `https://api.github.com/users/${owner}/repos` | ||
| ).then((response) => response.json()); |
There was a problem hiding this comment.
Handle potential pagination when fetching repositories
The GitHub API returns up to 30 repositories per request by default. If the user has more than 30 repositories, some repositories may be missed. Consider handling pagination or increasing the per_page parameter to ensure all repositories are fetched.
You can modify the fetch URL to include ?per_page=100:
- `https://api.github.com/users/${owner}/repos`
+ `https://api.github.com/users/${owner}/repos?per_page=100`Or implement pagination as follows:
let allPublicRepositoriesList = [];
let page = 1;
let per_page = 100;
let hasMore = true;
while (hasMore) {
const response = await fetch(
`https://api.github.com/users/${owner}/repos?page=${page}&per_page=${per_page}`
);
const repos = await response.json();
if (repos.length > 0) {
allPublicRepositoriesList = allPublicRepositoriesList.concat(repos);
page++;
} else {
hasMore = false;
}
}| const allPublicRepositories = allPublicRepositoriesList.map( | ||
| (repo) => repo.name |
There was a problem hiding this comment.
Add error handling for fetching repositories
There's no check to verify if the repository fetch request was successful. If the API call fails or returns an error, the script may throw an exception when trying to process the response.
Consider adding error handling:
const response = await fetch(
`https://api.github.com/users/${owner}/repos`
);
if (!response.ok) {
throw new Error(`Error fetching repositories: ${response.status} ${response.statusText}`);
}
const allPublicRepositoriesList = await response.json();| for (const repo of allPublicRepositories) { | ||
| if (excludedRepositories.includes(repo)) { | ||
| continue; | ||
| } | ||
| const contributorsList = await fetch( | ||
| `https://api.github.com/repos/${owner}/${repo}/contributors` | ||
| ).then((response) => response.json()); | ||
|
|
||
| // const contributorsListFiltered = contributorsList | ||
| // .map((contributor) => { | ||
| // return { | ||
| // username: contributor.login, | ||
| // contributions: contributor.contributions, | ||
| // }; | ||
| // }) | ||
| // .filter((contributor) => { | ||
| // return !excludedContributors.includes(contributor.username); | ||
| // }); | ||
| // contributors.push(...contributorsListFiltered); | ||
| // console.log(`Contributors for ${repo}:`, contributorsListFiltered); | ||
| // } | ||
| // const updatedContributors = [...new Set(contributors)]; | ||
|
|
||
| contributorsList.forEach((contributor) => { | ||
| if (!excludedContributors.includes(contributor.login)) { | ||
| if (contributorsMap[contributor.login]) { | ||
| contributorsMap[contributor.login] += contributor.contributions; | ||
| } else { | ||
| contributorsMap[contributor.login] = contributor.contributions; | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Handle API rate limiting and errors when fetching contributors
Fetching contributors for each repository in a loop may lead to hitting GitHub's API rate limits, especially for unauthenticated requests. Additionally, there's no error handling for these fetch requests, which could cause the script to fail silently.
To handle this:
-
Authenticate API Requests: Use a GitHub personal access token to increase the rate limit.
const GITHUB_TOKEN = process.env.GITHUB_TOKEN; // Ensure this environment variable is set
-
Add Error Handling: Check if the fetch request was successful before processing the response.
const contributorsResponse = await fetch( `https://api.github.com/repos/${owner}/${repo}/contributors`, { headers: { 'Authorization': `token ${GITHUB_TOKEN}` } } ); if (!contributorsResponse.ok) { console.error(`Error fetching contributors for ${repo}: ${contributorsResponse.status} ${contributorsResponse.statusText}`); continue; // Skip to the next repository } const contributorsList = await contributorsResponse.json();
-
Implement Rate Limiting Handling: Detect when the rate limit is reached and wait before retrying.
if (contributorsResponse.status === 403 && contributorsResponse.headers.get('X-RateLimit-Remaining') === '0') { const resetTime = contributorsResponse.headers.get('X-RateLimit-Reset'); const currentTime = Math.floor(Date.now() / 1000); const waitTime = resetTime - currentTime; console.log(`Rate limit exceeded. Waiting for ${waitTime} seconds.`); await new Promise(resolve => setTimeout(resolve, waitTime * 1000)); // Retry the request after waiting }
| contributorsList.forEach((contributor) => { | ||
| if (!excludedContributors.includes(contributor.login)) { | ||
| if (contributorsMap[contributor.login]) { | ||
| contributorsMap[contributor.login] += contributor.contributions; | ||
| } else { | ||
| contributorsMap[contributor.login] = contributor.contributions; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor contributor aggregation logic using a Map
While the current logic works, using a Map can make the code more efficient and cleaner when accumulating contributions.
Here's how you can implement it:
const contributorsMap = new Map();
contributorsList.forEach((contributor) => {
if (!excludedContributors.includes(contributor.login)) {
const currentContributions = contributorsMap.get(contributor.login) || 0;
contributorsMap.set(contributor.login, currentContributions + contributor.contributions);
}
});Then, when creating updatedContributors:
const updatedContributors = Array.from(contributorsMap, ([username, contributions]) => ({
username,
contributions
}));
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/frontendmu-data/scripts/update-contributors.js (1)
12-15: Validate the config structure.While loading the config is a good practice, there's no validation of its structure or required fields. Consider adding checks to ensure
includedRepositoriesandexcludedContributorsexist and are arrays.Example:
if (!Array.isArray(config.includedRepositories) || !Array.isArray(config.excludedContributors)) { throw new Error('Invalid config structure: includedRepositories and excludedContributors must be arrays'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/frontendmu-data/scripts/update-contributors.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/frontendmu-data/scripts/update-contributors.js (4)
4-4: LGTM: Owner variable updated.The change of the
ownervariable to "frontendmu" aligns with the PR objectives.
16-21: Previous comments about pagination and error handling are still applicable.The changes align with the PR objectives, but the concerns raised in previous comments about pagination and error handling when fetching repositories are still valid and should be addressed.
50-58: LGTM: Improved logging and file writing.The addition of console logging and the use of formatted JSON stringification for file writing are good improvements. They enhance debugging capabilities and improve the readability of the output file.
8-9: Add error handling for config file fetching.While the
configSourceURL construction looks correct, there's no error handling if the config file doesn't exist or can't be fetched. Consider adding a try-catch block in theloadConfigfunction to handle potential errors.
| const contributorsMap = {}; | ||
|
|
||
| for (const repo of allPublicRepositories) { | ||
| if (!includedRepositories.includes(repo)) { | ||
| continue; | ||
| } | ||
| const contributorsList = await fetch( | ||
| `https://api.github.com/repos/${owner}/${repo}/contributors` | ||
| ).then((response) => response.json()); | ||
|
|
||
| contributorsList.forEach((contributor) => { | ||
| if (!excludedContributors.includes(contributor.login)) { | ||
| if (contributorsMap[contributor.login]) { | ||
| contributorsMap[contributor.login] += contributor.contributions; | ||
| } else { | ||
| contributorsMap[contributor.login] = contributor.contributions; | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| const updatedContributors = Object.entries(contributorsMap).map(([username, contributions]) => ({ | ||
| username, | ||
| contributions | ||
| })); |
There was a problem hiding this comment.
Add error handling when fetching contributors.
While the logic for aggregating contributions has been improved, there's still no error handling when fetching contributors for each repository. Consider adding try-catch blocks or checking the response status to handle potential API errors or rate limiting issues.
Example:
const contributorsResponse = await fetch(`https://api.github.com/repos/${owner}/${repo}/contributors`);
if (!contributorsResponse.ok) {
console.error(`Failed to fetch contributors for ${repo}: ${contributorsResponse.status}`);
continue;
}
const contributorsList = await contributorsResponse.json();| async function loadConfig() { | ||
| return await fetch(configSource).then((response) => response.json()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the loadConfig function with error handling and timeout.
While the loadConfig function is concise, it lacks error handling and a timeout setting. Consider adding these to improve robustness:
async function loadConfig() {
try {
const response = await fetch(configSource, { timeout: 5000 });
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`);
}
return await response.json();
} catch (error) {
console.error('Failed to load config:', error);
throw error;
}
}
Summary by CodeRabbit
New Features
Bug Fixes
Documentation