Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

playground: print version when filling binpath #2334

Merged
merged 9 commits into from
Jul 15, 2024

Conversation

HuSharp
Copy link
Contributor

@HuSharp HuSharp commented Dec 19, 2023

What problem does this PR solve?

When filling in the binpath, it isbetter to print out the instance using binpath

What is changed and how it works?

before

$ tiup playground v7.5.0 --pd.binpath /Users/pingcap/CS/PingCAP/pd/bin/pd-server
Start pd instance:v7.5.0
Start tikv instance:v7.5.0
Start tidb instance:v7.5.0

after

$ ./tiup-playground v8.1.0 --pd.mode ms --pd 1 --tiflash 0 --scheduling 3 --tso 2 --pd.binpath /Users/pingcap/CS/PingCAP/pd/bin/pd-server
Start pd instance:/Users/pingcap/CS/PingCAP/pd/bin/pd-server
Start pd instance:/Users/pingcap/CS/PingCAP/pd/bin/pd-server
Start pd instance:/Users/pingcap/CS/PingCAP/pd/bin/pd-server
Start pd instance:/Users/pingcap/CS/PingCAP/pd/bin/pd-server
Start pd instance:/Users/pingcap/CS/PingCAP/pd/bin/pd-server
Start pd instance:/Users/pingcap/CS/PingCAP/pd/bin/pd-server
Start tikv instance:v8.1.0
Start tidb instance:v8.1.0

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Release notes:

print version when filling binpath

Signed-off-by: husharp <jinhao.hu@pingcap.com>
@ti-chi-bot ti-chi-bot bot requested a review from breezewish December 19, 2023 02:06
@HuSharp
Copy link
Contributor Author

HuSharp commented Dec 19, 2023

cc @lhy1024

@ti-chi-bot ti-chi-bot bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 19, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.91%. Comparing base (342c767) to head (b13c405).
Report is 1 commits behind head on master.

Current head b13c405 differs from pull request most recent head af5f9ec

Please upload reports for the commit af5f9ec to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2334      +/-   ##
==========================================
- Coverage   55.22%   50.91%   -4.32%     
==========================================
  Files         334      328       -6     
  Lines       35982    34984     -998     
==========================================
- Hits        19871    17810    -2061     
- Misses      13726    14853    +1127     
+ Partials     2385     2321      -64     
Flag Coverage Δ
tiup 33.59% <ø> (-0.23%) ⬇️
unittest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HuSharp HuSharp changed the title playground: print version playground: print version when filling binpath Dec 19, 2023
Copy link
Contributor

ti-chi-bot bot commented Dec 19, 2023

@lhy1024: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In 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/test-infra repository.

@@ -70,6 +70,8 @@ type Instance interface {
// Wait Should only call this if the instance is started successfully.
// The implementation should be safe to call Wait multi times.
Wait() error
// BinPathCheck return the bin path to check component version.
BinPathCheck(utils.Version) string
Copy link
Member

Choose a reason for hiding this comment

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

How about simply return the bin path (and call it BinPath)? The current API mixes two different things together, which is confusing and not useful for other purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about advancing the PrepareBinary function?

@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 19, 2023
Signed-off-by: husharp <jinhao.hu@pingcap.com>
Signed-off-by: husharp <jinhao.hu@pingcap.com>
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 19, 2023
Copy link
Member

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

ti-chi-bot bot commented Jan 4, 2024

@nolouch: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

lgtm

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/test-infra repository.

Signed-off-by: husharp <jinhao.hu@pingcap.com>
@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 5, 2024
Signed-off-by: husharp <jinhao.hu@pingcap.com>
Signed-off-by: husharp <jinhao.hu@pingcap.com>
@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 12, 2024
Signed-off-by: husharp <ihusharp@gmail.com>
@ti-chi-bot ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2024
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 15, 2024
Signed-off-by: husharp <ihusharp@gmail.com>
@HuSharp
Copy link
Contributor Author

HuSharp commented Jul 15, 2024

PTAL, thx! @xhebox

Signed-off-by: husharp <ihusharp@gmail.com>
Copy link
Contributor

ti-chi-bot bot commented Jul 15, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-07-15 04:20:32.73540351 +0000 UTC m=+241254.726344973: ☑️ agreed by xhebox.

@ti-chi-bot ti-chi-bot bot added the lgtm label Jul 15, 2024
@xhebox
Copy link
Collaborator

xhebox commented Jul 15, 2024

/approve

Copy link
Contributor

ti-chi-bot bot commented Jul 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xhebox

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

The pull request process is described here

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

@ti-chi-bot ti-chi-bot bot added the approved label Jul 15, 2024
@ti-chi-bot ti-chi-bot bot merged commit 654b087 into pingcap:master Jul 15, 2024
7 checks passed
@HuSharp HuSharp deleted the print_instance_ver branch July 15, 2024 04:36
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
approved lgtm size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants