Skip to content

specify component binary path#82

Merged
lonng merged 5 commits into
pingcap:masterfrom
fredchenbj:fredchenbj/add_component_binary_path
Mar 29, 2020
Merged

specify component binary path#82
lonng merged 5 commits into
pingcap:masterfrom
fredchenbj:fredchenbj/add_component_binary_path

Conversation

@fredchenbj
Copy link
Copy Markdown
Contributor

UCP #77

What problem does this PR solve?

Support specifies the binary path when running a component.

What is changed and how it works?

it runs like tiup playground --kv 3 --db 3 --pd 3 --db.binpath /xx/tidb-server --kv.binpath /xx/tikv-server --pd.binpath /xx/pd-server

Tests

  • Manual test

Signed-off-by: fredchenbj <cfworking@163.com>
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Mar 25, 2020

Thanks for your contribution. If your PR get merged, you will be rewarded 50 points.

@fredchenbj fredchenbj force-pushed the fredchenbj/add_component_binary_path branch from ee92217 to 3dfb0d0 Compare March 25, 2020 06:19
Signed-off-by: fredchenbj <cfworking@163.com>
@fredchenbj fredchenbj force-pushed the fredchenbj/add_component_binary_path branch from 3dfb0d0 to a73bb49 Compare March 25, 2020 06:33
@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 25, 2020

Codecov Report

Merging #82 into master will increase coverage by 3.96%.
The diff coverage is 20.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
+ Coverage   29.33%   33.29%   +3.96%     
==========================================
  Files          17       18       +1     
  Lines         958      889      -69     
==========================================
+ Hits          281      296      +15     
+ Misses        625      544      -81     
+ Partials       52       49       -3     
Impacted Files Coverage Δ
cmd/help.go 6.12% <0.00%> (-4.05%) ⬇️
cmd/run.go 0.00% <0.00%> (ø)
cmd/uninstall.go 43.75% <0.00%> (-1.91%) ⬇️
pkg/repository/component.go 0.00% <ø> (ø)
pkg/repository/mirror.go 8.19% <ø> (ø)
pkg/repository/version.go 78.26% <ø> (ø)
cmd/status.go 11.90% <20.00%> (ø)
pkg/repository/repository.go 36.36% <28.57%> (ø)
cmd/update.go 36.06% <31.25%> (+5.47%) ⬆️
cmd/clean.go 18.91% <33.33%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1a8dad...5dbc958. Read the comment docs.

@fredchenbj
Copy link
Copy Markdown
Contributor Author

@lonng PTAL

@lonng
Copy link
Copy Markdown
Contributor

lonng commented Mar 25, 2020

I think we shouldn't change the protocal component:version. Prefer to use a flag --binpath to specify the binary path.

The protocal component:version:binpath could make the user confused. And will introduce more corner case like component:binpath etc...

The protocal component:version can be used in install/update/uninstall.

@fredchenbj fredchenbj force-pushed the fredchenbj/add_component_binary_path branch 2 times, most recently from d58e1f3 to 0f6c6f4 Compare March 26, 2020 02:42
@fredchenbj
Copy link
Copy Markdown
Contributor Author

I think we shouldn't change the protocal component:version. Prefer to use a flag --binpath to specify the binary path.

The protocal component:version:binpath could make the user confused. And will introduce more corner case like component:binpath etc...

The protocal component:version can be used in install/update/uninstall.

I have modified it, please review again. @lonng

Signed-off-by: fredchenbj <cfworking@163.com>
@fredchenbj fredchenbj force-pushed the fredchenbj/add_component_binary_path branch from 0f6c6f4 to 158401d Compare March 26, 2020 02:47
Comment thread cmd/root.go Outdated
Comment thread components/playground/instance/pd.go Outdated
Comment thread components/playground/main.go
Signed-off-by: fredchenbj <cfworking@163.com>
Comment thread components/playground/main.go Outdated
Comment thread components/playground/main.go Outdated
@lonng
Copy link
Copy Markdown
Contributor

lonng commented Mar 29, 2020

@fredchenbj Thanks for your contribution, almost LGTM. It's better to add some integration tests in tests directory, you just need to add some cases to tests/cases.json and put the expected result to tests/expected directory.

@fredchenbj
Copy link
Copy Markdown
Contributor Author

@fredchenbj Thanks for your contribution, almost LGTM. It's better to add some integration tests in tests directory, you just should add some case to tests/cases.json and put the expected result to tests/expected directory.

Thanks for your review. Adding integration test is ok, but I don't know the binary path or binary version of component when ci test.

Signed-off-by: fredchenbj <cfworking@163.com>
Copy link
Copy Markdown
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

LGTM

@lonng lonng merged commit 424babc into pingcap:master Mar 29, 2020
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Mar 29, 2020

Congratulation! You have awarded a badge for usability challenge program! Please fill the form to get your reward! http://tidbcommunity.mikecrm.com/QMCv4QL

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Mar 29, 2020

Team fredchenbj complete task #77 and get 50 score, currerent score 50

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.

6 participants