Adding action.yaml#1030
Conversation
Signed-off-by: Iryna Boiko <iboiko@habana.ai>
There was a problem hiding this comment.
Pull request overview
This PR adds a new composite GitHub Action (get-pr-details) that retrieves pull request information for both pull_request_target and merge_group workflow events. The action extracts PR metadata including number, title, labels, base reference, and a flag indicating whether to skip Gaudi tests based on the presence of the skip-gaudi-tests label.
Changes:
- Added a new composite action that handles two event types:
pull_request_targetandmerge_group - Implements PR number extraction from merge group head refs using regex pattern matching
- Extracts and outputs PR details including labels and skip-gaudi-tests flag
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fs.appendFileSync(outputFile, `pr_number=${pullRequest.number}\n`); | ||
| fs.appendFileSync(outputFile, `pr_title=${pullRequest.title}\n`); |
There was a problem hiding this comment.
The PR title in the output could contain newline characters or other special characters that may cause issues when written to GITHUB_OUTPUT. If pullRequest.title contains newlines, the output format will be broken. Consider sanitizing the title by replacing newlines with spaces or using a multiline string format with delimiters (e.g., EOF markers) similar to how it's done in the pre-merge.yaml workflow.
| fs.appendFileSync(outputFile, `pr_number=${pullRequest.number}\n`); | |
| fs.appendFileSync(outputFile, `pr_title=${pullRequest.title}\n`); | |
| const sanitizedTitle = String(pullRequest.title || '').replace(/[\r\n]+/g, ' '); | |
| fs.appendFileSync(outputFile, `pr_number=${pullRequest.number}\n`); | |
| fs.appendFileSync(outputFile, `pr_title=${sanitizedTitle}\n`); |
| const fs = require('fs'); | ||
| const outputFile = process.env.GITHUB_OUTPUT; | ||
| fs.appendFileSync(outputFile, `pr_number=${pullRequest.number}\n`); | ||
| fs.appendFileSync(outputFile, `pr_title=${pullRequest.title}\n`); |
There was a problem hiding this comment.
The PR title in the output could contain newline characters or other special characters that may cause issues when written to GITHUB_OUTPUT. If pullRequest.title contains newlines, the output format will be broken. Consider sanitizing the title by replacing newlines with spaces or using a multiline string format with delimiters (e.g., EOF markers) similar to how it's done in the pre-merge.yaml workflow.
| const outputFile = process.env.GITHUB_OUTPUT; | ||
| fs.appendFileSync(outputFile, `pr_number=${pullRequest.number}\n`); | ||
| fs.appendFileSync(outputFile, `pr_title=${pullRequest.title}\n`); | ||
| fs.appendFileSync(outputFile, `pr_labels=${prLabels}\n`); |
There was a problem hiding this comment.
The PR labels in the output could contain newline characters or other special characters that may cause issues when written to GITHUB_OUTPUT. Label names are user-controlled and could potentially contain newlines or special characters. Consider sanitizing the labels string by replacing newlines with spaces or encoding it properly before writing to GITHUB_OUTPUT.
| fs.appendFileSync(outputFile, `skip_gaudi_tests=${skipGaudiTests}\n`); | ||
|
|
||
| } catch (error) { | ||
| console.error('❌ Error fetching PR details:', error.message); |
There was a problem hiding this comment.
The error handling only logs error.message but doesn't log the full error or stack trace. For debugging GitHub Action failures, having the full error context including the stack trace would be valuable. Consider logging the full error object or at least including error.stack in addition to error.message.
| console.error('❌ Error fetching PR details:', error.message); | |
| console.error('❌ Error fetching PR details:', error.message); | |
| if (error && error.stack) { | |
| console.error(error.stack); | |
| } else { | |
| console.error(error); | |
| } |
| return; | ||
| } | ||
|
|
||
| const prNumber = parseInt(prMatch[1], 10); |
There was a problem hiding this comment.
The parsed PR number should be validated after parseInt. While the regex ensures that prMatch[1] contains digits, parseInt could still return NaN in edge cases or if the number is too large. Consider checking if the result is a valid positive integer before proceeding with the API call.
| const prNumber = parseInt(prMatch[1], 10); | |
| const prNumber = parseInt(prMatch[1], 10); | |
| if (!Number.isFinite(prNumber) || !Number.isInteger(prNumber) || prNumber <= 0) { | |
| console.log('❌ Extracted PR number is invalid. Raw value: %s, Parsed value: %s', prMatch[1], prNumber); | |
| core.setFailed('Extracted PR number from merge group head ref is not a valid positive integer'); | |
| return; | |
| } | |
| try { | ||
| const { data: pullRequest } = await github.rest.pulls.get({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: prNumber |
There was a problem hiding this comment.
The PR_NUMBER input should be validated before use. Currently, when eventName is 'pull_request_target', the code directly uses process.env.PR_NUMBER without checking if it exists or is a valid number. If PR_NUMBER is missing or invalid, the github.rest.pulls.get call will fail with an unclear error. Add validation similar to what is done for MERGE_GROUP_HEAD_REF in the merge_group event handler to provide clearer error messages.
| try { | |
| const { data: pullRequest } = await github.rest.pulls.get({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: prNumber | |
| if (!prNumber) { | |
| const errorMessage = "PR_NUMBER environment variable is required for 'pull_request_target' events but was not provided."; | |
| console.error(errorMessage); | |
| core.setFailed(errorMessage); | |
| return; | |
| } | |
| const parsedPrNumber = parseInt(prNumber, 10); | |
| if (Number.isNaN(parsedPrNumber) || parsedPrNumber <= 0) { | |
| const errorMessage = `Invalid PR_NUMBER environment variable value: '${prNumber}'. Expected a positive integer.`; | |
| console.error(errorMessage); | |
| core.setFailed(errorMessage); | |
| return; | |
| } | |
| try { | |
| const { data: pullRequest } = await github.rest.pulls.get({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: parsedPrNumber |
| const outputFile = process.env.GITHUB_OUTPUT; | ||
| fs.appendFileSync(outputFile, `pr_number=${pullRequest.number}\n`); | ||
| fs.appendFileSync(outputFile, `pr_title=${pullRequest.title}\n`); | ||
| fs.appendFileSync(outputFile, `pr_labels=${prLabels}\n`); |
There was a problem hiding this comment.
The PR labels in the output could contain newline characters or other special characters that may cause issues when written to GITHUB_OUTPUT. Label names are user-controlled and could potentially contain newlines or special characters. Consider sanitizing the labels string by replacing newlines with spaces or encoding it properly before writing to GITHUB_OUTPUT.
| fs.appendFileSync(outputFile, `skip_gaudi_tests=${skipGaudiTests}\n`); | ||
|
|
||
| } catch (error) { | ||
| console.error('❌ Error fetching PR details:', error.message); |
There was a problem hiding this comment.
The error handling only logs error.message but doesn't log the full error or stack trace. For debugging GitHub Action failures, having the full error context including the stack trace would be valuable. Consider logging the full error object or at least including error.stack in addition to error.message.
| const prLabels = pullRequest.labels.map(l => l.name).join(', ') || 'None'; | ||
|
|
||
| console.log('\n✅ Pull Request Information:'); | ||
| console.log('- PR Number: #%s', pullRequest.number); | ||
| console.log('- PR Title: %s', pullRequest.title); | ||
| console.log('- PR Author: %s', pullRequest.user.login); | ||
| console.log('- PR State: %s', pullRequest.state); | ||
| console.log('- PR Labels: %s', prLabels); | ||
|
|
||
| const skipGaudiTests = pullRequest.labels.some(l => l.name === 'skip-gaudi-tests'); | ||
|
|
||
| const fs = require('fs'); | ||
| const outputFile = process.env.GITHUB_OUTPUT; | ||
| fs.appendFileSync(outputFile, `pr_number=${pullRequest.number}\n`); | ||
| fs.appendFileSync(outputFile, `pr_title=${pullRequest.title}\n`); | ||
| fs.appendFileSync(outputFile, `pr_labels=${prLabels}\n`); | ||
| fs.appendFileSync(outputFile, `pr_base_ref=${pullRequest.base.ref}\n`); | ||
| fs.appendFileSync(outputFile, `skip_gaudi_tests=${skipGaudiTests}\n`); | ||
|
|
There was a problem hiding this comment.
There is significant code duplication between the merge_group and pull_request_target event handlers. Lines 85-91 (in merge_group) and lines 118-124 (in pull_request_target) are identical, as are lines 74 (computing prLabels), lines 83 (computing skipGaudiTests), and lines 107-114 (console logging). This duplicated logic makes the code harder to maintain - any bug fix or enhancement would need to be applied in two places. Consider extracting the common logic into a helper function that both event handlers can call.
| const prLabels = pullRequest.labels.map(l => l.name).join(', ') || 'None'; | |
| console.log('\n✅ Pull Request Information:'); | |
| console.log('- PR Number: #%s', pullRequest.number); | |
| console.log('- PR Title: %s', pullRequest.title); | |
| console.log('- PR Author: %s', pullRequest.user.login); | |
| console.log('- PR State: %s', pullRequest.state); | |
| console.log('- PR Labels: %s', prLabels); | |
| const skipGaudiTests = pullRequest.labels.some(l => l.name === 'skip-gaudi-tests'); | |
| const fs = require('fs'); | |
| const outputFile = process.env.GITHUB_OUTPUT; | |
| fs.appendFileSync(outputFile, `pr_number=${pullRequest.number}\n`); | |
| fs.appendFileSync(outputFile, `pr_title=${pullRequest.title}\n`); | |
| fs.appendFileSync(outputFile, `pr_labels=${prLabels}\n`); | |
| fs.appendFileSync(outputFile, `pr_base_ref=${pullRequest.base.ref}\n`); | |
| fs.appendFileSync(outputFile, `skip_gaudi_tests=${skipGaudiTests}\n`); | |
| const handlePullRequestDetails = (pullRequest) => { | |
| const prLabels = pullRequest.labels.map(l => l.name).join(', ') || 'None'; | |
| console.log('\n✅ Pull Request Information:'); | |
| console.log('- PR Number: #%s', pullRequest.number); | |
| console.log('- PR Title: %s', pullRequest.title); | |
| console.log('- PR Author: %s', pullRequest.user.login); | |
| console.log('- PR State: %s', pullRequest.state); | |
| console.log('- PR Labels: %s', prLabels); | |
| const skipGaudiTests = pullRequest.labels.some(l => l.name === 'skip-gaudi-tests'); | |
| const fs = require('fs'); | |
| const outputFile = process.env.GITHUB_OUTPUT; | |
| fs.appendFileSync(outputFile, `pr_number=${pullRequest.number}\n`); | |
| fs.appendFileSync(outputFile, `pr_title=${pullRequest.title}\n`); | |
| fs.appendFileSync(outputFile, `pr_labels=${prLabels}\n`); | |
| fs.appendFileSync(outputFile, `pr_base_ref=${pullRequest.base.ref}\n`); | |
| fs.appendFileSync(outputFile, `skip_gaudi_tests=${skipGaudiTests}\n`); | |
| }; | |
| handlePullRequestDetails(pullRequest); | |
kamil-kaczor
left a comment
There was a problem hiding this comment.
needed to restart CI on 0.15.1
Signed-off-by: Iryna Boiko <iboiko@habana.ai>
Signed-off-by: Iryna Boiko <iboiko@habana.ai>
83caba6
into
vllm-project:releases/v0.15.1
No description provided.