Skip to content

fix: use merge_commit_sha instead of head.sha for auto-tag#786

Open
hudeng-go wants to merge 1 commit into
linuxdeepin:masterfrom
hudeng-go:fix-autotag
Open

fix: use merge_commit_sha instead of head.sha for auto-tag#786
hudeng-go wants to merge 1 commit into
linuxdeepin:masterfrom
hudeng-go:fix-autotag

Conversation

@hudeng-go
Copy link
Copy Markdown
Contributor

解决创建的tag无法找到对用的分支的问题

Log:

    解决创建的tag无法找到对用的分支的问题

Log:
@deepin-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hudeng-go

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@deepin-ci-robot
Copy link
Copy Markdown
Contributor

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的 GitHub Actions 工作流代码的 diff。本次修改将获取 Pull Request (PR) 提交 SHA 的方式从 head.sha 改为了 merge_commit_sha

这是一个在 CI/CD 流程中非常关键的改动,涉及到 Git 底层逻辑和 GitHub Actions 的运行机制。以下是我的详细审查意见:

1. 语法逻辑

  • 逻辑变更分析
    • context.payload.pull_request.head.sha:指向的是 PR 源分支的最新 commit(即用户最后一次 push 的 commit)。
    • context.payload.pull_request.merge_commit_sha:指向的是 GitHub 在后台将 PR 源分支合并到目标分支时生成的合并 commit
  • 潜在逻辑缺陷:如果该工作流的触发时机是在 PR 合并之前(例如 pull_request 事件的 opened, synchronize, reopened 等触发类型),此时 PR 尚未合并,merge_commit_sha 的值可能为 null。如果将 null 传给 github.rest.git.createTag,将会导致 API 报错(通常是 422 Unprocessable Entity)。
  • 改进建议:必须确保此段代码仅在 PR 已经合并后执行(例如触发类型为 pull_requestclosed 事件,且 context.payload.pull_request.merged === true)。如果逻辑不能保证这一点,需要增加空值检查。

2. 代码质量

  • 健壮性不足:直接访问深层对象属性而不做校验,在 CI 环境中容易产生难以调试的静默失败或晦涩报错。
  • 改进建议:增加防御性编程,对 merge_commit_sha 进行存在性校验,并提供清晰的错误提示。

3. 代码性能

  • 性能评估:本次改动仅涉及变量的引用替换,从 head.sha 换为 merge_commit_sha,两者都是直接从 GitHub Actions 的 context.payload 内存对象中读取,不涉及额外的网络 I/O 或 API 调用,因此对性能没有负面影响。

4. 代码安全

  • 安全风险(供应链/标签篡改)
    • 使用 head.sha 时,标签指向的是用户 fork 仓库中的明确 commit。而 merge_commit_sha 是 GitHub 自动生成的。
    • 虽然在 GitHub 的标准合并策略下,merge_commit_sha 是安全的,但如果目标仓库开启了 "Allow auto-merge" 或在合并时使用了 squash merge / rebase mergemerge_commit_sha 的行为可能会与预期不符(例如 squash merge 时,merge_commit_sha 指向的是 squash 后的 commit,这通常是符合预期的,但需要确认这是否符合你的打标签策略)。
    • 此外,合并提交本身包含了两个父提交,确保打标签的对象确实是最终合入主分支的代码状态。

💡 综合改进建议及代码示例

为了确保代码的健壮性和安全性,建议在获取 merge_commit_sha 后增加校验逻辑,并确保只在 PR 成功合并时执行打标签操作。

以下是改进后的代码片段:

// 确保只在 PR 成功合并时执行后续逻辑
if (!context.payload.pull_request.merged) {
  core.info("PR was not merged, skipping tag creation.")
  return
}

const mergeCommitSha = context.payload.pull_request.merge_commit_sha

// 增加防御性校验,防止 merge_commit_sha 为空导致 API 报错
if (!mergeCommitSha) {
  core.setFailed("Error: merge_commit_sha is null. This can happen if the PR is not fully merged or in certain merge strategies.")
  return
}

let data
try {
  const tagResult = await github.rest.git.createTag({
    repo: context.repo.repo,
    owner: context.repo.owner,
    tag: /* your tag variable */,
    message: /* your message variable */,
    object: mergeCommitSha, // 使用校验过的 mergeCommitSha
    type: "commit"
  })
  // ... 后续逻辑
} catch (error) {
  core.setFailed(`Failed to create tag: ${error.message}`)
}

总结:将 head.sha 改为 merge_commit_sha 在自动打标签的场景下通常是更正确的做法(因为打标签通常针对的是合入主分支的那个合并节点,而不是 PR 的开发节点),但必须辅以严格的合并状态检查和空值校验,以保证工作流的稳定运行。

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.

2 participants