From cc686ab4a2c08ccd427edf068ca164354fba1aa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jules=20Cast=C3=A9ran?= Date: Sat, 19 Nov 2022 18:00:19 +0100 Subject: [PATCH 01/10] feat(core): add tasks package that allows pretty printing of tasks --- go.mod | 1 + internal/tasks/tasks.go | 51 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) create mode 100644 internal/tasks/tasks.go diff --git a/go.mod b/go.mod index 55b2643834..ccc26396c7 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.19 require ( github.com/alecthomas/assert v1.0.0 + github.com/briandowns/spinner v1.19.0 github.com/c-bata/go-prompt v0.2.5 github.com/chzyer/readline v1.5.1 github.com/containerd/console v1.0.3 diff --git a/internal/tasks/tasks.go b/internal/tasks/tasks.go new file mode 100644 index 0000000000..6e211db5cc --- /dev/null +++ b/internal/tasks/tasks.go @@ -0,0 +1,51 @@ +package tasks + +import ( + "fmt" + "math/rand" + "time" + + "github.com/briandowns/spinner" +) + +type Task func(interface{}) (interface{}, error) + +type taskInfo struct { + Name string + function Task +} + +type Tasks struct { + tasks []taskInfo +} + +func Begin() *Tasks { + return &Tasks{} +} + +func (ts *Tasks) Add(name string, task Task) { + ts.tasks = append(ts.tasks, taskInfo{ + Name: name, + function: task, + }) +} + +func (ts *Tasks) Execute(data interface{}) (interface{}, error) { + var err error + totalTasks := len(ts.tasks) + rand.Seed(time.Now().UnixNano()) + spin := spinner.New(spinner.CharSets[rand.Int()%37], 100*time.Millisecond) + + for i, task := range ts.tasks { + fmt.Printf("[%d/%d] %s\n", i+1, totalTasks, task.Name) + spin.Start() + data, err = task.function(data) + if err != nil { + spin.Stop() + return nil, fmt.Errorf("task [%d/%d] failed: %w", i, totalTasks, err) + } + spin.Stop() + } + + return data, nil +} From 0f1e1948b58200569151e29b99daaf52c092fb97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jules=20Cast=C3=A9ran?= Date: Sat, 19 Nov 2022 18:38:54 +0100 Subject: [PATCH 02/10] add tasks cleanup --- internal/tasks/tasks.go | 55 +++++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 8 deletions(-) diff --git a/internal/tasks/tasks.go b/internal/tasks/tasks.go index 6e211db5cc..9bc360859e 100644 --- a/internal/tasks/tasks.go +++ b/internal/tasks/tasks.go @@ -8,11 +8,15 @@ import ( "github.com/briandowns/spinner" ) -type Task func(interface{}) (interface{}, error) +type Task func(args interface{}) (nextArgs interface{}, err error) +type TaskWithCleanup func(args interface{}) (nextArgs interface{}, cleanUpArgs interface{}, err error) +type CleanUpTask func(cleanUpArgs interface{}) error type taskInfo struct { - Name string - function Task + Name string + function TaskWithCleanup + cleanFunction CleanUpTask + cleanUpArgs interface{} } type Tasks struct { @@ -25,25 +29,60 @@ func Begin() *Tasks { func (ts *Tasks) Add(name string, task Task) { ts.tasks = append(ts.tasks, taskInfo{ - Name: name, - function: task, + Name: name, + function: func(i interface{}) (passedData interface{}, cleanUpData interface{}, err error) { + passedData, err = task(i) + return + }, }) } +func (ts *Tasks) AddWithCleanUp(name string, task TaskWithCleanup, clean CleanUpTask) { + ts.tasks = append(ts.tasks, taskInfo{ + Name: name, + function: task, + cleanFunction: clean, + }) +} + +// Cleanup execute all tasks cleanup function before failed one in reverse order +func (ts *Tasks) Cleanup(failed int) { + totalTasks := len(ts.tasks) + + i := failed - 1 + for ; i >= 0; i -= 1 { + task := ts.tasks[i] + + if task.cleanFunction != nil { + fmt.Printf("[%d/%d] Cleaning task %q\n", i+1, totalTasks, task.Name) + + err := task.cleanFunction(task.cleanUpArgs) + if err != nil { + fmt.Printf("task %d failed to cleanup: %s", i+1, err.Error()) + } + } + } +} + func (ts *Tasks) Execute(data interface{}) (interface{}, error) { var err error totalTasks := len(ts.tasks) rand.Seed(time.Now().UnixNano()) spin := spinner.New(spinner.CharSets[rand.Int()%37], 100*time.Millisecond) - for i, task := range ts.tasks { + for i := range ts.tasks { + task := &ts.tasks[i] fmt.Printf("[%d/%d] %s\n", i+1, totalTasks, task.Name) spin.Start() - data, err = task.function(data) + + data, task.cleanUpArgs, err = task.function(data) if err != nil { spin.Stop() - return nil, fmt.Errorf("task [%d/%d] failed: %w", i, totalTasks, err) + fmt.Println("task failed, cleaning up created resources") + ts.Cleanup(i) + return nil, fmt.Errorf("task %d %q failed: %w", i+1, task.Name, err) } + spin.Stop() } From 069771801c4451e54a0b5294e289c49d5f5cb3cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jules=20Cast=C3=A9ran?= Date: Sat, 19 Nov 2022 19:10:46 +0100 Subject: [PATCH 03/10] add generics to check task and cleanup function matching --- internal/tasks/tasks.go | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/internal/tasks/tasks.go b/internal/tasks/tasks.go index 9bc360859e..a5f5e7bf28 100644 --- a/internal/tasks/tasks.go +++ b/internal/tasks/tasks.go @@ -2,21 +2,20 @@ package tasks import ( "fmt" - "math/rand" "time" "github.com/briandowns/spinner" ) type Task func(args interface{}) (nextArgs interface{}, err error) -type TaskWithCleanup func(args interface{}) (nextArgs interface{}, cleanUpArgs interface{}, err error) -type CleanUpTask func(cleanUpArgs interface{}) error +type TaskWithCleanup[T any] func(args interface{}) (nextArgs interface{}, cleanupArgs T, err error) +type Cleanup[T any] func(cleanupArgs T) error type taskInfo struct { Name string - function TaskWithCleanup - cleanFunction CleanUpTask - cleanUpArgs interface{} + function TaskWithCleanup[any] + cleanFunction Cleanup[any] + cleanupArgs interface{} } type Tasks struct { @@ -27,6 +26,7 @@ func Begin() *Tasks { return &Tasks{} } +// Add a task that does not need cleanup func (ts *Tasks) Add(name string, task Task) { ts.tasks = append(ts.tasks, taskInfo{ Name: name, @@ -37,11 +37,16 @@ func (ts *Tasks) Add(name string, task Task) { }) } -func (ts *Tasks) AddWithCleanUp(name string, task TaskWithCleanup, clean CleanUpTask) { +// AddWithCleanUp adds a task to the list with a cleanup function in case of fail during tasks execution +func AddWithCleanUp[T any](ts *Tasks, name string, task TaskWithCleanup[T], clean Cleanup[T]) { ts.tasks = append(ts.tasks, taskInfo{ - Name: name, - function: task, - cleanFunction: clean, + Name: name, + function: func(args interface{}) (nextArgs interface{}, cleanUpArgs any, err error) { + return task(args) + }, + cleanFunction: func(cleanupArgs any) error { + return clean(cleanupArgs.(T)) + }, }) } @@ -50,13 +55,13 @@ func (ts *Tasks) Cleanup(failed int) { totalTasks := len(ts.tasks) i := failed - 1 - for ; i >= 0; i -= 1 { + for ; i >= 0; i-- { task := ts.tasks[i] if task.cleanFunction != nil { fmt.Printf("[%d/%d] Cleaning task %q\n", i+1, totalTasks, task.Name) - err := task.cleanFunction(task.cleanUpArgs) + err := task.cleanFunction(task.cleanupArgs) if err != nil { fmt.Printf("task %d failed to cleanup: %s", i+1, err.Error()) } @@ -64,18 +69,18 @@ func (ts *Tasks) Cleanup(failed int) { } } +// Execute tasks with interactive display and cleanup on fail func (ts *Tasks) Execute(data interface{}) (interface{}, error) { var err error totalTasks := len(ts.tasks) - rand.Seed(time.Now().UnixNano()) - spin := spinner.New(spinner.CharSets[rand.Int()%37], 100*time.Millisecond) + spin := spinner.New(spinner.CharSets[11], 100*time.Millisecond) for i := range ts.tasks { task := &ts.tasks[i] fmt.Printf("[%d/%d] %s\n", i+1, totalTasks, task.Name) spin.Start() - data, task.cleanUpArgs, err = task.function(data) + data, task.cleanupArgs, err = task.function(data) if err != nil { spin.Stop() fmt.Println("task failed, cleaning up created resources") From 0d169746caf49919ef6bf48eb878f626ba06434e Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Mon, 21 Nov 2022 11:08:16 +0100 Subject: [PATCH 04/10] add tests and cleanup when context stop --- internal/tasks/tasks.go | 12 ++++++- internal/tasks/tasks_test.go | 68 ++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 internal/tasks/tasks_test.go diff --git a/internal/tasks/tasks.go b/internal/tasks/tasks.go index a5f5e7bf28..f211ed2e76 100644 --- a/internal/tasks/tasks.go +++ b/internal/tasks/tasks.go @@ -1,6 +1,7 @@ package tasks import ( + "context" "fmt" "time" @@ -70,7 +71,7 @@ func (ts *Tasks) Cleanup(failed int) { } // Execute tasks with interactive display and cleanup on fail -func (ts *Tasks) Execute(data interface{}) (interface{}, error) { +func (ts *Tasks) Execute(ctx context.Context, data interface{}) (interface{}, error) { var err error totalTasks := len(ts.tasks) spin := spinner.New(spinner.CharSets[11], 100*time.Millisecond) @@ -88,6 +89,15 @@ func (ts *Tasks) Execute(data interface{}) (interface{}, error) { return nil, fmt.Errorf("task %d %q failed: %w", i+1, task.Name, err) } + select { + case <-ctx.Done(): + spin.Stop() + fmt.Println("context canceled, cleaning up created resources") + ts.Cleanup(i + 1) + return nil, fmt.Errorf("task %d %q failed: context canceled", i+1, task.Name) + default: + } + spin.Stop() } diff --git a/internal/tasks/tasks_test.go b/internal/tasks/tasks_test.go new file mode 100644 index 0000000000..0fd2ef7387 --- /dev/null +++ b/internal/tasks/tasks_test.go @@ -0,0 +1,68 @@ +package tasks_test + +import ( + "context" + "fmt" + "testing" + + "github.com/alecthomas/assert" + "github.com/scaleway/scaleway-cli/v2/internal/tasks" +) + +func TestCleanup(t *testing.T) { + ts := tasks.Begin() + + clean := 0 + + tasks.AddWithCleanUp(ts, "Task 1", func(_ interface{}) (interface{}, string, error) { + return nil, "", nil + }, func(string) error { + clean += 1 + return nil + }) + tasks.AddWithCleanUp(ts, "Task 2", func(_ interface{}) (interface{}, string, error) { + return nil, "", nil + }, func(string) error { + clean += 1 + return nil + }) + tasks.AddWithCleanUp(ts, "Task 3", func(_ interface{}) (interface{}, string, error) { + return nil, "", fmt.Errorf("fail") + }, func(string) error { + clean += 1 + return nil + }) + _, err := ts.Execute(context.Background(), nil) + assert.NotNil(t, err, "Execute should return error after cleanup") + assert.Equal(t, clean, 2, "2 task cleanup should have been executed") +} + +func TestCleanupOnContext(t *testing.T) { + ts := tasks.Begin() + + clean := 0 + ctx, cancel := context.WithCancel(context.Background()) + + tasks.AddWithCleanUp(ts, "Task 1", func(_ interface{}) (interface{}, string, error) { + return nil, "", nil + }, func(string) error { + clean += 1 + return nil + }) + tasks.AddWithCleanUp(ts, "Task 2", func(_ interface{}) (interface{}, string, error) { + return nil, "", nil + }, func(string) error { + clean += 1 + return nil + }) + tasks.AddWithCleanUp(ts, "Task 3", func(_ interface{}) (interface{}, string, error) { + cancel() + return nil, "", nil + }, func(string) error { + clean += 1 + return nil + }) + _, err := ts.Execute(ctx, nil) + assert.NotNil(t, err) + assert.Equal(t, clean, 3, "3 task cleanup should have been executed") +} From 8eeca2930cabe4de8023b4bd269708c293df6093 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 9 Mar 2023 13:51:46 +0100 Subject: [PATCH 05/10] go mod tidy --- go.sum | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/go.sum b/go.sum index d89982ecff..e38bd63442 100644 --- a/go.sum +++ b/go.sum @@ -4,6 +4,8 @@ github.com/alecthomas/colour v0.1.0 h1:nOE9rJm6dsZ66RGWYSFrXw461ZIt9A6+nHgL7FRrD github.com/alecthomas/colour v0.1.0/go.mod h1:QO9JBoKquHd+jz9nshCh40fOfO+JzsoXy8qTHF68zU0= github.com/alecthomas/repr v0.0.0-20210801044451-80ca428c5142 h1:8Uy0oSf5co/NZXje7U1z8Mpep++QJOldL2hs/sBQf48= github.com/alecthomas/repr v0.0.0-20210801044451-80ca428c5142/go.mod h1:2kn6fqh/zIyPLmm3ugklbEi5hg5wS435eygvNfaDQL8= +github.com/briandowns/spinner v1.19.0 h1:s8aq38H+Qju89yhp89b4iIiMzMm8YN3p6vGpwyh/a8E= +github.com/briandowns/spinner v1.19.0/go.mod h1:mQak9GHqbspjC/5iUx3qMlIho8xBS/ppAL/hX5SmPJU= github.com/c-bata/go-prompt v0.2.5 h1:3zg6PecEywxNn0xiqcXHD96fkbxghD+gdB2tbsYfl+Y= github.com/c-bata/go-prompt v0.2.5/go.mod h1:vFnjEGDIIA/Lib7giyE4E9c50Lvl8j0S+7FVlAwDAVw= github.com/certifi/gocertifi v0.0.0-20210507211836-431795d63e8d h1:S2NE3iHSwP0XV47EEXL8mWmRdEfGscSJ+7EgePNgt0s= @@ -26,6 +28,7 @@ github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkp github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= github.com/etdub/goparsetime v0.0.0-20160315173935-ea17b0ac3318 h1:iguwbR+9xsizl84VMHU47I4OOWYSex1HZRotEoqziWQ= github.com/etdub/goparsetime v0.0.0-20160315173935-ea17b0ac3318/go.mod h1:O/QFFckzvu1KpS1AOuQGgi6ErznEF8nZZVNDDMXlDP4= +github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= github.com/fatih/color v1.14.1 h1:qfhVLaG5s+nCROl1zJsZRxFeYrHLqWroPOQ8BWiNb4w= github.com/fatih/color v1.14.1/go.mod h1:2oHN61fhTpgcxD3TSWCgKDiH1+x4OiDVVGH8WlgGZGg= github.com/getsentry/raven-go v0.2.0 h1:no+xWJRb5ZI7eE8TWgIq1jLulQiIoLG0IfYxv5JYMGs= @@ -47,6 +50,7 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kubernetes-client/go-base v0.0.0-20190205182333-3d0e39759d98 h1:ZMIkOkl/Bg5H4EJI7zbjVXAo4rV0QJOGz2U5A0xUmZU= github.com/kubernetes-client/go-base v0.0.0-20190205182333-3d0e39759d98/go.mod h1:HPlr4uJEfrxar3JUY9cmXs3oooPjTLO6nEaEAIt5LI8= +github.com/mattn/go-colorable v0.1.2/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE= github.com/mattn/go-colorable v0.1.4/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE= github.com/mattn/go-colorable v0.1.7/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc= github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= From 8ecf3acee2bc8cf9692b912262bcc8ae4133f82b Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 9 Mar 2023 14:35:42 +0100 Subject: [PATCH 06/10] add context cancelling to tasks --- internal/tasks/loader.go | 26 +++++++++++++ internal/tasks/tasks.go | 73 ++++++++++++++++++++++++------------ internal/tasks/tasks_test.go | 63 +++++++++++++++++-------------- 3 files changed, 110 insertions(+), 52 deletions(-) create mode 100644 internal/tasks/loader.go diff --git a/internal/tasks/loader.go b/internal/tasks/loader.go new file mode 100644 index 0000000000..1d5968ea64 --- /dev/null +++ b/internal/tasks/loader.go @@ -0,0 +1,26 @@ +package tasks + +import ( + "time" + + "github.com/briandowns/spinner" +) + +// Loader will print loading activity to the terminal +type Loader struct { + spinner *spinner.Spinner +} + +func setupLoader() *Loader { + return &Loader{ + spinner.New(spinner.CharSets[11], 100*time.Millisecond), + } +} + +func (l *Loader) Start() { + l.spinner.Start() +} + +func (l *Loader) Stop() { + l.spinner.Stop() +} diff --git a/internal/tasks/tasks.go b/internal/tasks/tasks.go index f211ed2e76..b15a93cabc 100644 --- a/internal/tasks/tasks.go +++ b/internal/tasks/tasks.go @@ -3,14 +3,13 @@ package tasks import ( "context" "fmt" - "time" - - "github.com/briandowns/spinner" + "os" + "os/signal" ) -type Task func(args interface{}) (nextArgs interface{}, err error) -type TaskWithCleanup[T any] func(args interface{}) (nextArgs interface{}, cleanupArgs T, err error) -type Cleanup[T any] func(cleanupArgs T) error +type Task func(ctx context.Context, args interface{}) (nextArgs interface{}, err error) +type TaskWithCleanup[T any] func(ctx context.Context, args interface{}) (nextArgs interface{}, cleanupArgs T, err error) +type Cleanup[T any] func(ctx context.Context, cleanupArgs T) error type taskInfo struct { Name string @@ -31,8 +30,8 @@ func Begin() *Tasks { func (ts *Tasks) Add(name string, task Task) { ts.tasks = append(ts.tasks, taskInfo{ Name: name, - function: func(i interface{}) (passedData interface{}, cleanUpData interface{}, err error) { - passedData, err = task(i) + function: func(ctx context.Context, i interface{}) (passedData interface{}, cleanUpData interface{}, err error) { + passedData, err = task(ctx, i) return }, }) @@ -42,18 +41,26 @@ func (ts *Tasks) Add(name string, task Task) { func AddWithCleanUp[T any](ts *Tasks, name string, task TaskWithCleanup[T], clean Cleanup[T]) { ts.tasks = append(ts.tasks, taskInfo{ Name: name, - function: func(args interface{}) (nextArgs interface{}, cleanUpArgs any, err error) { - return task(args) + function: func(ctx context.Context, args interface{}) (nextArgs interface{}, cleanUpArgs any, err error) { + return task(ctx, args) }, - cleanFunction: func(cleanupArgs any) error { - return clean(cleanupArgs.(T)) + cleanFunction: func(ctx context.Context, cleanupArgs any) error { + return clean(ctx, cleanupArgs.(T)) }, }) } +// setupContext return a contextWithCancel that will cancel on os interrupt (Ctrl-C) +func setupContext(ctx context.Context) (context.Context, func()) { + return signal.NotifyContext(ctx, os.Interrupt) +} + // Cleanup execute all tasks cleanup function before failed one in reverse order -func (ts *Tasks) Cleanup(failed int) { +func (ts *Tasks) Cleanup(ctx context.Context, failed int) { totalTasks := len(ts.tasks) + loader := setupLoader() + cancelableCtx, cleanCtx := setupContext(ctx) + defer cleanCtx() i := failed - 1 for ; i >= 0; i-- { @@ -61,10 +68,19 @@ func (ts *Tasks) Cleanup(failed int) { if task.cleanFunction != nil { fmt.Printf("[%d/%d] Cleaning task %q\n", i+1, totalTasks, task.Name) + loader.Start() - err := task.cleanFunction(task.cleanupArgs) + err := task.cleanFunction(cancelableCtx, task.cleanupArgs) if err != nil { - fmt.Printf("task %d failed to cleanup: %s", i+1, err.Error()) + fmt.Printf("task %d failed to cleanup: %s\n", i+1, err.Error()) + } + loader.Stop() + + select { + case <-cancelableCtx.Done(): + fmt.Println("cleanup has been cancelled") + return + default: } } } @@ -74,31 +90,40 @@ func (ts *Tasks) Cleanup(failed int) { func (ts *Tasks) Execute(ctx context.Context, data interface{}) (interface{}, error) { var err error totalTasks := len(ts.tasks) - spin := spinner.New(spinner.CharSets[11], 100*time.Millisecond) + loader := setupLoader() + + cancelableCtx, cleanCtx := setupContext(ctx) + defer cleanCtx() for i := range ts.tasks { task := &ts.tasks[i] fmt.Printf("[%d/%d] %s\n", i+1, totalTasks, task.Name) - spin.Start() + loader.Start() - data, task.cleanupArgs, err = task.function(data) - if err != nil { - spin.Stop() + data, task.cleanupArgs, err = task.function(cancelableCtx, data) + taskIsCancelled := false + select { + case <-cancelableCtx.Done(): + taskIsCancelled = true + default: + } + if err != nil || taskIsCancelled { + loader.Stop() fmt.Println("task failed, cleaning up created resources") - ts.Cleanup(i) + ts.Cleanup(ctx, i) return nil, fmt.Errorf("task %d %q failed: %w", i+1, task.Name, err) } select { case <-ctx.Done(): - spin.Stop() + loader.Stop() fmt.Println("context canceled, cleaning up created resources") - ts.Cleanup(i + 1) + ts.Cleanup(ctx, i+1) return nil, fmt.Errorf("task %d %q failed: context canceled", i+1, task.Name) default: } - spin.Stop() + loader.Stop() } return data, nil diff --git a/internal/tasks/tasks_test.go b/internal/tasks/tasks_test.go index 0fd2ef7387..1d1f1615dc 100644 --- a/internal/tasks/tasks_test.go +++ b/internal/tasks/tasks_test.go @@ -14,22 +14,22 @@ func TestCleanup(t *testing.T) { clean := 0 - tasks.AddWithCleanUp(ts, "Task 1", func(_ interface{}) (interface{}, string, error) { + tasks.AddWithCleanUp(ts, "Task 1", func(context.Context, interface{}) (interface{}, string, error) { return nil, "", nil - }, func(string) error { - clean += 1 + }, func(context.Context, string) error { + clean++ return nil }) - tasks.AddWithCleanUp(ts, "Task 2", func(_ interface{}) (interface{}, string, error) { + tasks.AddWithCleanUp(ts, "Task 2", func(context.Context, interface{}) (interface{}, string, error) { return nil, "", nil - }, func(string) error { - clean += 1 + }, func(context.Context, string) error { + clean++ return nil }) - tasks.AddWithCleanUp(ts, "Task 3", func(_ interface{}) (interface{}, string, error) { + tasks.AddWithCleanUp(ts, "Task 3", func(context.Context, interface{}) (interface{}, string, error) { return nil, "", fmt.Errorf("fail") - }, func(string) error { - clean += 1 + }, func(context.Context, string) error { + clean++ return nil }) _, err := ts.Execute(context.Background(), nil) @@ -43,25 +43,32 @@ func TestCleanupOnContext(t *testing.T) { clean := 0 ctx, cancel := context.WithCancel(context.Background()) - tasks.AddWithCleanUp(ts, "Task 1", func(_ interface{}) (interface{}, string, error) { - return nil, "", nil - }, func(string) error { - clean += 1 - return nil - }) - tasks.AddWithCleanUp(ts, "Task 2", func(_ interface{}) (interface{}, string, error) { - return nil, "", nil - }, func(string) error { - clean += 1 - return nil - }) - tasks.AddWithCleanUp(ts, "Task 3", func(_ interface{}) (interface{}, string, error) { - cancel() - return nil, "", nil - }, func(string) error { - clean += 1 - return nil - }) + tasks.AddWithCleanUp(ts, "Task 1", + func(context.Context, interface{}) (interface{}, string, error) { + return nil, "", nil + }, func(context.Context, string) error { + clean++ + return nil + }, + ) + tasks.AddWithCleanUp(ts, "Task 2", + func(context.Context, interface{}) (interface{}, string, error) { + return nil, "", nil + }, func(context.Context, string) error { + clean++ + return nil + }, + ) + tasks.AddWithCleanUp(ts, "Task 3", + func(context.Context, interface{}) (interface{}, string, error) { + cancel() + return nil, "", nil + }, func(context.Context, string) error { + clean++ + return nil + }, + ) + _, err := ts.Execute(ctx, nil) assert.NotNil(t, err) assert.Equal(t, clean, 3, "3 task cleanup should have been executed") From ae2c13e0fa68d7036397e46bee6e930e0b735430 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 9 Mar 2023 14:37:56 +0100 Subject: [PATCH 07/10] move clean cancel check --- internal/tasks/tasks.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/tasks/tasks.go b/internal/tasks/tasks.go index b15a93cabc..b71c1d0102 100644 --- a/internal/tasks/tasks.go +++ b/internal/tasks/tasks.go @@ -75,13 +75,13 @@ func (ts *Tasks) Cleanup(ctx context.Context, failed int) { fmt.Printf("task %d failed to cleanup: %s\n", i+1, err.Error()) } loader.Stop() - - select { - case <-cancelableCtx.Done(): - fmt.Println("cleanup has been cancelled") - return - default: - } + } + + select { + case <-cancelableCtx.Done(): + fmt.Println("cleanup has been cancelled") + return + default: } } } From d49fe5607be410a8c6ce6e5c43663826a30dd5eb Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 9 Mar 2023 14:41:31 +0100 Subject: [PATCH 08/10] move clean cancel check before cleaning --- internal/tasks/tasks.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/tasks/tasks.go b/internal/tasks/tasks.go index b71c1d0102..3590101800 100644 --- a/internal/tasks/tasks.go +++ b/internal/tasks/tasks.go @@ -66,23 +66,23 @@ func (ts *Tasks) Cleanup(ctx context.Context, failed int) { for ; i >= 0; i-- { task := ts.tasks[i] + select { + case <-cancelableCtx.Done(): + fmt.Println("cleanup has been cancelled, there may be dangling resources") + return + default: + } + if task.cleanFunction != nil { fmt.Printf("[%d/%d] Cleaning task %q\n", i+1, totalTasks, task.Name) loader.Start() err := task.cleanFunction(cancelableCtx, task.cleanupArgs) if err != nil { - fmt.Printf("task %d failed to cleanup: %s\n", i+1, err.Error()) + fmt.Printf("task %d failed to cleanup, there may be dangling resources: %s\n", i+1, err.Error()) } loader.Stop() } - - select { - case <-cancelableCtx.Done(): - fmt.Println("cleanup has been cancelled") - return - default: - } } } From 128eafe27b796169ab9468ac77c1ddd0b3b57088 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 9 Mar 2023 14:50:44 +0100 Subject: [PATCH 09/10] update tests --- internal/tasks/tasks_test.go | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/internal/tasks/tasks_test.go b/internal/tasks/tasks_test.go index 1d1f1615dc..384f3bf0f5 100644 --- a/internal/tasks/tasks_test.go +++ b/internal/tasks/tasks_test.go @@ -3,7 +3,9 @@ package tasks_test import ( "context" "fmt" + "os" "testing" + "time" "github.com/alecthomas/assert" "github.com/scaleway/scaleway-cli/v2/internal/tasks" @@ -41,7 +43,7 @@ func TestCleanupOnContext(t *testing.T) { ts := tasks.Begin() clean := 0 - ctx, cancel := context.WithCancel(context.Background()) + ctx := context.Background() tasks.AddWithCleanUp(ts, "Task 1", func(context.Context, interface{}) (interface{}, string, error) { @@ -60,9 +62,24 @@ func TestCleanupOnContext(t *testing.T) { }, ) tasks.AddWithCleanUp(ts, "Task 3", - func(context.Context, interface{}) (interface{}, string, error) { - cancel() - return nil, "", nil + func(ctx context.Context, _ interface{}) (interface{}, string, error) { + p, err := os.FindProcess(os.Getpid()) + if err != nil { + return nil, "", err + } + + // Interrupt tasks, as done with Ctrl-C + err = p.Signal(os.Interrupt) + if err != nil { + t.Fatal(err) + } + + select { + case <-time.After(time.Second): + return nil, "", nil + case <-ctx.Done(): + return nil, "", fmt.Errorf("interrupted") + } }, func(context.Context, string) error { clean++ return nil @@ -70,6 +87,6 @@ func TestCleanupOnContext(t *testing.T) { ) _, err := ts.Execute(ctx, nil) - assert.NotNil(t, err) - assert.Equal(t, clean, 3, "3 task cleanup should have been executed") + assert.NotNil(t, err, "context should have been interrupted") + assert.Equal(t, clean, 2, "2 task cleanup should have been executed") } From 6679e1f81eb58f1767072cffe12a6105ba5aae06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jules=20Cast=C3=A9ran?= Date: Thu, 9 Mar 2023 17:14:43 +0100 Subject: [PATCH 10/10] skip signal test on windows --- internal/tasks/tasks_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/tasks/tasks_test.go b/internal/tasks/tasks_test.go index 384f3bf0f5..b4912d6f85 100644 --- a/internal/tasks/tasks_test.go +++ b/internal/tasks/tasks_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "runtime" "testing" "time" @@ -40,6 +41,9 @@ func TestCleanup(t *testing.T) { } func TestCleanupOnContext(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Cannot send signal on windows") + } ts := tasks.Begin() clean := 0