Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Support tiup env command #788

Merged
merged 3 commits into from
Sep 21, 2020
Merged

Support tiup env command #788

merged 3 commits into from
Sep 21, 2020

Conversation

lucklove
Copy link
Member

Fix #632

Signed-off-by: lucklove gnu.crazier@gmail.com

What problem does this PR solve?

Fix #632

What is changed and how it works?

Add the env subcommand

Check List

Tests

  • Integration test

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release notes:

Support tiup env sub command

Fix pingcap#632

Signed-off-by: lucklove <gnu.crazier@gmail.com>
@lucklove lucklove added status/PTAL type/new-feature Categorizes pr as related to a new feature. labels Sep 17, 2020
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2020

Codecov Report

Merging #788 into master will decrease coverage by 9.88%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #788      +/-   ##
==========================================
- Coverage   52.35%   42.46%   -9.89%     
==========================================
  Files         261      259       -2     
  Lines       18820    18710     -110     
==========================================
- Hits         9853     7945    -1908     
- Misses       7432     9446    +2014     
+ Partials     1535     1319     -216     
Flag Coverage Δ
#cluster 35.93% <ø> (-8.78%) ⬇️
#dm 25.31% <ø> (ø)
#integrate 42.46% <92.85%> (-5.12%) ⬇️
#playground 21.69% <ø> (ø)
#tiup 10.73% <92.85%> (+0.11%) ⬆️
#unittest ?

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

Impacted Files Coverage Δ
cmd/env.go 92.30% <92.30%> (ø)
cmd/root.go 61.53% <100.00%> (+0.42%) ⬆️
pkg/utils/semver.go 0.00% <0.00%> (-100.00%) ⬇️
components/dm/ansible/worker.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/telemetry/scrub.go 6.66% <0.00%> (-80.00%) ⬇️
components/cluster/command/check.go 6.39% <0.00%> (-72.73%) ⬇️
pkg/cluster/task/limits.go 0.00% <0.00%> (-68.75%) ⬇️
pkg/cluster/task/sysctl.go 0.00% <0.00%> (-66.67%) ⬇️
pkg/telemetry/telemetry.go 0.00% <0.00%> (-66.67%) ⬇️
components/dm/ansible/import.go 0.00% <0.00%> (-61.74%) ⬇️
... and 68 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 81ec3eb...4badc41. Read the comment docs.

)

var envList = []string{
localdata.EnvNameHome,
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to add a comment in the localdata package and link here.

Copy link
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.

Rest LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 19, 2020
cmd/env.go Outdated

func showEnvList(names ...string) {
for _, name := range names {
fmt.Printf("%s=\"%s\"\n", name, os.Getenv(name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to show only value if the user specifies environment variables explicitly.

➜  devel go env GOPATH
/c/devel/gopath
➜  devel go env GOPATH GOROOT
/c/devel/gopath
/usr/local/go

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I have noticed the golang's behavior, but I don't know what's the benefit of this. Can you explain this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think to output the value of an envvar instead of key=value may help get the value in the script, and the script doesn't need to split the string.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

Signed-off-by: lucklove <gnu.crazier@gmail.com>
Copy link
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
Copy link
Contributor

lonng commented Sep 21, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 21, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 65fdf56 into pingcap:master Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/new-feature Categorizes pr as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tiup env subcommand to show all environment variables of tiup used.
4 participants