Skip to content

Feat/benchmark workflows #1671

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.

Sign up for GitHub

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

Merged
sorrycc merged 6 commits into from
Nov 1, 2024
Merged

Feat/benchmark workflows #1671

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c7aadee
feat: benchmark-workflows
Wu-kung Oct 24, 2024
1bc66cb
Merge branch 'master' into feat/benchmark-workflows
Wu-kung Oct 25, 2024
41c5c52
fix: 修改Github Secrets
Wu-kung Oct 25, 2024
dac431f
Merge branch 'umijs:master' into feat/benchmark-workflows
Wu-kung Oct 30, 2024
34beccd
Merge branch 'umijs:master' into master
Wu-kung Oct 30, 2024
64d649c
Merge branch 'master' into feat/benchmark-workflows
Wu-kung Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions .github/workflows/bench.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
name: Benchmark

on:
issue_comment:
types: [created]

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }}
cancel-in-progress: ${{ github.ref_name != 'master' }}
Comment on lines +3 to +9
Copy link

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议增强工作流触发控制

工作流配置基本合理，但建议考虑以下改进：

  1. 可以添加 permissions 字段明确声明所需权限
  2. 考虑将 cancel-in-progress 的判断改为更精确的表达式，比如：
-  cancel-in-progress: ${{ github.ref_name != 'master' }}
+  cancel-in-progress: ${{ github.ref_name != 'master' && github.ref_name != 'main' }}

Committable suggestion was skipped due to low confidence.


jobs:
trigger:
runs-on: ubuntu-latest
if: github.event.issue.pull_request && startsWith(github.event.comment.body, '!bench')
steps:
- uses: actions/github-script@v7
with:
script: |
const user = context.payload.sender.login
console.log(`Validate user: ${user}`)

let hasTriagePermission = false
try {
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner: context.repo.owner,
repo: context.repo.repo,
username: user,
});
hasTriagePermission = data.user.permissions.triage
} catch (e) {
console.warn(e)
}
Comment on lines +23 to +32
Copy link

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议改进错误处理逻辑

当前的错误处理过于简单，建议：

  1. 在 catch 块中记录更详细的错误信息
  2. 区分不同类型的错误情况（如 API 限制、网络问题等）

建议修改为：

 try {
   const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
     owner: context.repo.owner,
     repo: context.repo.repo,
     username: user,
   });
   hasTriagePermission = data.user.permissions.triage
 } catch (e) {
-  console.warn(e)
+  console.error(`Failed to check permissions for user ${user}: ${e.message}`);
+  if (e.status === 404) {
+    console.log('User is not a collaborator');
+  } else if (e.status === 403) {
+    console.log('API rate limit exceeded or token scope issue');
+  }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner: context.repo.owner,
repo: context.repo.repo,
username: user,
});
hasTriagePermission = data.user.permissions.triage
} catch (e) {
console.warn(e)
}
try {
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner: context.repo.owner,
repo: context.repo.repo,
username: user,
});
hasTriagePermission = data.user.permissions.triage
} catch (e) {
console.error(`Failed to check permissions for user ${user}: ${e.message}`);
if (e.status === 404) {
console.log('User is not a collaborator');
} else if (e.status === 403) {
console.log('API rate limit exceeded or token scope issue');
}
}


if (hasTriagePermission) {
console.log('Allowed')
await github.rest.reactions.createForIssueComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: context.payload.comment.id,
content: '+1',
})
} else {
console.log('Not allowed')
await github.rest.reactions.createForIssueComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: context.payload.comment.id,
content: '-1',
})
throw new Error('not allowed')
}

- uses: actions/github-script@v7
id: get-pr-data
with:
script: |
console.log(`Get PR info: ${context.repo.owner}/${context.repo.repo}#${context.issue.number}`)
const { data: pr } = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number
})
return {
num: context.issue.number,
branchName: pr.head.ref,
repo: pr.head.repo.full_name
}
Comment on lines +57 to +67
Copy link

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议增加数据验证

获取 PR 数据时缺少必要的数据验证，建议：

  1. 验证 PR 是否处于开放状态
  2. 确保返回的数据字段都存在

建议添加验证：

 const { data: pr } = await github.rest.pulls.get({
   owner: context.repo.owner,
   repo: context.repo.repo,
   pull_number: context.issue.number
 })
+if (pr.state !== 'open') {
+  throw new Error('PR must be open to run benchmarks')
+}
+if (!pr.head?.ref || !pr.head?.repo?.full_name) {
+  throw new Error('Invalid PR data structure')
+}
 return {
   num: context.issue.number,
   branchName: pr.head.ref,
   repo: pr.head.repo.full_name
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
console.log(`Get PR info: ${context.repo.owner}/${context.repo.repo}#${context.issue.number}`)
const { data: pr } = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number
})
return {
num: context.issue.number,
branchName: pr.head.ref,
repo: pr.head.repo.full_name
}
console.log(`Get PR info: ${context.repo.owner}/${context.repo.repo}#${context.issue.number}`)
const { data: pr } = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number
})
if (pr.state !== 'open') {
throw new Error('PR must be open to run benchmarks')
}
if (!pr.head?.ref || !pr.head?.repo?.full_name) {
throw new Error('Invalid PR data structure')
}
return {
num: context.issue.number,
branchName: pr.head.ref,
repo: pr.head.repo.full_name
}


- uses: actions/github-script@v7
id: trigger
with:
github-token: ${{ secrets.MAKO_BOT_ACCESS_TOKEN }}
result-encoding: string
script: |
const prData = ${{ steps.get-pr-data.outputs.result }}

await github.rest.actions.createWorkflowDispatch({
owner: context.repo.owner,
repo: 'mako-ecosystem-benchmark',
workflow_id: 'bench_mako_pr.yml',
ref: 'main',
inputs: {
prNumber: '' + prData.num
}
})