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.

Sign up for GitHub

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

Jump to bottom

Add enable and disable sub command #654

Merged
merged 9 commits into from
Sep 1, 2020
Merged

Add enable and disable sub command #654

merged 9 commits into from
Sep 1, 2020

Conversation

lucklove
Copy link
Member

@lucklove lucklove commented Aug 5, 2020 •
edited
Loading

issue: https://asktug.com/t/topic/36387
TODOLIST:

  • Don't enable systemd service on in startInstance()
  • Enable systems service in deploy command and add --skip-enable as an option
  • Add enable and disable command

Unresoved issue: #697

Support tiup cluster enable and tiup cluster disable command

@lucklove lucklove marked this pull request as draft August 5, 2020 04:07
@lucklove lucklove changed the title Don't enable systemd module on start Add enable and disable sub command Aug 5, 2020
@lucklove lucklove added status/WIP type/new-feature Categorizes pr as related to a new feature. labels Aug 5, 2020
@lucklove lucklove self-assigned this Aug 5, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2020 •
edited
Loading

Codecov Report

Merging #654 into master will increase coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
+ Coverage   57.86%   58.07%   +0.21%     
==========================================
  Files         253      255       +2     
  Lines       18742    18902     +160     
==========================================
+ Hits        10845    10978     +133     
- Misses       6454     6471      +17     
- Partials     1443     1453      +10
Flag Coverage Δ
#coverage 58.07% <ø> (+0.21%) ⬆️

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

Impacted Files Coverage Δ
...hub.com/pingcap/tiup/pkg/cluster/module/systemd.go 72.00% <0.00%> (-3.00%) ⬇️
...pingcap/tiup/components/cluster/command/disable.go 73.33% <0.00%> (ø)
.../pingcap/tiup/components/cluster/command/enable.go 73.33% <0.00%> (ø)
...src/github.com/pingcap/tiup/pkg/cluster/manager.go 69.49% <0.00%> (+0.49%) ⬆️
...c/github.com/pingcap/tiup/pkg/cluster/api/pdapi.go 55.48% <0.00%> (+0.60%) ⬆️
...om/pingcap/tiup/components/cluster/command/root.go 46.51% <0.00%> (+0.62%) ⬆️
.../pingcap/tiup/components/cluster/command/deploy.go 73.58% <0.00%> (+2.15%) ⬆️
...c/github.com/pingcap/tiup/pkg/cluster/task/step.go 78.40% <0.00%> (+2.27%) ⬆️
...b.com/pingcap/tiup/pkg/cluster/operation/action.go 56.02% <0.00%> (+5.10%) ⬆️
.../src/github.com/pingcap/tiup/pkg/logger/log/log.go 100.00% <0.00%> (+25.00%) ⬆️

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 82f6dc3...177339f. Read the comment docs.

@lucklove
Don't enable systemd module on start
f47df95 
Signed-off-by: lucklove <gnu.crazier@gmail.com>
@lucklove
Support enable and disable sub command
818943b 
Signed-off-by: lucklove <gnu.crazier@gmail.com>
@lucklove lucklove marked this pull request as ready for review August 19, 2020 09:50
@lucklove lucklove requested review from july2993 and AstroProfundis and removed request for july2993 August 19, 2020 09:51
@lucklove
Fix test
c1c2e49 
Signed-off-by: lucklove <gnu.crazier@gmail.com>
@lonng lonng added the status/need-doc Indicates that we should update document before merge a PR. label Aug 20, 2020
@lonng lonng added this to the v1.1.0 milestone Aug 20, 2020
AstroProfundis
AstroProfundis reviewed Aug 20, 2020
View reviewed changes
components/cluster/command/deploy.go Outdated Show resolved Hide resolved
components/cluster/command/disable.go Outdated Show resolved Hide resolved
pkg/cluster/operation/action.go Show resolved Hide resolved
tests/tiup-cluster/script/cmd_subtest.sh Show resolved Hide resolved
lucklove and others added 2 commits August 20, 2020 14:19
@lucklove @lonng
Update components/cluster/command/disable.go
827c4f0 
Co-authored-by: Lonng <heng@lonng.org>
@lucklove @lonng
Update components/cluster/command/disable.go
31adab9 
Co-authored-by: Lonng <heng@lonng.org>
@lucklove @lonng
Update components/cluster/command/enable.go
c61d2f6 
Co-authored-by: Lonng <heng@lonng.org>
lonng
lonng reviewed Aug 20, 2020
View reviewed changes
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.

This PR needs to take some consideration more carefully about how to handle the exporters processes.

@lucklove
Address comment
7efb962 
Signed-off-by: lucklove <gnu.crazier@gmail.com>
@lucklove
Copy link
Member Author

This PR needs to take some consideration more carefully about how to handle the exporters processes.

@lonng I added an unresolved issue: #697

@AstroProfundis AstroProfundis modified the milestones: v1.1.0, v1.2.0 Aug 28, 2020
lonng
lonng approved these changes Sep 1, 2020
View reviewed changes
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

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 1, 2020
@lonng
Copy link
Contributor

lonng commented Sep 1, 2020

/merge

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

/run-all-tests

@ti-srebot ti-srebot merged commit cfed19f into pingcap:master Sep 1, 2020
@lonng lonng deleted the without-enable branch September 2, 2020 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AstroProfundis AstroProfundis AstroProfundis left review comments

@lonng lonng lonng approved these changes

@july2993 july2993 Awaiting requested review from july2993

Assignees

@lucklove lucklove

Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. status/need-doc Indicates that we should update document before merge a PR. type/new-feature Categorizes pr as related to a new feature.
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lucklove @codecov-commenter @lonng @ti-srebot @AstroProfundis