support deleting data volumes while deleting services #613

ilgooz · 2018-12-02T14:10:18Z

closes #422

@mesg-foundation/core Docker creates one more extra data volume which we cannot track it's creation so we cannot delete it. I don't know what it's for. We need to investigate it. I'm putting steps to reproduce.

First delete all of your data volumes for a clean inspection:

run docker volume rm $(docker volume ls -q --filter dangling=true)

Following has no problems:

run ./dev-cli service deploy https://github.com/ilgooz/service-articles
run docker volume ls > You'll see one.
run ./dev-cli service delete c13a74b6b50f19791de1ba3ab561928c71376882
run docker volume ls > You'll see all volumes gone.

Problematic one:

run ./dev-cli service deploy https://github.com/ilgooz/service-articles
run ./dev-cli service start c13a74b6b50f19791de1ba3ab561928c71376882
run docker volume ls > You'll see two.
run ./dev-cli service delete c13a74b6b50f19791de1ba3ab561928c71376882
run docker volume ls > You'll see one gone but extra volume left.

asc:core ilgooz$ docker volume ls
DRIVER              VOLUME NAME
asc:core ilgooz$ docker volume ls
DRIVER              VOLUME NAME
local               41030f6ecde6ef18ca7b66fb341351e1b950acf8
local               d2161474f2c18d96ef0a067fabd29f84e38485f4b71f70ba9acebe97926c0420
asc:core ilgooz$ docker volume ls
DRIVER              VOLUME NAME
local               d2161474f2c18d96ef0a067fabd29f84e38485f4b71f70ba9acebe97926c0420
asc:core ilgooz$

antho1404

General feeling: We talk about volumes, data, persistent data, from the user perspective and in the variable names, we might want to have some consistency. I would use volumes everywhere or persistent data, but we talk about data for the events and task, it can be confusing to use persistent data

api/deploy.go

commands/service_delete.go

ilgooz · 2018-12-03T06:22:23Z

@antho1404

General feeling: We talk about volumes, data, persistent data, from the user perspective and in the variable names, we might want to have some consistency. I would use volumes everywhere or persistent data, but we talk about data for the events and task, it can be confusing to use persistent data

In the users' perspective I think we should say persistent data because volume name is a bit technical. For internal use, data volume/volume name makes for sense because we're not deleting or creating data, we're creating data volumes or deleting data volumes.

Do you have any suggestions?

ilgooz · 2018-12-03T06:24:31Z

How about -delete-data-volumes?

api/delete.go

NicolasMahe · 2018-12-03T08:32:43Z

How about -delete-data-volumes?

I like --delete-data. From a user point of view, he doesn't know about docker volume. He only cares about data.

commands/service_delete.go

protobuf/coreapi/api.proto

Co-Authored-By: ilgooz <ilkergoktugozturk@gmail.com>

commands/service_delete.go

ilgooz · 2018-12-04T19:33:21Z

commands/service_delete_test.go

 }

-func TestServiceDeleteWithAllAndYesFlags(t *testing.T) {
+func TestServiceDeleteRunE(t *testing.T) {


This test doesn't cover the tests deleted within this commit. I don't understand why they're removed.

It tests only runE method. The previous tests were for both preRunE and runE at the same time

ilgooz · 2018-12-04T19:35:06Z

commands/survey.go

-import survey "gopkg.in/AlecAivazis/survey.v1"
-
-// Survey is a command line survey.
-type Survey interface {


we should have tests for survey. we already had some tests, why removing them within this commit?

Because it was too complicated,

Again there was a new interface, new mock, mocks in the tests just to test AskOne method. We need to find a better way to test it. If we will mock everything that we can't tests we will end up in mock hell.

Also, those tests mix testing of validation (preRunE) and execution (runE) and good unit tests should test only one behavior/func at once (test one thing at a time).

On survey's README, the guy suggest to use go-expect to catch stuff in stdout

I don't approve removing the tests for survey. Having one more mock doesn't lead to a mock hell. I had a look on suvery's testing utilities yesterday but it seemed complicated to me, mocks are way simpler. If you want to delete the tests anyway please put their replacement otherwise we will lose some meaningful tests.

https://github.com/mesg-foundation/core/issues/622

My motiviation to delete those tests was:

mock hell what I mean by that if we swtich to passing interface to commands we will need: daemon.Deamon, api.API, service.Service (maybe) CoreClient mocks so it's already a lot. Adding one more mock seems good from short perspective, but in long run it's not so obious.

so choosing mocking should be the last thing while testing. We alwasy need to try to find other solutions/design patterns that will free code of mocks. Why I think so. Look at go source code:
There is only one mock net/dnsclient_unix_test.go:func here.

➜ moby git:(master) gogrep -i mock |grep struct |wc -l 36

So docker has 36 mocks and we probably haven't reached 2% of their codebase and we already have:

➜ core git:(feature/auto-remove-volume) ✗ gogrep -i mock |grep struct |wc -l 13

13 mocks.

while mocking seems easy to go solution it is not in long run (compared to js where it's standard and it's super easy, but js are more flexible as it is dynamic programming language)

survey suggested go-expect and vx10 for testing but vx10 is suspicious to me, but it's still better to use it as it doesn't require to introduce changes in the source code.

about using https://github.com/AlecAivazis/survey/blob/v1.7.0/confirm_test.go#L21 I was wrong. It can't be used.

So to summarize we should think of a better solution if possible, sleep with it, look from a different perspective, but introduce mocks because it easy should be avoided in general.

In was the same with commands package before caputereStd was created, there were no tests at all and in the meantime, we introduced them because we find a way to do so.

Again, this is something that I just don't agree. There is no logical reason to remove a test if it's not replaced by an equivalent one. I'm not even talking about using the mocks here, it's not relevant.

Survey pkg doesn't provide a nice way of testing as I already checked so, it's a proper option to use mocks here. I don't mind adopting a better way. Could you please provide one? It seems that we're seeing things in different perspectives.

mock hell what I mean by that if we swtich to passing interface to commands we will need: daemon.Deamon, api.API, service.Service (maybe) CoreClient mocks so it's already a lot. Adding one more mock seems good from short perspective, but in long run it's not so obvious.

This is totally an another topic. If we want to reduce possibility for programming errors we should have tests about how daemon and coreapi used in the commands/* pkgs as well. And I'd like doing that with unit tests. And I remember that you said it's not necessary to test wrapper funcs (the default implementation between these pkgs). And we don't have tests for them. I also suggested to have at least some integration tests for reducing manual tests that we always required to do. Otherwise we don't have a way of catching bugs inside wrappers, the place where commands pkg does IO which is very important.

I too prefer using the native testing utilities that can be provided by the pkg itself. But testing how coreapi is used in commands pkg is easier by mocking the client side funcs of coreapi as I experimented. We had the same problem before with the go-application and go-service. We didn't use mocks there and implemented a fake net.Conn, which is a bit harder to maintain when compared to mocking because starting a fake gRPC server causes some async behavior that needed be synced otherwise it introduces bug into tests.

For daemon, we already have its interface so the best option is is to create mocks on top of that.

Survey pkg doesn't provide a nice way of testing as I already checked so, it's a proper option to use mocks here.

This is the point that it isn't good to use mocks. Again Survey pkg option for testing is still better then modifying code in order to introduce the Survey interface. Maybe there is an option to write Survery bridge. I don't know.

I don't mind adopting a better way. Could you please provide one?

I don't mind either, but as I said there is have no one at this moment, but shouldn't be the root cause of introducing mocks, because maybe there is another way that needs to be discovered.

For the rest of things is a different discussion, but in general, having too many tests as bad as having too little. And about the other things we can talk in different thread/PR

…se and test.

…ut in TestIntegrationDeleteVolumes

…foundation/core into feature/auto-remove-volume

ilgooz added 7 commits December 2, 2018 17:08

support deleting data volumes while deleting services, closes #422

79edc37

merge dev

8b3ed89

service: cleanup

35909fe

commands>dev: do not delete service's data by default

a23d9f6

fix typo

9f2f6ec

interface/grpc/*: fix tests

e08107b

interface/grpc/*: fix tests

bc244af

ilgooz requested review from krhubert and NicolasMahe December 2, 2018 15:00

ilgooz requested a review from a team December 2, 2018 15:00

ilgooz mentioned this pull request Dec 2, 2018

Delete docker's volume when deleting a service #422

Closed

antho1404 reviewed Dec 2, 2018

View reviewed changes

api/deploy.go Outdated Show resolved Hide resolved

krhubert reviewed Dec 2, 2018

View reviewed changes

commands/service_delete.go Outdated Show resolved Hide resolved

delete logic about creating volumes

cb1a0ad

NicolasMahe reviewed Dec 3, 2018

View reviewed changes

api/delete.go Show resolved Hide resolved

NicolasMahe reviewed Dec 3, 2018

View reviewed changes

commands/service_delete.go Outdated Show resolved Hide resolved

NicolasMahe reviewed Dec 3, 2018

View reviewed changes

commands/service_delete.go Outdated Show resolved Hide resolved

NicolasMahe reviewed Dec 3, 2018

View reviewed changes

protobuf/coreapi/api.proto Outdated Show resolved Hide resolved

NicolasMahe and others added 2 commits December 3, 2018 11:37

commands>delete: fix delete data flag

81f79a4

Co-Authored-By: ilgooz <ilkergoktugozturk@gmail.com>

commands>delete: change the default of delete data dialog to false

4c48c78

Co-Authored-By: ilgooz <ilkergoktugozturk@gmail.com>

NicolasMahe changed the title ~~support deleting data volumes while deleting services, closes #422~~ support deleting data volumes while deleting services Dec 3, 2018

NicolasMahe and others added 2 commits December 3, 2018 11:43

protobuf>coreapi: improve docs

3f7f55e

Co-Authored-By: ilgooz <ilkergoktugozturk@gmail.com>

generate pb files & docs

e54c021

krhubert dismissed NicolasMahe’s stale review via f75edb7 December 4, 2018 18:42

ilgooz assigned krhubert Dec 4, 2018

ilgooz commented Dec 4, 2018

View reviewed changes

commands/service_delete.go Show resolved Hide resolved

ilgooz commented Dec 4, 2018

View reviewed changes

krhubert and others added 9 commits December 4, 2018 23:20

Add one test case

1799814

Add delete volume integration test

3998e17

Separate dep.extractVolumes and dep.extractVolumesFrom. Update codeba…

2718c05

…se and test.

Simplify logic on data deletion confirmation question

ee556c5

Fix TestServiceDeletePreRunE

659edbc

Add container.option to newIntegrationContainer to set a longer timeo…

d4494c7

…ut in TestIntegrationDeleteVolumes

Increase TestIntegrationDeleteVolumes timeout to 1 minute

af9dfe3

Fix lint on dep.extractVolumes

25c5f61

Comment TestIntegrationDeleteVolumes because it doesn't work on CircleCI

3ade1f6

NicolasMahe force-pushed the feature/auto-remove-volume branch from 0739a74 to 3ade1f6 Compare December 5, 2018 05:58

NicolasMahe added 5 commits December 5, 2018 13:00

Remove option in newIntegrationContainer

ec382b5

Merge branch 'dev' into feature/auto-remove-volume

2374af9

Fix lint

ac80351

Merge branch 'feature/auto-remove-volume' of https://github.com/mesg-…

f0b15d9

…foundation/core into feature/auto-remove-volume

Remove survey mocks

669d864

NicolasMahe previously approved these changes Dec 5, 2018

View reviewed changes

Uncomment tests + remove discardOutput call

a0b2da9

krhubert dismissed NicolasMahe’s stale review via a0b2da9 December 5, 2018 11:55

krhubert approved these changes Dec 5, 2018

View reviewed changes

NicolasMahe approved these changes Dec 6, 2018

View reviewed changes

NicolasMahe merged commit a38df2d into dev Dec 6, 2018

NicolasMahe deleted the feature/auto-remove-volume branch December 6, 2018 02:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support deleting data volumes while deleting services #613

support deleting data volumes while deleting services #613

ilgooz commented Dec 2, 2018 •

edited by NicolasMahe

Loading

antho1404 left a comment

ilgooz commented Dec 3, 2018 •

edited

Loading

ilgooz commented Dec 3, 2018

NicolasMahe commented Dec 3, 2018 •

edited

Loading

ilgooz Dec 4, 2018

krhubert Dec 4, 2018 •

edited

Loading

ilgooz Dec 4, 2018

krhubert Dec 4, 2018

NicolasMahe Dec 5, 2018 •

edited

Loading

ilgooz Dec 5, 2018 •

edited

Loading

NicolasMahe Dec 5, 2018

krhubert Dec 5, 2018

ilgooz Dec 5, 2018 •

edited

Loading

krhubert Dec 5, 2018

support deleting data volumes while deleting services #613

support deleting data volumes while deleting services #613

Conversation

ilgooz commented Dec 2, 2018 • edited by NicolasMahe Loading

antho1404 left a comment

Choose a reason for hiding this comment

ilgooz commented Dec 3, 2018 • edited Loading

ilgooz commented Dec 3, 2018

NicolasMahe commented Dec 3, 2018 • edited Loading

ilgooz Dec 4, 2018

Choose a reason for hiding this comment

krhubert Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

ilgooz Dec 4, 2018

Choose a reason for hiding this comment

krhubert Dec 4, 2018

Choose a reason for hiding this comment

NicolasMahe Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

ilgooz Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

NicolasMahe Dec 5, 2018

Choose a reason for hiding this comment

krhubert Dec 5, 2018

Choose a reason for hiding this comment

ilgooz Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

krhubert Dec 5, 2018

Choose a reason for hiding this comment

ilgooz commented Dec 2, 2018 •

edited by NicolasMahe

Loading

ilgooz commented Dec 3, 2018 •

edited

Loading

NicolasMahe commented Dec 3, 2018 •

edited

Loading

krhubert Dec 4, 2018 •

edited

Loading

NicolasMahe Dec 5, 2018 •

edited

Loading

ilgooz Dec 5, 2018 •

edited

Loading

ilgooz Dec 5, 2018 •

edited

Loading