move delete logic to a pkg #5693

mschwrz · 2019-10-22T19:29:06Z

As in #4780 discussed, I moved the logic for the deletion of profiles and machines to the pkg folder, to decouple the command logic from the delete logic.
For this I introduced a new folder named delete, where I moved all the logic and testdata. I'm not happy with the name of the folder and created it because of import cycles, but we can discuss the name or the place where the logic sits if you want.
While doing this had to move the getClusterBootstrapper-Method from root.go to cluster.go.

Moved DeleteProfileDirectory to pkg

Moved DeleteInvalidProfile to pkg

Moved GetClusterBootstrapper to cluster pkg

Moved testdata to delete pkg

# Conflicts: # cmd/minikube/cmd/delete.go # cmd/minikube/cmd/mount.go # pkg/minikube/cluster/cluster.go # pkg/minikube/delete/delete_test.go

Bugfix

k8s-ci-robot · 2019-10-22T19:29:14Z

Hi @marekschwarz. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

minikube-bot · 2019-10-22T19:30:03Z

Can one of the admins verify this patch?

codecov-io · 2019-10-22T19:35:11Z

Codecov Report

Merging #5693 into master will decrease coverage by 0.12%.
The diff coverage is 23.86%.

@@            Coverage Diff             @@
##           master    #5693      +/-   ##
==========================================
- Coverage   36.54%   36.41%   -0.13%     
==========================================
  Files         110      111       +1     
  Lines        8114     8107       -7     
==========================================
- Hits         2965     2952      -13     
- Misses       4760     4765       +5     
- Partials      389      390       +1

Impacted Files	Coverage Δ
cmd/minikube/cmd/root.go	`58.51% <ø> (+4.4%)`	⬆️
cmd/minikube/cmd/mount.go	`8.54% <0%> (ø)`	⬆️
pkg/minikube/cluster/cluster.go	`35.59% <0%> (-2.02%)`	⬇️
pkg/minikube/cluster/machine.go	`45.31% <0%> (-5.57%)`	⬇️
cmd/minikube/cmd/status.go	`7.69% <0%> (ø)`	⬆️
cmd/minikube/cmd/logs.go	`11.11% <0%> (ø)`	⬆️
pkg/minikube/cluster/mount.go	`37.03% <0%> (-22.97%)`	⬇️
cmd/minikube/cmd/stop.go	`0% <0%> (ø)`	⬆️
cmd/minikube/cmd/delete.go	`9.3% <0%> (-19.85%)`	⬇️
cmd/minikube/cmd/start.go	`20% <0%> (ø)`	⬆️
... and 3 more

sharifelgamal · 2019-10-22T23:33:38Z

@minikube-bot OK to test

medyagh · 2019-10-23T02:43:31Z

@marekschwarz can you please provide a PR description of what it does and how it will affect other things?

RA489 · 2019-10-23T06:51:12Z

/ok-to-test

mschwrz · 2019-10-23T07:07:17Z

@marekschwarz can you please provide a PR description of what it does and how it will affect other things?

Done

medyagh · 2019-10-23T17:33:22Z

while I didn't study the PR, but a quick look I noticed it doesn't Read well

for example delete.RemoveProfiles() doesn't read natural to me.
I don't know what would be a better name to suggest since I haven't studied it. but I believe the code should read nice and not stutter.

for example
action.DeleteAll()
or
profile.DeleteAll()

Renamed DeleteProfile to DeleteProfileDirectory

TravisBuddy · 2019-10-23T19:38:38Z

Travis tests have failed

Hey @marekschwarz,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

make test

which go-bindata || GO111MODULE=off GOBIN="/home/travis/gopath/bin" go get github.com/jteeuwen/go-bindata/...
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.12.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/assets/assets.go -pkg assets deploy/addons/...
gofmt -s -w pkg/minikube/assets/assets.go
which go-bindata || GO111MODULE=off GOBIN="/home/travis/gopath/bin" go get github.com/jteeuwen/go-bindata/...
/home/travis/gopath/bin/go-bindata
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.12.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/translate/translations.go -pkg translate translations/...
gofmt -s -w pkg/minikube/translate/translations.go
./test.sh
= make lint =============================================================
golangci/golangci-lint info checking GitHub for tag 'v1.20.0'
golangci/golangci-lint info found version: 1.20.0 for v1.20.0/linux/amd64
golangci/golangci-lint info installed out/linters/golangci-lint
cmd/minikube/cmd/delete.go:20: File is not `goimports`-ed (goimports)
	"k8s.io/minikube/pkg/minikube/profile"
cmd/minikube/cmd/mount.go:21: File is not `goimports`-ed (goimports)
	"k8s.io/minikube/pkg/minikube/profile"
cmd/minikube/cmd/stop.go:20: File is not `goimports`-ed (goimports)
	profile2 "k8s.io/minikube/pkg/minikube/profile"
Makefile:341: recipe for target 'lint-ci' failed
make[1]: *** [lint-ci] Error 1
= go mod ================================================================
ok
Makefile:245: recipe for target 'test' failed
make: *** [test] Error 4

TravisBuddy Request Identifier: b2f1b7f0-f5cc-11e9-a1fe-396a3052531f

…/minikube into DELETE_LOGIC_TO_PKG

medyagh

minor changes requested

cmd/minikube/cmd/start.go

cmd/minikube/cmd/logs.go

pkg/minikube/cluster/mount.go

…DELETE_LOGIC_TO_PKG

# Conflicts: # cmd/minikube/cmd/delete.go

minikube-bot · 2019-11-05T00:13:55Z

Old binary: [180.365082364 170.932122883 180.425103299]
New binary: [171.581173364 163.22752629 185.423275946]
Average Old: 177.240770
Average New: 173.410659

mschwrz · 2019-11-05T14:21:32Z

/assign @medyagh

k8s-ci-robot · 2019-11-05T17:33:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marekschwarz
To complete the pull request process, please assign medyagh
You can assign the PR to them by writing /assign @medyagh in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

minikube-bot · 2019-11-05T17:56:27Z

Old binary: [186.533519105 175.746501642 180.046910261]
New binary: [192.234230559 172.547649495 172.524845534]
Average Old: 180.775644
Average New: 179.102242

medyagh · 2019-11-25T23:09:33Z

@marekschwarz sorry for the delay can u plz rebase

tstromberg · 2019-12-09T18:56:42Z

Do you mind resolving the merge conflicts?

mschwrz · 2019-12-16T19:38:39Z

Do you mind resolving the merge conflicts?

I will take care of it this week :-)

medyagh · 2019-12-21T03:43:05Z

/retest this please

# Conflicts: # cmd/minikube/cmd/delete.go # cmd/minikube/cmd/root.go # cmd/minikube/cmd/start.go # cmd/minikube/cmd/stop.go # pkg/minikube/cluster/cluster.go # pkg/minikube/machine/machine.go

TravisBuddy · 2019-12-23T14:05:41Z

Travis tests have failed

Hey @marekschwarz,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 4a5c7090-258d-11ea-8c31-f79bb2699b7f

minikube-bot · 2019-12-23T14:20:04Z

All Times minikube: [ 123.302119 126.979365 123.297754]
All Times Minikube (PR 5693): [ 197.269070 196.444170 197.530789]

Average minikube: 124.526413
Average Minikube (PR 5693): 197.081343

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 5693) |
+----------------------+-----------+--------------------+
| minikube v           |  0.397416 |           0.264217 |
| Creating kvm2        | 47.677672 |          40.306184 |
| Preparing Kubernetes | 49.891975 |          54.116033 |
| Pulling images       |  2.970122 |           2.647821 |
| Launching Kubernetes | 20.783975 |          20.979480 |
| Waiting for cluster  |  2.757686 |                    |
+----------------------+-----------+--------------------+

medyagh · 2019-12-24T09:55:40Z

@marekschwarz please make sure to run make test to see the test failures and make lint to see lint issues locally

minikube-bot · 2020-01-07T04:47:17Z

All Times minikube: [ 128.996987 132.344816 133.932775]
All Times Minikube (PR 5693): [ 200.793334 194.728134 188.542909]

Average minikube: 131.758193
Average Minikube (PR 5693): 194.688125

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 5693) |
+----------------------+-----------+--------------------+
| minikube v           |  0.434149 |           0.373043 |
| Creating kvm2        | 50.693711 |          44.419174 |
| Preparing Kubernetes | 51.966699 |          53.616097 |
| Pulling images       |  3.038614 |           2.197745 |
| Launching Kubernetes | 22.824704 |          21.223334 |
| Waiting for cluster  |  2.744661 |                    |
| Done                 |           |                    |
|                      |  0.058713 |          72.909395 |
+----------------------+-----------+--------------------+

medyagh · 2020-01-13T22:21:49Z

@marekschwarz are you still working on this ?

minikube-pr-bot · 2020-01-25T00:26:21Z

All Times minikube: [ 96.511587 96.447060 96.683337]
All Times Minikube (PR 5693): [ 202.063067 167.858721 187.369475]

Average minikube: 96.547328
Average Minikube (PR 5693): 185.763754

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 5693) |
+----------------------+-----------+--------------------+
| minikube v           |  0.238506 |           0.357990 |
| Creating kvm2        | 20.030817 |          41.428169 |
| Preparing Kubernetes | 49.273888 |          51.880981 |
| Pulling images       |  3.769051 |           2.309576 |
| Launching Kubernetes | 20.117193 |          19.953306 |
| Waiting for cluster  |  2.559414 |                    |
+----------------------+-----------+--------------------+

k8s-ci-robot · 2020-01-25T00:26:28Z

@marekschwarz: PR needs rebase.

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.

medyagh · 2020-01-28T23:28:48Z

@marekschwarz I close this PR for now, please feel free to re-open when ready

mschwrz added 11 commits October 21, 2019 14:24

Added delete and delete_test in pkg/minikube/config

6bfe4e8

Removed unnecessary import alias "pkg_config"

73f6608

Moved DeleteProfileDirectory to pkg

Introduced delete package

f2c156c

Moved DeleteInvalidProfile to pkg

Moved UninstallKubernetes to delete pkg

3e5c096

Moved GetClusterBootstrapper to cluster pkg

Moved KillMountProcess to delete pkg

842a924

Moved delete logic and error handling logic to delete pkg

cb8c72d

Moved delete_test to delete pkg

6268013

Moved testdata to delete pkg

Bugfix for delete_test

35e1de3

Merge branch 'master' into DELETE_LOGIC_TO_PKG

536e439

# Conflicts: # cmd/minikube/cmd/delete.go # cmd/minikube/cmd/mount.go # pkg/minikube/cluster/cluster.go # pkg/minikube/delete/delete_test.go

Integrated changes from master

69568d1

Bugfix

Fixed lint errors

42426f2

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 22, 2019

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 22, 2019

k8s-ci-robot requested review from josedonizetti and sharifelgamal October 22, 2019 19:29

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 23, 2019

mschwrz and others added 3 commits October 23, 2019 20:54

Merge branch 'master' into DELETE_LOGIC_TO_PKG

05597f0

Merge branch 'master' into DELETE_LOGIC_TO_PKG

17cc3c5

Renamed delete package to profile

66a2f0f

Renamed DeleteProfile to DeleteProfileDirectory

Merge branch 'DELETE_LOGIC_TO_PKG' of https://github.com/marekschwarz…

09bb0ea

…/minikube into DELETE_LOGIC_TO_PKG

tstromberg approved these changes Oct 30, 2019

View reviewed changes

Merge branch 'master' into DELETE_LOGIC_TO_PKG

9f737c5

medyagh requested changes Oct 31, 2019

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

cmd/minikube/cmd/logs.go Outdated Show resolved Hide resolved

pkg/minikube/cluster/mount.go Show resolved Hide resolved

mschwrz added 3 commits October 31, 2019 23:15

Fixed review findings

533c0ba

Merge remote-tracking branch 'marekschwarz/DELETE_LOGIC_TO_PKG' into …

5731a7e

…DELETE_LOGIC_TO_PKG

Merge branch 'master' into DELETE_LOGIC_TO_PKG

3db4f26

# Conflicts: # cmd/minikube/cmd/delete.go

k8s-ci-robot assigned medyagh Nov 5, 2019

Merge branch 'master' into DELETE_LOGIC_TO_PKG

7b0ece7

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2019

Merge branch 'master' into DELETE_LOGIC_TO_PKG

d31cc21

# Conflicts: # cmd/minikube/cmd/delete.go # cmd/minikube/cmd/root.go # cmd/minikube/cmd/start.go # cmd/minikube/cmd/stop.go # pkg/minikube/cluster/cluster.go # pkg/minikube/machine/machine.go

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2020

medyagh closed this Jan 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move delete logic to a pkg #5693

move delete logic to a pkg #5693

mschwrz commented Oct 22, 2019 •

edited

Loading

k8s-ci-robot commented Oct 22, 2019

minikube-bot commented Oct 22, 2019

codecov-io commented Oct 22, 2019 •

edited

Loading

sharifelgamal commented Oct 22, 2019

medyagh commented Oct 23, 2019

RA489 commented Oct 23, 2019

mschwrz commented Oct 23, 2019

medyagh commented Oct 23, 2019 •

edited

Loading

TravisBuddy commented Oct 23, 2019

medyagh left a comment

minikube-bot commented Nov 5, 2019

mschwrz commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

minikube-bot commented Nov 5, 2019

medyagh commented Nov 25, 2019

tstromberg commented Dec 9, 2019

mschwrz commented Dec 16, 2019

medyagh commented Dec 21, 2019

TravisBuddy commented Dec 23, 2019

minikube-bot commented Dec 23, 2019

medyagh commented Dec 24, 2019

minikube-bot commented Jan 7, 2020

medyagh commented Jan 13, 2020

minikube-pr-bot commented Jan 25, 2020

k8s-ci-robot commented Jan 25, 2020

medyagh commented Jan 28, 2020

move delete logic to a pkg #5693

move delete logic to a pkg #5693

Conversation

mschwrz commented Oct 22, 2019 • edited Loading

k8s-ci-robot commented Oct 22, 2019

minikube-bot commented Oct 22, 2019

codecov-io commented Oct 22, 2019 • edited Loading

Codecov Report

sharifelgamal commented Oct 22, 2019

medyagh commented Oct 23, 2019

RA489 commented Oct 23, 2019

mschwrz commented Oct 23, 2019

medyagh commented Oct 23, 2019 • edited Loading

TravisBuddy commented Oct 23, 2019

Travis tests have failed

1st Build

TravisBuddy Request Identifier: b2f1b7f0-f5cc-11e9-a1fe-396a3052531f

medyagh left a comment

Choose a reason for hiding this comment

minikube-bot commented Nov 5, 2019

mschwrz commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

minikube-bot commented Nov 5, 2019

medyagh commented Nov 25, 2019

tstromberg commented Dec 9, 2019

mschwrz commented Dec 16, 2019

medyagh commented Dec 21, 2019

TravisBuddy commented Dec 23, 2019

Travis tests have failed

TravisBuddy Request Identifier: 4a5c7090-258d-11ea-8c31-f79bb2699b7f

minikube-bot commented Dec 23, 2019

medyagh commented Dec 24, 2019

minikube-bot commented Jan 7, 2020

medyagh commented Jan 13, 2020

minikube-pr-bot commented Jan 25, 2020

k8s-ci-robot commented Jan 25, 2020

medyagh commented Jan 28, 2020

mschwrz commented Oct 22, 2019 •

edited

Loading

codecov-io commented Oct 22, 2019 •

edited

Loading

medyagh commented Oct 23, 2019 •

edited

Loading