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 deleting data volumes while deleting services #613

Merged
merged 32 commits into from Dec 6, 2018
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
79edc37
support deleting data volumes while deleting services, closes #422
ilgooz Nov 29, 2018
8b3ed89
merge dev
ilgooz Dec 2, 2018
35909fe
service: cleanup
ilgooz Dec 2, 2018
a23d9f6
commands>dev: do not delete service's data by default
ilgooz Dec 2, 2018
9f2f6ec
fix typo
ilgooz Dec 2, 2018
e08107b
interface/grpc/*: fix tests
ilgooz Dec 2, 2018
bc244af
interface/grpc/*: fix tests
ilgooz Dec 2, 2018
cb1a0ad
delete logic about creating volumes
ilgooz Dec 3, 2018
81f79a4
commands>delete: fix delete data flag
NicolasMahe Dec 3, 2018
4c48c78
commands>delete: change the default of delete data dialog to false
NicolasMahe Dec 3, 2018
3f7f55e
protobuf>coreapi: improve docs
NicolasMahe Dec 3, 2018
e54c021
generate pb files & docs
ilgooz Dec 3, 2018
5a52246
commands>delete: have yes, all, keep-data flags. add some tests
ilgooz Dec 4, 2018
23fa50c
merge dev
ilgooz Dec 4, 2018
6480e18
commands>delete: confirm single service deletion, add more tests
ilgooz Dec 4, 2018
3c2dab7
Merge branch 'dev' into feature/auto-remove-volume
NicolasMahe Dec 4, 2018
f75edb7
Lint
krhubert Dec 4, 2018
1799814
Add one test case
krhubert Dec 4, 2018
3998e17
Add delete volume integration test
NicolasMahe Dec 5, 2018
2718c05
Separate dep.extractVolumes and dep.extractVolumesFrom. Update codeba…
NicolasMahe Dec 5, 2018
ee556c5
Simplify logic on data deletion confirmation question
NicolasMahe Dec 5, 2018
659edbc
Fix TestServiceDeletePreRunE
NicolasMahe Dec 5, 2018
d4494c7
Add container.option to newIntegrationContainer to set a longer timeo…
NicolasMahe Dec 5, 2018
af9dfe3
Increase TestIntegrationDeleteVolumes timeout to 1 minute
NicolasMahe Dec 5, 2018
25c5f61
Fix lint on dep.extractVolumes
NicolasMahe Dec 5, 2018
3ade1f6
Comment TestIntegrationDeleteVolumes because it doesn't work on CircleCI
NicolasMahe Dec 5, 2018
ec382b5
Remove option in newIntegrationContainer
NicolasMahe Dec 5, 2018
2374af9
Merge branch 'dev' into feature/auto-remove-volume
NicolasMahe Dec 5, 2018
ac80351
Fix lint
NicolasMahe Dec 5, 2018
f0b15d9
Merge branch 'feature/auto-remove-volume' of https://github.com/mesg-…
NicolasMahe Dec 5, 2018
669d864
Remove survey mocks
NicolasMahe Dec 5, 2018
a0b2da9
Uncomment tests + remove discardOutput call
krhubert Dec 5, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions api/delete.go
Expand Up @@ -5,8 +5,10 @@ import (
)

// DeleteService stops and deletes service serviceID.
func (a *API) DeleteService(serviceID string) error {
return newServiceDeletor(a).Delete(serviceID)
// when deleteData is enabled, any persistent data that belongs to
// service and its dependencies also will be deleted.
func (a *API) DeleteService(serviceID string, deleteData bool) error {
return newServiceDeletor(a).Delete(serviceID, deleteData)
}

// serviceDeletor provides functionalities to delete a MESG service.
Expand All @@ -22,7 +24,9 @@ func newServiceDeletor(api *API) *serviceDeletor {
}

// Delete stops and deletes service serviceID.
func (d *serviceDeletor) Delete(serviceID string) error {
// when deleteData is enabled, any persistent data that belongs to
// service and its dependencies also will be deleted.
func (d *serviceDeletor) Delete(serviceID string, deleteData bool) error {
s, err := d.api.db.Get(serviceID)
if err != nil {
return err
Expand All @@ -34,5 +38,13 @@ func (d *serviceDeletor) Delete(serviceID string) error {
if err := s.Stop(); err != nil {
return err
}
// delete volumes first before the service. this way if
// deleting volumes fails, process can be retried by the user again
// because service still will be in the db.
if deleteData {
NicolasMahe marked this conversation as resolved.
Show resolved Hide resolved
if err := s.DeleteVolumes(); err != nil {
return err
}
}
return d.api.db.Delete(serviceID)
}
1 change: 1 addition & 0 deletions api/deploy.go
Expand Up @@ -139,6 +139,7 @@ func (d *serviceDeployer) deploy(r io.Reader) (*service.Service, *importer.Valid
if validationErr != nil {
return nil, validationErr, nil
}

return s, nil, d.api.db.Save(s)
}

Expand Down
10 changes: 8 additions & 2 deletions commands/commands.go
Expand Up @@ -2,6 +2,7 @@ package commands

import (
"io"
"io/ioutil"

"github.com/mesg-foundation/core/commands/provider"
"github.com/mesg-foundation/core/container"
Expand All @@ -21,8 +22,8 @@ type RootExecutor interface {
// ServiceExecutor is an interface that handles services commands.
type ServiceExecutor interface {
ServiceByID(id string) (*coreapi.Service, error)
ServiceDeleteAll() error
ServiceDelete(ids ...string) error
ServiceDeleteAll(deleteData bool) error
ServiceDelete(deleteData bool, ids ...string) error
ServiceDeploy(path string, statuses chan provider.DeployStatus) (id string, validationError, err error)
ServiceListenEvents(id, eventFilter string) (chan *coreapi.EventData, chan error, error)
ServiceListenResults(id, taskFilter, outputFilter string, tagFilters []string) (chan *coreapi.ResultData, chan error, error)
Expand Down Expand Up @@ -65,6 +66,11 @@ func newCommand(c *cobra.Command) *cobra.Command {
return c
}

// discardOutput discards usage and error messages.
func (c *baseCmd) discardOutput() {
c.cmd.SetOutput(ioutil.Discard)
}

// getFirstOrDefault returns directory if args len is gt 0 or current directory.
func getFirstOrDefault(args []string) string {
if len(args) > 0 {
Expand Down
5 changes: 5 additions & 0 deletions commands/commands_test.go
Expand Up @@ -66,6 +66,11 @@ func newMockExecutor() *mocks.Executor {
return &mocks.Executor{}
}

// newMockSurvey returns a Survey mock for testing.
func newMockSurvey() *mocks.Survey {
return &mocks.Survey{}
}

func TestBaseCommandCmd(t *testing.T) {
// NOTE: this test is only to satisfy structcheck
// as it doesn't handle embedded structs yet.
Expand Down
19 changes: 10 additions & 9 deletions commands/mocks/Executor.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 32 additions & 0 deletions commands/mocks/Survey.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 10 additions & 5 deletions commands/provider/service_provider.go
Expand Up @@ -38,7 +38,7 @@ func (p *ServiceProvider) ServiceByID(id string) (*coreapi.Service, error) {
}

// ServiceDeleteAll deletes all services.
func (p *ServiceProvider) ServiceDeleteAll() error {
func (p *ServiceProvider) ServiceDeleteAll(deleteData bool) error {
rep, err := p.client.ListServices(context.Background(), &coreapi.ListServicesRequest{})
if err != nil {
return err
Expand All @@ -51,7 +51,10 @@ func (p *ServiceProvider) ServiceDeleteAll() error {
wg.Add(len(rep.Services))
for _, s := range rep.Services {
go func(id string) {
_, err := p.client.DeleteService(context.Background(), &coreapi.DeleteServiceRequest{ServiceID: id})
_, err := p.client.DeleteService(context.Background(), &coreapi.DeleteServiceRequest{
ServiceID: id,
DeleteData: deleteData,
})
if err != nil {
errs.Append(err)
}
Expand All @@ -63,11 +66,13 @@ func (p *ServiceProvider) ServiceDeleteAll() error {
}

// ServiceDelete deletes service with given ids.
func (p *ServiceProvider) ServiceDelete(ids ...string) error {
func (p *ServiceProvider) ServiceDelete(deleteData bool, ids ...string) error {
var errs xerrors.Errors
for _, id := range ids {
_, err := p.client.DeleteService(context.Background(), &coreapi.DeleteServiceRequest{ServiceID: id})
if err != nil {
if _, err := p.client.DeleteService(context.Background(), &coreapi.DeleteServiceRequest{
ServiceID: id,
DeleteData: deleteData,
}); err != nil {
errs = append(errs, err)
}
}
Expand Down
69 changes: 55 additions & 14 deletions commands/service_delete.go
Expand Up @@ -10,11 +10,17 @@ import (
survey "gopkg.in/AlecAivazis/survey.v1"
)

var (
errConfirmationNeeded = errors.New("can't continue without confirmation")
errNoID = errors.New("at least one service id must be provided (or run with --all flag)")
)

type serviceDeleteCmd struct {
baseCmd

all bool
force bool
yes bool
all bool
keepData bool

e ServiceExecutor
}
Expand All @@ -29,39 +35,74 @@ mesg-core service delete --all`,
PreRunE: c.preRunE,
RunE: c.runE,
})

c.cmd.Flags().BoolVarP(&c.yes, "yes", "y", c.yes, `Automatic "yes" to all prompts and run non-interactively`)
c.cmd.Flags().BoolVar(&c.all, "all", c.all, "Delete all services")
c.cmd.Flags().BoolVarP(&c.force, "force", "f", c.force, "Force delete all services")
c.cmd.Flags().BoolVar(&c.keepData, "keep-data", c.keepData, "Do not delete services' persistent data")
NicolasMahe marked this conversation as resolved.
Show resolved Hide resolved
return c
}

func (c *serviceDeleteCmd) preRunE(cmd *cobra.Command, args []string) error {
if len(args) == 0 && !c.all {
return errors.New("at least one service id must be provided (or run with --all flag)")
return errNoID
}

if !c.all || (c.all && c.force) {
if c.yes {
return nil
}

if err := survey.AskOne(&survey.Confirm{
Message: "Are you sure to delete all services?",
}, &c.force, nil); err != nil {
if err := c.confirmServiceDelete(); err != nil {
return err
}

if !c.keepData {
if err := c.confirmDataDelete(); err != nil {
return err
}
}

return nil
}

// confirmServiceDelete prompts a confirmation dialog for deleting services.
func (c *serviceDeleteCmd) confirmServiceDelete() error {
var (
confirmed bool
confirm = &survey.Confirm{
Message: "Are you sure to delete service(s)?",
}
)

if c.all {
confirm.Message = "Are you sure to delete all services?"
}

if err := survey.AskOne(confirm, &confirmed, nil); err != nil {
return err
}

// if still no confirm .
if !c.force {
return errors.New("can't continue without confirmation")
if !confirmed {
return errConfirmationNeeded
}
return nil
}

// confirmServiceDelete prompts a confirmation dialog for deleting services' data.
func (c *serviceDeleteCmd) confirmDataDelete() error {
if err := survey.AskOne(&survey.Confirm{
Message: "Are you sure to remove service(s)' persistent data as well?",
}, &c.keepData, nil); err != nil {
return err
}

c.keepData = !c.keepData
NicolasMahe marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

func (c *serviceDeleteCmd) runE(cmd *cobra.Command, args []string) error {
var err error
if c.all {
pretty.Progress("Deleting all services...", func() {
err = c.e.ServiceDeleteAll()
err = c.e.ServiceDeleteAll(!c.keepData)
})
if err != nil {
return err
Expand All @@ -75,7 +116,7 @@ func (c *serviceDeleteCmd) runE(cmd *cobra.Command, args []string) error {
// build function to avoid using arg inside progress
fn := func(id string) func() {
return func() {
err = c.e.ServiceDelete(id)
err = c.e.ServiceDelete(!c.keepData, id)
}
}(arg)
pretty.Progress(fmt.Sprintf("Deleting service %q...", arg), fn)
Expand Down
57 changes: 52 additions & 5 deletions commands/service_delete_test.go
Expand Up @@ -7,14 +7,61 @@ import (
)

func TestServiceDeleteCmdFlags(t *testing.T) {
c := newServiceDeleteCmd(nil)
var (
c = newServiceDeleteCmd(nil)
flags = c.cmd.Flags()
)

// check defaults.
require.False(t, c.yes)
require.False(t, c.all)
require.False(t, c.keepData)

flags := c.cmd.Flags()
require.Equal(t, flags.ShorthandLookup("f"), flags.Lookup("force"))
require.Equal(t, flags.ShorthandLookup("y"), flags.Lookup("yes"))

flags.Set("force", "true")
require.True(t, c.force)
flags.Set("yes", "true")
require.True(t, c.yes)

flags.Set("all", "true")
require.True(t, c.all)

flags.Set("keep-data", "true")
require.True(t, c.keepData)
}

func TestServiceDeletePreRunE(t *testing.T) {
c := newServiceDeleteCmd(nil)
c.discardOutput()
require.Equal(t, errNoID, c.preRunE(c.cmd, nil))
}

func TestServiceDeleteRunE(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@krhubert krhubert Dec 4, 2018

Choose a reason for hiding this comment

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

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

var (
m = newMockExecutor()
c = newServiceDeleteCmd(m)
)

tests := []struct {
all bool
keepData bool
arg string
}{
{all: false, keepData: false, arg: "0"},
{all: false, keepData: true, arg: "0"},
{all: true, keepData: false},
{all: true, keepData: true},
}

for _, tt := range tests {
c.all = tt.all
c.keepData = tt.keepData

if tt.all {
m.On("ServiceDeleteAll", !tt.keepData).Once().Return(nil)
} else {
m.On("ServiceDelete", !tt.keepData, tt.arg).Once().Return(nil)
}
require.NoError(t, c.runE(c.cmd, []string{tt.arg}))
}
m.AssertExpectations(t)
}
2 changes: 1 addition & 1 deletion commands/service_dev.go
Expand Up @@ -79,7 +79,7 @@ func (c *serviceDevCmd) runE(cmd *cobra.Command, args []string) error {
defer func() {
var err error
pretty.Progress("Removing the service...", func() {
err = c.e.ServiceDelete(id)
err = c.e.ServiceDelete(false, id)
})
if err != nil {
fmt.Printf("%s Removing the service completed with an error: %s\n", pretty.FailSign, err)
Expand Down