tests: support overriding HUB_ADDR/PD_IMAGE/TIKV_IMAGE/TIDB_IMAGE via env vars in next-gen fullstack test#10791
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
size/S
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRegistry base defaulting now only applies when Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
/hold |
ti-chi-bot
bot
added
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/fullstack-test-next-gen/run.sh (1)
46-46: Consider mappingmasterforTIKV_BRANCHas well for consistency.At Line 46, only empty/
dedicatedis remapped. Sinceset_branch()(intests/docker/util.sh, Lines 121-130) can setTIKV_BRANCHfromBRANCH, aBRANCH=masterpath can leave TiKV on:masterwhile PD/TiDB are mapped to next-gen branches.Proposed adjustment
- if [[ -z "${TIKV_BRANCH}" || "${TIKV_BRANCH}" == "dedicated" ]]; then + if [[ -z "${TIKV_BRANCH}" || "${TIKV_BRANCH}" == "dedicated" || "${TIKV_BRANCH}" == "master" ]]; then TIKV_BRANCH="cloud-engine-next-gen" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fullstack-test-next-gen/run.sh` at line 46, The TIKV_BRANCH assignment only remaps empty/`dedicated` but misses handling `master`, so when set_branch() sets BRANCH=master TiKV can remain on `:master` while PD/TiDB use next-gen; update the logic that sets TIKV_BRANCH (the TIKV_BRANCH="cloud-engine-next-gen" assignment) to also map BRANCH=="master" to "cloud-engine-next-gen" (mirror the existing mapping for empty/dedicated) so TIKV_BRANCH is consistent with PD/TiDB when BRANCH is master.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/fullstack-test-next-gen/run.sh`:
- Line 46: The TIKV_BRANCH assignment only remaps empty/`dedicated` but misses
handling `master`, so when set_branch() sets BRANCH=master TiKV can remain on
`:master` while PD/TiDB use next-gen; update the logic that sets TIKV_BRANCH
(the TIKV_BRANCH="cloud-engine-next-gen" assignment) to also map
BRANCH=="master" to "cloud-engine-next-gen" (mirror the existing mapping for
empty/dedicated) so TIKV_BRANCH is consistent with PD/TiDB when BRANCH is
master.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4abfcf93-8153-424f-a163-541d3e3648c4
📒 Files selected for processing (1)
tests/fullstack-test-next-gen/run.sh
|
@wuhuizuo: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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. |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fullstack-test-next-gen/run.sh`:
- Around line 45-46: The branch rename is incomplete: replace all occurrences of
the old tag string "dedicated-next-gen" with "cloud-engine-next-gen" in the test
infra files (the Makefile used for next-gen Docker builds and the cluster.yaml
Docker Compose config) so both the Makefile and cluster.yaml reference
"cloud-engine-next-gen" consistently; search for the exact token
"dedicated-next-gen" and update it to "cloud-engine-next-gen" and verify the
CI/test run uses the new variable TIKV_BRANCH value.
🪄 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: 62e90179-2000-4cb1-9f15-6fa00a882065
📒 Files selected for processing (1)
tests/fullstack-test-next-gen/run.sh
… env vars in next-gen fullstack test
JaySon-Huang
force-pushed
the
fix_ng_image
branch
from
971bcb9 to
e81d449
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fullstack-test-next-gen/run.sh`:
- Line 41: Update the fallback image defaults in cluster.yaml to match the new
HUB_ADDR namespace by replacing occurrences of the old
"us-docker.pkg.dev/pingcap-testing-account/hub/..." with
"us-docker.pkg.dev/pingcap-testing-account/tidbx/..." for the PD_IMAGE,
TIKV_IMAGE and TIDB_IMAGE fallbacks so that PD_IMAGE, TIKV_IMAGE and TIDB_IMAGE
use the tidbx namespace when run.sh is not sourced.
🪄 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: dadd96f3-4ccc-4eaa-a018-ab11d9aa7c21
📒 Files selected for processing (3)
tests/docker/next-gen-utils/Makefiletests/docker/next-gen-yaml/cluster.yamltests/fullstack-test-next-gen/run.sh
✅ Files skipped from review due to trivial changes (1)
- tests/docker/next-gen-yaml/cluster.yaml
|
@JaySon-Huang: 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 |
Signed-off-by: JaySon-Huang <tshent@qq.com>
|
/unhold |
ti-chi-bot
bot
removed
the
do-not-merge/hold
ti-chi-bot
bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CalvinNeo, JinheLin, wuhuizuo 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
lgtm
and removed
needs-1-more-lgtm
4 participants
What problem does this PR solve?
Issue Number: ref #6233
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit