*: use default commit author in bazel update workflow#67000
Conversation
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
📝 WalkthroughWalkthroughThis PR introduces a GitHub Actions workflow to automatically update Bazel configuration files on pull requests and removes redundant download URLs from the WORKSPACE file, consolidating dependencies to essential mirrors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67000 +/- ##
================================================
+ Coverage 77.7062% 77.7632% +0.0570%
================================================
Files 2012 1934 -78
Lines 550606 542872 -7734
================================================
- Hits 427855 422155 -5700
+ Misses 121035 120708 -327
+ Partials 1716 9 -1707
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/update-bazel-files.yml (1)
51-55: Darwin branch is dead code onubuntu-latest.The workflow runs on
ubuntu-latest(line 30), souname -swill always returnLinux. The Darwin branch (lines 51-55) is unreachable and can be removed to reduce maintenance burden.Simplified version
run: | #!/bin/bash unset CI - system_name=$(uname -s) - if [ "$system_name" == "Darwin" ]; then - sed -i '' '/bazel-cache/d' DEPS.bzl - sed -i '' '/ats.apps.svc/d' DEPS.bzl - sed -i '' '/bazel-cache/d' WORKSPACE - sed -i '' '/ats.apps.svc/d' WORKSPACE - elif [ "$system_name" == "Linux" ]; then - sed -i '/bazel-cache/d' DEPS.bzl - sed -i '/ats.apps.svc/d' DEPS.bzl - sed -i '/bazel-cache/d' WORKSPACE - sed -i '/ats.apps.svc/d' WORKSPACE - fi make bazel_prepare + sed -i '/bazel-cache/d' DEPS.bzl WORKSPACE + sed -i '/ats.apps.svc/d' DEPS.bzl WORKSPACE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/update-bazel-files.yml around lines 51 - 55, Remove the unreachable Darwin branch by deleting the conditional block that checks if [ "$system_name" == "Darwin" ] and its four sed -i '' commands (the lines containing sed -i '' '/bazel-cache/d' DEPS.bzl, sed -i '' '/ats.apps.svc/d' DEPS.bzl, sed -i '' '/bazel-cache/d' WORKSPACE, and sed -i '' '/ats.apps.svc/d' WORKSPACE); leave the existing Linux branch (sed -i ... without the '' macOS argument) intact so the workflow running on ubuntu-latest only executes the applicable sed commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/update-bazel-files.yml:
- Around line 6-13: The workflow currently uses pull_request_target and then
checks out and runs PR head code (pull_request_target), which exposes repo
secrets; re-enable the commented conditional guard by adding the if condition
that checks github.event.pull_request.head.repo.full_name == github.repository
|| github.event.pull_request.maintainer_can_modify so the job only runs for
same-repo PRs or maintainer-modifiable PRs, or alternatively switch the trigger
to pull_request and avoid checking out PR head with repository secrets; ensure
secrets referenced (secrets.BAZELRC, secrets.GCP_SA_KEY,
secrets.BAZEL_PREPARE_PUSH_TOKEN) are only used when the guard passes and the
checkout action does not pull untrusted PR code.
- Around line 47-62: The sed deletions are running before make bazel_prepare and
get overwritten; reorder the steps so the script runs make bazel_prepare first
(invoke make bazel_prepare), then detect the OS (system_name=$(uname -s)) and
apply the OS-specific sed -i commands to DEPS.bzl and WORKSPACE to remove
'bazel-cache' and 'ats.apps.svc'; keep the existing Darwin vs Linux sed flags
and unset CI logic but move the four sed invocations for DEPS.bzl and WORKSPACE
to execute after the make bazel_prepare call.
In `@pkg/util/dummy_bazel_trigger.go`:
- Around line 1-4: Add the standard TiDB Apache 2.0 license header to the top of
pkg/util/dummy_bazel_trigger.go (above package util); copy the exact header text
from an existing file in the same pkg/util/ directory to ensure consistency and
formatting, keeping the rest of the file intact (function DummyBazelTrigger
should remain unchanged).
---
Nitpick comments:
In @.github/workflows/update-bazel-files.yml:
- Around line 51-55: Remove the unreachable Darwin branch by deleting the
conditional block that checks if [ "$system_name" == "Darwin" ] and its four sed
-i '' commands (the lines containing sed -i '' '/bazel-cache/d' DEPS.bzl, sed -i
'' '/ats.apps.svc/d' DEPS.bzl, sed -i '' '/bazel-cache/d' WORKSPACE, and sed -i
'' '/ats.apps.svc/d' WORKSPACE); leave the existing Linux branch (sed -i ...
without the '' macOS argument) intact so the workflow running on ubuntu-latest
only executes the applicable sed commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dbbc80e0-65df-4ef8-ad3c-ceebce4df476
📒 Files selected for processing (5)
.github/workflows/update-bazel-files.ymlWORKSPACEpkg/dxf/importinto/conflict_resolution_test.gopkg/util/BUILD.bazelpkg/util/dummy_bazel_trigger.go
💤 Files with no reviewable changes (1)
- WORKSPACE
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/update-bazel-files.yml (1)
40-49:⚠️ Potential issue | 🟠 Majorsed modifications are still overwritten by
make bazel_prepare.The sed commands (lines 45-48) that remove
bazel-cacheandats.apps.svcURLs run beforemake bazel_prepare(line 49). Sincemake bazel_prepareregeneratesDEPS.bzlfromgo.mod, these sed modifications will be overwritten.Move
make bazel_prepareto run before the sed commands:Proposed fix
- name: Run Bazel Prepare shell: bash run: | #!/bin/bash unset CI + make bazel_prepare sed -i '/bazel-cache/d' DEPS.bzl sed -i '/ats.apps.svc/d' DEPS.bzl sed -i '/bazel-cache/d' WORKSPACE sed -i '/ats.apps.svc/d' WORKSPACE - make bazel_prepare🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/update-bazel-files.yml around lines 40 - 49, The sed removals for 'bazel-cache' and 'ats.apps.svc' are executed before make bazel_prepare so their edits get clobbered; reorder the steps so make bazel_prepare runs first (invoke make bazel_prepare before running the sed commands) and then run the sed commands that modify DEPS.bzl and WORKSPACE to remove those URLs (keep references to the existing sed invocations and the make bazel_prepare target so you update the same job commands).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/update-bazel-files.yml:
- Around line 40-49: The sed removals for 'bazel-cache' and 'ats.apps.svc' are
executed before make bazel_prepare so their edits get clobbered; reorder the
steps so make bazel_prepare runs first (invoke make bazel_prepare before running
the sed commands) and then run the sed commands that modify DEPS.bzl and
WORKSPACE to remove those URLs (keep references to the existing sed invocations
and the make bazel_prepare target so you update the same job commands).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 51966bc6-e569-4e51-910f-1ef51fa2e8ca
📒 Files selected for processing (2)
.github/workflows/update-bazel-files.ymlDEPS.bzl
|
/hold Let us merge this pr on monday. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, lance6716 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/unhold |
|
@hawkingrei: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
What problem does this PR solve?
Issue Number: ref #66999
Problem Summary:
The Bazel auto-update workflow redundantly hard-codes the commit author fields even though
stefanzweifel/git-auto-commit-actionalready provides a default commit identity.What changed and how does it work?
This PR removes the explicit
commit_author,commit_user_name, andcommit_user_emailsettings from.github/workflows/update-bazel-files.ymland lets the auto-commit action use its default author configuration.Check List
Tests
Manual test steps:
.github/workflows/update-bazel-files.yml.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit