Repository navigation
br: fix fake-gcs test - #67645
br: fix fake-gcs test#67645
Conversation
ti-chi-bot
Bot
added
do-not-merge/invalid-title
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
@JoyC-dev I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/S
|
Hi @JoyC-dev. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughUpdated GCS test scripts to include Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
br/tests/br_gcs/run.sh (1)
92-92: Quote"$PD_ADDR"consistently in BR invocations.These three commands still use unquoted
--pd $PD_ADDR; quoting avoids shell splitting/globbing pitfalls.Suggested patch
-run_br --pd $PD_ADDR backup full -s "gcs://$BUCKET/$DB?endpoint=http://$GCS_HOST:$GCS_PORT" +run_br --pd "$PD_ADDR" backup full -s "gcs://$BUCKET/$DB?endpoint=http://$GCS_HOST:$GCS_PORT" -run_br restore full -s "gcs://$BUCKET/$DB?" --pd $PD_ADDR --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false +run_br restore full -s "gcs://$BUCKET/$DB?" --pd "$PD_ADDR" --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false -run_br restore full -s "gcs://$BUCKET/${DB}_old" --pd $PD_ADDR --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false +run_br restore full -s "gcs://$BUCKET/${DB}_old" --pd "$PD_ADDR" --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=falseAlso applies to: 110-110, 138-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/tests/br_gcs/run.sh` at line 92, The BR invocations call run_br with an unquoted PD address (e.g., the command using run_br --pd $PD_ADDR backup full ...); update each invocation to quote the variable to prevent shell word-splitting/globbing by changing --pd $PD_ADDR to --pd "$PD_ADDR" in the run_br calls found around the lines with run_br --pd $PD_ADDR (also apply same fix at the other occurrences mentioned).br/tests/docker_compatible_gcs/_run.sh (1)
45-45: Quote"$PD_ADDR"in the BR command.
--pd $PD_ADDRis currently unquoted; quote it to avoid shell word splitting/globbing edge cases. This is inconsistent with the quoting of other variables like"$BUCKET","$GCS_HOST", and"$GCS_PORT"on the same line.Suggested patch
- bin/br restore db --db test -s "gcs://$BUCKET/bk${TAG}" --pd $PD_ADDR --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false + bin/br restore db --db test -s "gcs://$BUCKET/bk${TAG}" --pd "$PD_ADDR" --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/tests/docker_compatible_gcs/_run.sh` at line 45, The BR restore invocation uses an unquoted --pd $PD_ADDR which risks shell word-splitting/globbing; update the command that calls bin/br restore db (the line with --pd $PD_ADDR) to quote the PD_ADDR variable as --pd "$PD_ADDR" so it matches the quoting style used for "$BUCKET", "$GCS_HOST", and "$GCS_PORT".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@br/tests/br_gcs/run.sh`:
- Line 92: The BR invocations call run_br with an unquoted PD address (e.g., the
command using run_br --pd $PD_ADDR backup full ...); update each invocation to
quote the variable to prevent shell word-splitting/globbing by changing --pd
$PD_ADDR to --pd "$PD_ADDR" in the run_br calls found around the lines with
run_br --pd $PD_ADDR (also apply same fix at the other occurrences mentioned).
In `@br/tests/docker_compatible_gcs/_run.sh`:
- Line 45: The BR restore invocation uses an unquoted --pd $PD_ADDR which risks
shell word-splitting/globbing; update the command that calls bin/br restore db
(the line with --pd $PD_ADDR) to quote the PD_ADDR variable as --pd "$PD_ADDR"
so it matches the quoting style used for "$BUCKET", "$GCS_HOST", and
"$GCS_PORT".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dbc1ebb0-1645-4778-ac64-3ee03cff7bca
📒 Files selected for processing (3)
br/tests/br_gcs/run.shbr/tests/docker_compatible_gcs/_run.shbr/tests/docker_compatible_gcs/prepare.sh
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67645 +/- ##
================================================
+ Coverage 77.5935% 78.6548% +1.0612%
================================================
Files 1981 1983 +2
Lines 547950 548035 +85
================================================
+ Hits 425174 431056 +5882
+ Misses 121966 115955 -6011
- Partials 810 1024 +214
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
@JoyC-dev: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
ti-chi-bot
Bot
added
size/M
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
br/tests/br_gcs/run.sh (1)
138-139: Log text is misleading for the executed restore binary.The message says
v4.0.8 version restore start..., but this block runsrun_br restore(current BR). Consider renaming the log to reflect “restore legacy backup with current BR” for clearer diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/tests/br_gcs/run.sh` around lines 138 - 139, The log message "v4.0.8 version restore start..." is misleading because the script actually invokes the current BR via the run_br restore command; update the echo to clearly state that the current BR is restoring a legacy v4.0.8 backup (e.g., change the message to something like "restoring v4.0.8 backup using current BR...") so diagnostics reflect that run_br is used; locate the echo string near the run_br restore invocation and adjust its text accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@br/tests/br_gcs/run.sh`:
- Line 111: The restore command uses a storage URL with a dangling query marker
'-s "gcs://$BUCKET/$DB?"' which adds an empty query component; update the
invocation in run_br (the line calling run_br restore full) to pass the storage
URL without the trailing '?' (use 'gcs://$BUCKET/$DB') so the GCS URL is clean
and avoids parser edge cases across BR versions.
---
Nitpick comments:
In `@br/tests/br_gcs/run.sh`:
- Around line 138-139: The log message "v4.0.8 version restore start..." is
misleading because the script actually invokes the current BR via the run_br
restore command; update the echo to clearly state that the current BR is
restoring a legacy v4.0.8 backup (e.g., change the message to something like
"restoring v4.0.8 backup using current BR...") so diagnostics reflect that
run_br is used; locate the echo string near the run_br restore invocation and
adjust its text accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1e21c7fb-2221-440d-9860-492b59d6e097
📒 Files selected for processing (1)
br/tests/br_gcs/run.sh
| # new version restore full | ||
| echo "restore start..." | ||
| run_br restore full -s "gcs://$BUCKET/$DB?" --pd $PD_ADDR --gcs.endpoint="http://$GCS_HOST:$GCS_PORT/storage/v1/" --check-requirements=false | ||
| run_br restore full -s "gcs://$BUCKET/$DB?" --pd $PD_ADDR --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false |
There was a problem hiding this comment.
Remove the dangling ? in the restore storage URL.
-s "gcs://$BUCKET/$DB?" carries an empty query component. It’s safer to pass gcs://$BUCKET/$DB directly to avoid parser edge behavior across BR versions.
💡 Proposed fix
-run_br restore full -s "gcs://$BUCKET/$DB?" --pd $PD_ADDR --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false
+run_br restore full -s "gcs://$BUCKET/$DB" --pd $PD_ADDR --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false📝 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.
| run_br restore full -s "gcs://$BUCKET/$DB?" --pd $PD_ADDR --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false | |
| run_br restore full -s "gcs://$BUCKET/$DB" --pd $PD_ADDR --gcs.endpoint="http://$GCS_HOST:$GCS_PORT" --check-requirements=false |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 111-111: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@br/tests/br_gcs/run.sh` at line 111, The restore command uses a storage URL
with a dangling query marker '-s "gcs://$BUCKET/$DB?"' which adds an empty query
component; update the invocation in run_br (the line calling run_br restore
full) to pass the storage URL without the trailing '?' (use 'gcs://$BUCKET/$DB')
so the GCS URL is clean and avoids parser edge cases across BR versions.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, Leavrth 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 |
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
Port upstream pingcap#67645 (997e75c). Update service-account fields and base endpoints; retain legacy BR v4.0.8 checks behind TEST_LEGACY_BR_GCS=true, matching upstream.
|
/cherry-pick feature/release-8.5-read_only |
|
@expxiaoli: new pull request created to branch DetailsIn response to this:
Instructions 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 ti-community-infra/tichi repository. |
|
fixed by #67142 in release-8.5 |
|
/cherry-pick release-8.5-20260608-v8.5.6 |
|
@gengliqi: new pull request created to branch DetailsIn response to this:
Instructions 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 ti-community-infra/tichi repository. |
|
NO NEED cherry-pick to feature branch, you can add |
What problem does this PR solve?
Issue Number: close #67623
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit