From 5044102d9dd70b0c27f07d282110fa3cc1251e9b Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 16 Mar 2023 18:49:24 +0100 Subject: [PATCH 01/37] feat: add editor --- internal/config/editor.go | 25 ++++++ internal/editor/arg_mapper.go | 60 +++++++++++++++ internal/editor/arg_mapper_test.go | 70 +++++++++++++++++ internal/editor/editor.go | 91 ++++++++++++++++++++++ internal/editor/editor_test.go | 115 ++++++++++++++++++++++++++++ internal/editor/interceptor.go | 27 +++++++ internal/editor/interceptor_test.go | 24 ++++++ 7 files changed, 412 insertions(+) create mode 100644 internal/config/editor.go create mode 100644 internal/editor/arg_mapper.go create mode 100644 internal/editor/arg_mapper_test.go create mode 100644 internal/editor/editor.go create mode 100644 internal/editor/editor_test.go create mode 100644 internal/editor/interceptor.go create mode 100644 internal/editor/interceptor_test.go diff --git a/internal/config/editor.go b/internal/config/editor.go new file mode 100644 index 0000000000..916055e991 --- /dev/null +++ b/internal/config/editor.go @@ -0,0 +1,25 @@ +package config + +import "os" + +var ( + // List of env variables where to find the editor to use + // Order in slice is override order, the latest will override the first ones + editorEnvVariables = []string{"EDITOR", "VISUAL"} +) + +func GetDefaultEditor() string { + editor := "" + for _, envVar := range editorEnvVariables { + tmp := os.Getenv(envVar) + if tmp != "" { + editor = tmp + } + } + + if editor == "" { + return "vi" + } + + return editor +} diff --git a/internal/editor/arg_mapper.go b/internal/editor/arg_mapper.go new file mode 100644 index 0000000000..931cb48032 --- /dev/null +++ b/internal/editor/arg_mapper.go @@ -0,0 +1,60 @@ +package editor + +import ( + "reflect" +) + +func tryElem(v reflect.Value) reflect.Value { + if v.Kind() == reflect.Pointer { + return v.Elem() + } + return v +} + +func areSameType(v1 reflect.Value, v2 reflect.Value) bool { + v1t := v1.Type() + v2t := v2.Type() + + if v1t == v2t { + return true + } + if v1t.Kind() == reflect.Pointer { + v1t = v1t.Elem() + } + if v2t.Kind() == reflect.Pointer { + v2t = v2t.Elem() + } + + return v1t == v2t +} + +// valueMapper get all fields present both in src and dest and set them in dest +// if argument is not zero-value in dest, it is not set +func valueMapper(src reflect.Value, dest reflect.Value) interface{} { + if dest.Kind() == reflect.Pointer { + dest = dest.Elem() + } + if src.Kind() == reflect.Pointer { + src = src.Elem() + } + + for i := 0; i < dest.NumField(); i++ { + destField := dest.Field(i) + fieldType := dest.Type().Field(i) + srcField := src.FieldByName(fieldType.Name) + + if !srcField.IsValid() || srcField.IsZero() || !areSameType(srcField, destField) { + continue + } + + // If dest is a nil pointer (*string) and src is not, allocate a pointer for dest + if destField.Kind() == reflect.Pointer && srcField.Kind() != reflect.Pointer && destField.IsZero() { + destField.Set(reflect.New(srcField.Type())) + destField = destField.Elem() + } + + destField.Set(srcField) + } + + return dest +} diff --git a/internal/editor/arg_mapper_test.go b/internal/editor/arg_mapper_test.go new file mode 100644 index 0000000000..bf4530563e --- /dev/null +++ b/internal/editor/arg_mapper_test.go @@ -0,0 +1,70 @@ +package editor + +import ( + "reflect" + "testing" + + "github.com/alecthomas/assert" +) + +func Test_valueMapper(t *testing.T) { + src := struct { + Arg1 string + Arg2 int + }{"1", 1} + dest := struct { + Arg1 string + Arg2 int + }{} + + valueMapper(reflect.ValueOf(&src), reflect.ValueOf(&dest)) + assert.Equal(t, src.Arg1, dest.Arg1) + assert.Equal(t, src.Arg2, dest.Arg2) +} + +func Test_valueMapperInvalidType(t *testing.T) { + src := struct { + Arg1 string + Arg2 int + }{"1", 1} + dest := struct { + Arg1 string + Arg2 bool + }{} + + valueMapper(reflect.ValueOf(&src), reflect.ValueOf(&dest)) + assert.Equal(t, src.Arg1, dest.Arg1) + assert.Zero(t, dest.Arg2) +} + +func Test_valueMapperDifferentFields(t *testing.T) { + src := struct { + Arg1 string + Arg2 int + }{"1", 1} + dest := struct { + Arg3 string + Arg4 bool + }{} + + valueMapper(reflect.ValueOf(&src), reflect.ValueOf(&dest)) + assert.Zero(t, dest.Arg3) + assert.Zero(t, dest.Arg4) +} + +func Test_valueMapperPointers(t *testing.T) { + src := struct { + Arg1 string + Arg2 int + }{"1", 1} + dest := struct { + Arg1 *string + Arg2 *int + }{} + + valueMapper(reflect.ValueOf(&src), reflect.ValueOf(&dest)) + assert.NotNil(t, dest.Arg1) + assert.EqualValues(t, src.Arg1, *dest.Arg1) + assert.NotNil(t, dest.Arg2) + assert.EqualValues(t, src.Arg2, *dest.Arg2) +} diff --git a/internal/editor/editor.go b/internal/editor/editor.go new file mode 100644 index 0000000000..8dd2645760 --- /dev/null +++ b/internal/editor/editor.go @@ -0,0 +1,91 @@ +package editor + +import ( + "bytes" + "encoding/json" + "fmt" + "os/exec" + "reflect" + + "github.com/scaleway/scaleway-cli/v2/internal/config" +) + +var SkipEditor = false + +func edit(content []byte) ([]byte, error) { + if SkipEditor == true { + return content, nil + } + editionBuffer := bytes.NewBuffer(nil) + + defaultEditor := config.GetDefaultEditor() + cmd := exec.Command(defaultEditor) + cmd.Stdin = bytes.NewBuffer(content) + cmd.Stdout = editionBuffer + + err := cmd.Run() + if err != nil { + return nil, fmt.Errorf("failed to edit temporary file: %w", err) + } + + return editionBuffer.Bytes(), nil +} + +// editor takes a complete resource and a partial resourceUpdate +func editor(resource interface{}, resourceUpdate interface{}, editedJson ...string) (interface{}, error) { + resourceV := reflect.ValueOf(resource) + resourceUpdateV := reflect.ValueOf(resourceUpdate) + + // Create a new resourceUpdate that will be edited + // It will allow user to edit it, then we will extract diff to perform update + resourceUpdateToEdit := reflect.New(resourceUpdateV.Type().Elem()) + valueMapper(resourceUpdateV, resourceUpdateToEdit) + valueMapper(resourceV, resourceUpdateToEdit) + + tmpArgsJson, err := json.MarshalIndent(resourceUpdateToEdit.Interface(), "", " ") + if err != nil { + return nil, fmt.Errorf("failed to convert update request to json: %w", err) + } + + tmpArgsJson, err = edit(tmpArgsJson) + if err != nil { + return nil, fmt.Errorf("failed to edit json: %w", err) + } + + if len(editedJson) == 1 { + tmpArgsJson = []byte(editedJson[0]) + } + + // Create a new resourceUpdate as destination for edited one + resourceUpdateEdited := reflect.New(resourceUpdateV.Type().Elem()) + + err = json.Unmarshal(tmpArgsJson, resourceUpdateEdited.Interface()) + if err != nil { + return nil, fmt.Errorf("failed to marshal edited data: %w", err) + } + + return resourceUpdateEdited.Interface(), nil +} + +// Editor takes a partial UpdateResourceRequest, the type of the GetResourceRequest and a function that return the resource using GetResourceRequest +func Editor(resourceUpdate interface{}, resourceGetType reflect.Type, resourceGetter func(interface{}) (interface{}, error)) (interface{}, error) { + resourceUpdateV := reflect.ValueOf(resourceUpdate) + + resourceGetterArg := reflect.New(resourceGetType).Interface() + resourceGetterArgV := reflect.ValueOf(resourceGetterArg) + + // Fill Getter args using Update arg content + valueMapper(resourceUpdateV, resourceGetterArgV) + + resource, err := resourceGetter(resourceGetterArg) + if err != nil { + return nil, fmt.Errorf("failed to get resource: %w", err) + } + + editedArgs, err := editor(resource, resourceUpdateV.Interface()) + if err != nil { + return nil, fmt.Errorf("failed to edit update arguments: %w", err) + } + + return editedArgs, nil +} diff --git a/internal/editor/editor_test.go b/internal/editor/editor_test.go new file mode 100644 index 0000000000..b13737624b --- /dev/null +++ b/internal/editor/editor_test.go @@ -0,0 +1,115 @@ +package editor + +import ( + "log" + "reflect" + "testing" + + "github.com/alecthomas/assert" +) + +func Test_editor(t *testing.T) { + SkipEditor = true + + resource := &struct { + ID string + Name string + }{ + "uuid", + "name", + } + resourceUpdate := &struct { + ID string + Name string + }{ + "uuid", + "", + } + + _, err := editor(resource, resourceUpdate) + assert.Nil(t, err) +} + +func Test_editor_pointers(t *testing.T) { + SkipEditor = true + + type ResourceUpdate struct { + ID string + Name *string + } + resource := &struct { + ID string + Name string + }{ + "uuid", + "name", + } + + resourceUpdate := &ResourceUpdate{ + "uuid", + nil, + } + + editedResourceUpdateI, err := editor(resource, resourceUpdate) + assert.Nil(t, err) + editedResourceUpdate := editedResourceUpdateI.(*ResourceUpdate) + assert.NotNil(t, editedResourceUpdate.Name) + assert.Equal(t, resource.Name, *editedResourceUpdate.Name) +} + +func Test_editor_map(t *testing.T) { + SkipEditor = true + + type ResourceUpdate struct { + ID string `json:"id"` + Env *map[string]string `json:"env"` + } + resource := &struct { + ID string `json:"id"` + Env map[string]string `json:"env"` + }{ + "uuid", + map[string]string{ + "foo": "bar", + }, + } + + resourceUpdate := &ResourceUpdate{ + "uuid", + nil, + } + + SkipEditor = true + editedResourceUpdateI, err := editor(resource, resourceUpdate, ` + {"id":"uuid", "env": {}} +`) + assert.Nil(t, err) + editedResourceUpdate := editedResourceUpdateI.(*ResourceUpdate) + assert.NotNil(t, editedResourceUpdate.Env) + assert.True(t, len(*editedResourceUpdate.Env) == 0) +} + +func TestEditor(t *testing.T) { + SkipEditor = true + + resource := &struct { + ID string + Name string + }{ + "uuid", + "name", + } + resourceUpdate := &struct { + ID string + Name string + }{ + "uuid", + "", + } + + editedResource, err := Editor(resourceUpdate, reflect.TypeOf(*resource), func(i interface{}) (interface{}, error) { + return resource, nil + }) + assert.Nil(t, err) + log.Println(editedResource) +} diff --git a/internal/editor/interceptor.go b/internal/editor/interceptor.go new file mode 100644 index 0000000000..63f5fedeea --- /dev/null +++ b/internal/editor/interceptor.go @@ -0,0 +1,27 @@ +package editor + +import ( + "context" + "fmt" + "reflect" + + "github.com/scaleway/scaleway-cli/v2/internal/core" +) + +func Interceptor(getter *core.Command) core.CommandInterceptor { + return func(ctx context.Context, argsI interface{}, runner core.CommandRunner) (interface{}, error) { + argsV := reflect.ValueOf(argsI) + + editedArgs, err := Editor(argsI, getter.ArgsType, func(i interface{}) (interface{}, error) { + return getter.Run(ctx, i) + }) + if err != nil { + return nil, fmt.Errorf("failed to edit args: %w", err) + } + + // TODO: only map diff + valueMapper(reflect.ValueOf(editedArgs), argsV) + + return runner(ctx, argsI) + } +} diff --git a/internal/editor/interceptor_test.go b/internal/editor/interceptor_test.go new file mode 100644 index 0000000000..cc76cfc550 --- /dev/null +++ b/internal/editor/interceptor_test.go @@ -0,0 +1,24 @@ +package editor_test + +import ( + "context" + "log" + "testing" + + "github.com/alecthomas/assert" + "github.com/scaleway/scaleway-cli/v2/internal/editor" + "github.com/scaleway/scaleway-cli/v2/internal/namespaces" + container "github.com/scaleway/scaleway-sdk-go/api/container/v1beta1" +) + +func TestInterceptor(t *testing.T) { + cmds := namespaces.GetCommands() + interceptor := editor.Interceptor(cmds.MustFind("container", "namespace", "get")) + ctx := context.Background() + res, err := interceptor(ctx, &container.UpdateNamespaceRequest{ + Region: "fr-par", + NamespaceID: "47c8f0ad-3567-451e-a0a5-820318678ce3", + }, cmds.MustFind("container", "namespace", "update").Run) + assert.Nil(t, err) + log.Println(res) +} From 98abe5b37ff518912777d5feb010f0d83bbb8964 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 17 Mar 2023 13:46:00 +0100 Subject: [PATCH 02/37] refactor and add custom marshaler --- internal/editor/editor.go | 57 +++++++++++-------- internal/editor/interceptor.go | 2 +- internal/editor/marshal.go | 37 ++++++++++++ internal/editor/{arg_mapper.go => reflect.go} | 2 +- .../{arg_mapper_test.go => reflect_test.go} | 8 +-- 5 files changed, 76 insertions(+), 30 deletions(-) create mode 100644 internal/editor/marshal.go rename internal/editor/{arg_mapper.go => reflect.go} (95%) rename internal/editor/{arg_mapper_test.go => reflect_test.go} (81%) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index 8dd2645760..b47f5b04e3 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -12,6 +12,8 @@ import ( var SkipEditor = false +type GetResourceFunc func(interface{}) (interface{}, error) + func edit(content []byte) ([]byte, error) { if SkipEditor == true { return content, nil @@ -32,57 +34,64 @@ func edit(content []byte) ([]byte, error) { } // editor takes a complete resource and a partial resourceUpdate -func editor(resource interface{}, resourceUpdate interface{}, editedJson ...string) (interface{}, error) { +func editor(resource interface{}, updateResourceRequest interface{}, editedJson ...string) (interface{}, error) { resourceV := reflect.ValueOf(resource) - resourceUpdateV := reflect.ValueOf(resourceUpdate) + updateResourceRequestV := reflect.ValueOf(updateResourceRequest) - // Create a new resourceUpdate that will be edited + // Create a new updateResourceRequest that will be edited // It will allow user to edit it, then we will extract diff to perform update - resourceUpdateToEdit := reflect.New(resourceUpdateV.Type().Elem()) - valueMapper(resourceUpdateV, resourceUpdateToEdit) - valueMapper(resourceV, resourceUpdateToEdit) + updateResourceRequestToEditV := reflect.New(updateResourceRequestV.Type().Elem()) + valueMapper(updateResourceRequestToEditV, updateResourceRequestV) + valueMapper(updateResourceRequestToEditV, resourceV) - tmpArgsJson, err := json.MarshalIndent(resourceUpdateToEdit.Interface(), "", " ") + // TODO: allow yaml marshal + updateResourceRequestJson, err := Marshal(updateResourceRequestToEditV.Interface(), MarshalModeJson) if err != nil { - return nil, fmt.Errorf("failed to convert update request to json: %w", err) + return nil, fmt.Errorf("failed to marshal update request: %w", err) } - tmpArgsJson, err = edit(tmpArgsJson) + // Start text editor to edit json + updateResourceRequestJson, err = edit(updateResourceRequestJson) if err != nil { - return nil, fmt.Errorf("failed to edit json: %w", err) + return nil, fmt.Errorf("failed to edit marshalled data: %w", err) } + // If editedJson is present, override edited json + // This is useful for testing purpose if len(editedJson) == 1 { - tmpArgsJson = []byte(editedJson[0]) + updateResourceRequestJson = []byte(editedJson[0]) } - // Create a new resourceUpdate as destination for edited one - resourceUpdateEdited := reflect.New(resourceUpdateV.Type().Elem()) + // Create a new updateResourceRequest as destination for edited one + updateResourceRequestEdited := reflect.New(updateResourceRequestV.Type().Elem()) - err = json.Unmarshal(tmpArgsJson, resourceUpdateEdited.Interface()) + err = Unmarshal(updateResourceRequestJson, updateResourceRequestEdited.Interface(), MarshalModeJson) if err != nil { - return nil, fmt.Errorf("failed to marshal edited data: %w", err) + return nil, fmt.Errorf("failed to unmarshal edited data: %w", err) } - return resourceUpdateEdited.Interface(), nil + return updateResourceRequestEdited.Interface(), nil } // Editor takes a partial UpdateResourceRequest, the type of the GetResourceRequest and a function that return the resource using GetResourceRequest -func Editor(resourceUpdate interface{}, resourceGetType reflect.Type, resourceGetter func(interface{}) (interface{}, error)) (interface{}, error) { - resourceUpdateV := reflect.ValueOf(resourceUpdate) +func Editor(updateResourceRequest interface{}, getResourceRequestType reflect.Type, getResource GetResourceFunc) (interface{}, error) { + updateResourceRequestV := reflect.ValueOf(updateResourceRequest) - resourceGetterArg := reflect.New(resourceGetType).Interface() - resourceGetterArgV := reflect.ValueOf(resourceGetterArg) + // Create GetResourceRequest to be able to fetch the actual resource + getResourceRequest := reflect.New(getResourceRequestType).Interface() + getResourceRequestV := reflect.ValueOf(getResourceRequest) - // Fill Getter args using Update arg content - valueMapper(resourceUpdateV, resourceGetterArgV) + // Fill GetResourceRequest args using Update arg content + // This should copy important argument like ResourceID + valueMapper(getResourceRequestV, updateResourceRequestV) - resource, err := resourceGetter(resourceGetterArg) + resource, err := getResource(getResourceRequest) if err != nil { return nil, fmt.Errorf("failed to get resource: %w", err) } - editedArgs, err := editor(resource, resourceUpdateV.Interface()) + // Start edition of UpdateResourceRequest + editedArgs, err := editor(resource, updateResourceRequest) if err != nil { return nil, fmt.Errorf("failed to edit update arguments: %w", err) } diff --git a/internal/editor/interceptor.go b/internal/editor/interceptor.go index 63f5fedeea..d96310f883 100644 --- a/internal/editor/interceptor.go +++ b/internal/editor/interceptor.go @@ -20,7 +20,7 @@ func Interceptor(getter *core.Command) core.CommandInterceptor { } // TODO: only map diff - valueMapper(reflect.ValueOf(editedArgs), argsV) + valueMapper(argsV, reflect.ValueOf(editedArgs)) return runner(ctx, argsI) } diff --git a/internal/editor/marshal.go b/internal/editor/marshal.go new file mode 100644 index 0000000000..edded4aab4 --- /dev/null +++ b/internal/editor/marshal.go @@ -0,0 +1,37 @@ +package editor + +import ( + "encoding/json" + "fmt" + + "github.com/ghodss/yaml" +) + +type MarshalMode string + +const ( + MarshalModeYaml = MarshalMode("yaml") + MarshalModeJson = MarshalMode("json") +) + +func Marshal(i interface{}, mode MarshalMode) ([]byte, error) { + switch mode { + case MarshalModeYaml: + return yaml.Marshal(i) + case MarshalModeJson: + return json.MarshalIndent(i, "", " ") + } + + return nil, fmt.Errorf("unknown marshal mode %q", mode) +} + +func Unmarshal(data []byte, i interface{}, mode MarshalMode) error { + switch mode { + case MarshalModeYaml: + return yaml.Unmarshal(data, i) + case MarshalModeJson: + return json.Unmarshal(data, i) + } + + return fmt.Errorf("unknown marshal mode %q", mode) +} diff --git a/internal/editor/arg_mapper.go b/internal/editor/reflect.go similarity index 95% rename from internal/editor/arg_mapper.go rename to internal/editor/reflect.go index 931cb48032..063150bebd 100644 --- a/internal/editor/arg_mapper.go +++ b/internal/editor/reflect.go @@ -30,7 +30,7 @@ func areSameType(v1 reflect.Value, v2 reflect.Value) bool { // valueMapper get all fields present both in src and dest and set them in dest // if argument is not zero-value in dest, it is not set -func valueMapper(src reflect.Value, dest reflect.Value) interface{} { +func valueMapper(dest reflect.Value, src reflect.Value) interface{} { if dest.Kind() == reflect.Pointer { dest = dest.Elem() } diff --git a/internal/editor/arg_mapper_test.go b/internal/editor/reflect_test.go similarity index 81% rename from internal/editor/arg_mapper_test.go rename to internal/editor/reflect_test.go index bf4530563e..867358dfd3 100644 --- a/internal/editor/arg_mapper_test.go +++ b/internal/editor/reflect_test.go @@ -17,7 +17,7 @@ func Test_valueMapper(t *testing.T) { Arg2 int }{} - valueMapper(reflect.ValueOf(&src), reflect.ValueOf(&dest)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.Equal(t, src.Arg1, dest.Arg1) assert.Equal(t, src.Arg2, dest.Arg2) } @@ -32,7 +32,7 @@ func Test_valueMapperInvalidType(t *testing.T) { Arg2 bool }{} - valueMapper(reflect.ValueOf(&src), reflect.ValueOf(&dest)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.Equal(t, src.Arg1, dest.Arg1) assert.Zero(t, dest.Arg2) } @@ -47,7 +47,7 @@ func Test_valueMapperDifferentFields(t *testing.T) { Arg4 bool }{} - valueMapper(reflect.ValueOf(&src), reflect.ValueOf(&dest)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.Zero(t, dest.Arg3) assert.Zero(t, dest.Arg4) } @@ -62,7 +62,7 @@ func Test_valueMapperPointers(t *testing.T) { Arg2 *int }{} - valueMapper(reflect.ValueOf(&src), reflect.ValueOf(&dest)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.NotNil(t, dest.Arg1) assert.EqualValues(t, src.Arg1, *dest.Arg1) assert.NotNil(t, dest.Arg2) From c9d353ae7a239a6039df8a1a54b990ddfc767558 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 17 Mar 2023 13:58:01 +0100 Subject: [PATCH 03/37] edit temporary file instead of using stdin and split arguments from editor path --- internal/editor/editor.go | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index b47f5b04e3..a51145a52e 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -1,11 +1,10 @@ package editor import ( - "bytes" - "encoding/json" "fmt" "os/exec" "reflect" + "strings" "github.com/scaleway/scaleway-cli/v2/internal/config" ) @@ -18,19 +17,31 @@ func edit(content []byte) ([]byte, error) { if SkipEditor == true { return content, nil } - editionBuffer := bytes.NewBuffer(nil) + + tmpFileName, err := createTemporaryFile(content) + if err != nil { + return nil, fmt.Errorf("failed to create temporary file: %w", err) + } defaultEditor := config.GetDefaultEditor() - cmd := exec.Command(defaultEditor) - cmd.Stdin = bytes.NewBuffer(content) - cmd.Stdout = editionBuffer + editorAndArguments := strings.Fields(defaultEditor) + args := []string{tmpFileName} + if len(editorAndArguments) > 1 { + args = append(editorAndArguments[1:], args...) + } + cmd := exec.Command(editorAndArguments[0], args...) + + err = cmd.Run() + if err != nil { + return nil, fmt.Errorf("failed to edit temporary file %q: %w", tmpFileName, err) + } - err := cmd.Run() + editedContent, err := readAndDeleteFile(tmpFileName) if err != nil { - return nil, fmt.Errorf("failed to edit temporary file: %w", err) + return nil, fmt.Errorf("failed to read and delete temporary file: %w", err) } - return editionBuffer.Bytes(), nil + return editedContent, nil } // editor takes a complete resource and a partial resourceUpdate From 385c4310940c4873db1c0fa8752de782d34b70a0 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 17 Mar 2023 14:35:12 +0100 Subject: [PATCH 04/37] improve text editors support and add file extension to temporary file --- internal/editor/editor.go | 11 +++++---- internal/editor/marshal.go | 2 +- internal/editor/tempfile.go | 48 +++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) create mode 100644 internal/editor/tempfile.go diff --git a/internal/editor/editor.go b/internal/editor/editor.go index a51145a52e..0e2e97ede6 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -2,6 +2,7 @@ package editor import ( "fmt" + "os" "os/exec" "reflect" "strings" @@ -10,6 +11,7 @@ import ( ) var SkipEditor = false +var marshalMode = MarshalModeYaml type GetResourceFunc func(interface{}) (interface{}, error) @@ -18,7 +20,7 @@ func edit(content []byte) ([]byte, error) { return content, nil } - tmpFileName, err := createTemporaryFile(content) + tmpFileName, err := createTemporaryFile(content, marshalMode) if err != nil { return nil, fmt.Errorf("failed to create temporary file: %w", err) } @@ -30,6 +32,8 @@ func edit(content []byte) ([]byte, error) { args = append(editorAndArguments[1:], args...) } cmd := exec.Command(editorAndArguments[0], args...) + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout err = cmd.Run() if err != nil { @@ -55,8 +59,7 @@ func editor(resource interface{}, updateResourceRequest interface{}, editedJson valueMapper(updateResourceRequestToEditV, updateResourceRequestV) valueMapper(updateResourceRequestToEditV, resourceV) - // TODO: allow yaml marshal - updateResourceRequestJson, err := Marshal(updateResourceRequestToEditV.Interface(), MarshalModeJson) + updateResourceRequestJson, err := Marshal(updateResourceRequestToEditV.Interface(), marshalMode) if err != nil { return nil, fmt.Errorf("failed to marshal update request: %w", err) } @@ -76,7 +79,7 @@ func editor(resource interface{}, updateResourceRequest interface{}, editedJson // Create a new updateResourceRequest as destination for edited one updateResourceRequestEdited := reflect.New(updateResourceRequestV.Type().Elem()) - err = Unmarshal(updateResourceRequestJson, updateResourceRequestEdited.Interface(), MarshalModeJson) + err = Unmarshal(updateResourceRequestJson, updateResourceRequestEdited.Interface(), marshalMode) if err != nil { return nil, fmt.Errorf("failed to unmarshal edited data: %w", err) } diff --git a/internal/editor/marshal.go b/internal/editor/marshal.go index edded4aab4..c557caa4ac 100644 --- a/internal/editor/marshal.go +++ b/internal/editor/marshal.go @@ -4,7 +4,7 @@ import ( "encoding/json" "fmt" - "github.com/ghodss/yaml" + "gopkg.in/yaml.v3" ) type MarshalMode string diff --git a/internal/editor/tempfile.go b/internal/editor/tempfile.go new file mode 100644 index 0000000000..d048d8c728 --- /dev/null +++ b/internal/editor/tempfile.go @@ -0,0 +1,48 @@ +package editor + +import ( + "fmt" + "os" +) + +func temporaryFileNamePattern(marshalMode MarshalMode) string { + pattern := "scw-editor" + switch marshalMode { + case MarshalModeYaml: + pattern += "*.yml" + case MarshalModeJson: + pattern += "*.json" + } + return pattern +} + +func createTemporaryFile(content []byte, marshalMode MarshalMode) (string, error) { + tmpFile, err := os.CreateTemp(os.TempDir(), temporaryFileNamePattern(marshalMode)) + if err != nil { + return "", fmt.Errorf("failed to create file: %w", err) + } + _, err = tmpFile.Write(content) + if err != nil { + return "", fmt.Errorf("failed to write to file %q: %w", tmpFile.Name(), err) + } + err = tmpFile.Close() + if err != nil { + return "", fmt.Errorf("failed to close file %q: %w", tmpFile.Name(), err) + } + + return tmpFile.Name(), nil +} + +func readAndDeleteFile(name string) ([]byte, error) { + content, err := os.ReadFile(name) + if err != nil { + return nil, fmt.Errorf("failed to read file %q: %w", name, err) + } + + err = os.Remove(name) + if err != nil { + return nil, fmt.Errorf("failed to delete file %q: %w", name, err) + } + + return content, nil +} From ad9aee244c68a046e2dec9bfab1920de7fc41568 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 17 Mar 2023 15:34:36 +0100 Subject: [PATCH 05/37] refactor --- internal/editor/editor.go | 67 ++++++++++++++++----------------- internal/editor/editor_test.go | 46 +++++++++++----------- internal/editor/interceptor.go | 2 +- internal/editor/request.go | 41 ++++++++++++++++++++ internal/editor/request_test.go | 22 +++++++++++ internal/editor/tempfile.go | 2 +- 6 files changed, 121 insertions(+), 59 deletions(-) create mode 100644 internal/editor/request.go create mode 100644 internal/editor/request_test.go diff --git a/internal/editor/editor.go b/internal/editor/editor.go index 0e2e97ede6..98d906a267 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -15,6 +15,9 @@ var marshalMode = MarshalModeYaml type GetResourceFunc func(interface{}) (interface{}, error) +// edit create a temporary file with given content, start a text editor then return edited content +// temporary file will be deleted on complete +// temporary file is not deleted if edit fails func edit(content []byte) ([]byte, error) { if SkipEditor == true { return content, nil @@ -48,67 +51,61 @@ func edit(content []byte) ([]byte, error) { return editedContent, nil } -// editor takes a complete resource and a partial resourceUpdate -func editor(resource interface{}, updateResourceRequest interface{}, editedJson ...string) (interface{}, error) { - resourceV := reflect.ValueOf(resource) - updateResourceRequestV := reflect.ValueOf(updateResourceRequest) +// updateResourceEditor takes a complete resource and a partial updateRequest +// will return a copy of updateRequest that has been edited +func updateResourceEditor(resource interface{}, updateRequest interface{}, editedResource ...string) (interface{}, error) { + // Create a copy of updateRequest completed with resource content + completeUpdateRequest := copyAndCompleteUpdateRequest(updateRequest, resource) - // Create a new updateResourceRequest that will be edited - // It will allow user to edit it, then we will extract diff to perform update - updateResourceRequestToEditV := reflect.New(updateResourceRequestV.Type().Elem()) - valueMapper(updateResourceRequestToEditV, updateResourceRequestV) - valueMapper(updateResourceRequestToEditV, resourceV) - - updateResourceRequestJson, err := Marshal(updateResourceRequestToEditV.Interface(), marshalMode) + updateRequestMarshaled, err := Marshal(completeUpdateRequest, marshalMode) if err != nil { return nil, fmt.Errorf("failed to marshal update request: %w", err) } - // Start text editor to edit json - updateResourceRequestJson, err = edit(updateResourceRequestJson) + // Start text editor to edit marshaled request + updateRequestMarshaled, err = edit(updateRequestMarshaled) if err != nil { return nil, fmt.Errorf("failed to edit marshalled data: %w", err) } - // If editedJson is present, override edited json + // If editedResource is present, override edited resource // This is useful for testing purpose - if len(editedJson) == 1 { - updateResourceRequestJson = []byte(editedJson[0]) + if len(editedResource) == 1 { + updateRequestMarshaled = []byte(editedResource[0]) } - // Create a new updateResourceRequest as destination for edited one - updateResourceRequestEdited := reflect.New(updateResourceRequestV.Type().Elem()) + // Create a new updateRequest as destination for edited yaml/json + // Must be a new one to avoid merge of maps content + updateRequestEdited := newRequest(updateRequest) + + // TODO: fill updateRequestEdited with only edited fields and fields present in updateRequest + // TODO: fields should be compared with completeUpdateRequest to find edited ones - err = Unmarshal(updateResourceRequestJson, updateResourceRequestEdited.Interface(), marshalMode) + err = Unmarshal(updateRequestMarshaled, updateRequestEdited, marshalMode) if err != nil { return nil, fmt.Errorf("failed to unmarshal edited data: %w", err) } - return updateResourceRequestEdited.Interface(), nil + return updateRequestEdited, nil } -// Editor takes a partial UpdateResourceRequest, the type of the GetResourceRequest and a function that return the resource using GetResourceRequest -func Editor(updateResourceRequest interface{}, getResourceRequestType reflect.Type, getResource GetResourceFunc) (interface{}, error) { - updateResourceRequestV := reflect.ValueOf(updateResourceRequest) - - // Create GetResourceRequest to be able to fetch the actual resource - getResourceRequest := reflect.New(getResourceRequestType).Interface() - getResourceRequestV := reflect.ValueOf(getResourceRequest) - - // Fill GetResourceRequest args using Update arg content - // This should copy important argument like ResourceID - valueMapper(getResourceRequestV, updateResourceRequestV) +// UpdateResourceEditor takes: +// - a partial UpdateRequest +// - the type of the GetRequest +// - a function that return the resource using GetRequest +func UpdateResourceEditor(updateRequest interface{}, getRequestType reflect.Type, getResource GetResourceFunc) (interface{}, error) { + getRequest := createGetRequest(updateRequest, getRequestType) - resource, err := getResource(getResourceRequest) + resource, err := getResource(getRequest) if err != nil { return nil, fmt.Errorf("failed to get resource: %w", err) } - // Start edition of UpdateResourceRequest - editedArgs, err := editor(resource, updateResourceRequest) + // Start edition of UpdateRequest + editedUpdateRequest, err := updateResourceEditor(resource, updateRequest) if err != nil { return nil, fmt.Errorf("failed to edit update arguments: %w", err) } - return editedArgs, nil + return editedUpdateRequest, nil } diff --git a/internal/editor/editor_test.go b/internal/editor/editor_test.go index b13737624b..aff8ba142a 100644 --- a/internal/editor/editor_test.go +++ b/internal/editor/editor_test.go @@ -8,7 +8,7 @@ import ( "github.com/alecthomas/assert" ) -func Test_editor(t *testing.T) { +func Test_updateResourceEditor(t *testing.T) { SkipEditor = true resource := &struct { @@ -18,7 +18,7 @@ func Test_editor(t *testing.T) { "uuid", "name", } - resourceUpdate := &struct { + updateRequest := &struct { ID string Name string }{ @@ -26,14 +26,14 @@ func Test_editor(t *testing.T) { "", } - _, err := editor(resource, resourceUpdate) + _, err := updateResourceEditor(resource, updateRequest) assert.Nil(t, err) } -func Test_editor_pointers(t *testing.T) { +func Test_updateResourceEditor_pointers(t *testing.T) { SkipEditor = true - type ResourceUpdate struct { + type UpdateRequest struct { ID string Name *string } @@ -45,22 +45,23 @@ func Test_editor_pointers(t *testing.T) { "name", } - resourceUpdate := &ResourceUpdate{ + updateRequest := &UpdateRequest{ "uuid", nil, } - editedResourceUpdateI, err := editor(resource, resourceUpdate) + editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest) assert.Nil(t, err) - editedResourceUpdate := editedResourceUpdateI.(*ResourceUpdate) - assert.NotNil(t, editedResourceUpdate.Name) - assert.Equal(t, resource.Name, *editedResourceUpdate.Name) + editedUpdateRequest := editedUpdateRequestI.(*UpdateRequest) + + assert.NotNil(t, editedUpdateRequest.Name) + assert.Equal(t, resource.Name, *editedUpdateRequest.Name) } -func Test_editor_map(t *testing.T) { +func Test_updateResourceEditor_map(t *testing.T) { SkipEditor = true - type ResourceUpdate struct { + type UpdateRequest struct { ID string `json:"id"` Env *map[string]string `json:"env"` } @@ -74,22 +75,23 @@ func Test_editor_map(t *testing.T) { }, } - resourceUpdate := &ResourceUpdate{ + updateRequest := &UpdateRequest{ "uuid", nil, } SkipEditor = true - editedResourceUpdateI, err := editor(resource, resourceUpdate, ` - {"id":"uuid", "env": {}} + editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, ` +id: uuid +env: {} `) assert.Nil(t, err) - editedResourceUpdate := editedResourceUpdateI.(*ResourceUpdate) - assert.NotNil(t, editedResourceUpdate.Env) - assert.True(t, len(*editedResourceUpdate.Env) == 0) + editedUpdateRequest := editedUpdateRequestI.(*UpdateRequest) + assert.NotNil(t, editedUpdateRequest.Env) + assert.True(t, len(*editedUpdateRequest.Env) == 0) } -func TestEditor(t *testing.T) { +func TestUpdateResourceEditor(t *testing.T) { SkipEditor = true resource := &struct { @@ -99,7 +101,7 @@ func TestEditor(t *testing.T) { "uuid", "name", } - resourceUpdate := &struct { + updateRequest := &struct { ID string Name string }{ @@ -107,9 +109,9 @@ func TestEditor(t *testing.T) { "", } - editedResource, err := Editor(resourceUpdate, reflect.TypeOf(*resource), func(i interface{}) (interface{}, error) { + editedUpdateRequest, err := UpdateResourceEditor(updateRequest, reflect.TypeOf(*resource), func(i interface{}) (interface{}, error) { return resource, nil }) assert.Nil(t, err) - log.Println(editedResource) + log.Println(editedUpdateRequest) } diff --git a/internal/editor/interceptor.go b/internal/editor/interceptor.go index d96310f883..9cf931dfba 100644 --- a/internal/editor/interceptor.go +++ b/internal/editor/interceptor.go @@ -12,7 +12,7 @@ func Interceptor(getter *core.Command) core.CommandInterceptor { return func(ctx context.Context, argsI interface{}, runner core.CommandRunner) (interface{}, error) { argsV := reflect.ValueOf(argsI) - editedArgs, err := Editor(argsI, getter.ArgsType, func(i interface{}) (interface{}, error) { + editedArgs, err := UpdateResourceEditor(argsI, getter.ArgsType, func(i interface{}) (interface{}, error) { return getter.Run(ctx, i) }) if err != nil { diff --git a/internal/editor/request.go b/internal/editor/request.go new file mode 100644 index 0000000000..454c3f4eed --- /dev/null +++ b/internal/editor/request.go @@ -0,0 +1,41 @@ +package editor + +import "reflect" + +// createGetRequest creates a GetRequest from given type and populate it with content from updateRequest +func createGetRequest(updateRequest interface{}, getRequestType reflect.Type) interface{} { + updateRequestV := reflect.ValueOf(updateRequest) + + getRequest := reflect.New(getRequestType).Interface() + getRequestV := reflect.ValueOf(getRequest) + + // Fill GetRequest args using Update arg content + // This should copy important argument like ID, zone + valueMapper(getRequestV, updateRequestV) + + return getRequest +} + +// copyAndCompleteUpdateRequest return a copy of updateRequest completed with resource content +func copyAndCompleteUpdateRequest(updateRequest interface{}, resource interface{}) interface{} { + resourceV := reflect.ValueOf(resource) + updateRequestV := reflect.ValueOf(updateRequest) + + // Create a new updateRequest that will be edited + // It will allow user to edit it, then we will extract diff to perform update + newUpdateRequestV := reflect.New(updateRequestV.Type().Elem()) + valueMapper(newUpdateRequestV, updateRequestV) + valueMapper(newUpdateRequestV, resourceV) + + return newUpdateRequestV.Interface() +} + +func newRequest(request interface{}) interface{} { + requestType := reflect.TypeOf(request) + + if requestType.Kind() == reflect.Pointer { + requestType = requestType.Elem() + } + + return reflect.New(requestType).Interface() +} diff --git a/internal/editor/request_test.go b/internal/editor/request_test.go new file mode 100644 index 0000000000..b62e06699b --- /dev/null +++ b/internal/editor/request_test.go @@ -0,0 +1,22 @@ +package editor + +import ( + "reflect" + "testing" + + "github.com/alecthomas/assert" +) + +func Test_createGetResourceRequest(t *testing.T) { + type GetRequest struct { + ID string + } + updateRequest := struct { + ID string + Name string + }{"uuid", ""} + expectedRequest := &GetRequest{"uuid"} + + actualRequest := createGetRequest(updateRequest, reflect.TypeOf(GetRequest{})) + assert.Equal(t, expectedRequest, actualRequest) +} diff --git a/internal/editor/tempfile.go b/internal/editor/tempfile.go index d048d8c728..6ef5cb4093 100644 --- a/internal/editor/tempfile.go +++ b/internal/editor/tempfile.go @@ -6,7 +6,7 @@ import ( ) func temporaryFileNamePattern(marshalMode MarshalMode) string { - pattern := "scw-editor" + pattern := "scw-updateResourceEditor" switch marshalMode { case MarshalModeYaml: pattern += "*.yml" From c6a973a3e139986f0c515c4d02d3fc8a952537b1 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 17 Mar 2023 15:35:29 +0100 Subject: [PATCH 06/37] disable broken test --- internal/editor/interceptor_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/editor/interceptor_test.go b/internal/editor/interceptor_test.go index cc76cfc550..be4019c9a1 100644 --- a/internal/editor/interceptor_test.go +++ b/internal/editor/interceptor_test.go @@ -12,6 +12,9 @@ import ( ) func TestInterceptor(t *testing.T) { + t.Skip("Cannot be run here as there is no cli environment") + // TODO: fix + cmds := namespaces.GetCommands() interceptor := editor.Interceptor(cmds.MustFind("container", "namespace", "get")) ctx := context.Background() From fddba65de53211270bfe8d3142b388e1f7eec02c Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 17 Mar 2023 15:41:26 +0100 Subject: [PATCH 07/37] lint --- internal/editor/editor.go | 25 ++++++++++++++++--------- internal/editor/marshal.go | 12 ++++++------ internal/editor/reflect.go | 11 +---------- internal/editor/tempfile.go | 4 ++-- 4 files changed, 25 insertions(+), 27 deletions(-) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index 98d906a267..ad5909e2c4 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -11,15 +11,27 @@ import ( ) var SkipEditor = false -var marshalMode = MarshalModeYaml +var marshalMode = MarshalModeYAML type GetResourceFunc func(interface{}) (interface{}, error) +func editorPathAndArgs(fileName string) (string, []string) { + defaultEditor := config.GetDefaultEditor() + editorAndArguments := strings.Fields(defaultEditor) + args := []string{fileName} + + if len(editorAndArguments) > 1 { + args = append(editorAndArguments[1:], args...) + } + + return editorAndArguments[0], args +} + // edit create a temporary file with given content, start a text editor then return edited content // temporary file will be deleted on complete // temporary file is not deleted if edit fails func edit(content []byte) ([]byte, error) { - if SkipEditor == true { + if SkipEditor { return content, nil } @@ -28,13 +40,8 @@ func edit(content []byte) ([]byte, error) { return nil, fmt.Errorf("failed to create temporary file: %w", err) } - defaultEditor := config.GetDefaultEditor() - editorAndArguments := strings.Fields(defaultEditor) - args := []string{tmpFileName} - if len(editorAndArguments) > 1 { - args = append(editorAndArguments[1:], args...) - } - cmd := exec.Command(editorAndArguments[0], args...) + editorPath, args := editorPathAndArgs(tmpFileName) + cmd := exec.Command(editorPath, args...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout diff --git a/internal/editor/marshal.go b/internal/editor/marshal.go index c557caa4ac..f5133785b5 100644 --- a/internal/editor/marshal.go +++ b/internal/editor/marshal.go @@ -10,15 +10,15 @@ import ( type MarshalMode string const ( - MarshalModeYaml = MarshalMode("yaml") - MarshalModeJson = MarshalMode("json") + MarshalModeYAML = MarshalMode("yaml") + MarshalModeJSON = MarshalMode("json") ) func Marshal(i interface{}, mode MarshalMode) ([]byte, error) { switch mode { - case MarshalModeYaml: + case MarshalModeYAML: return yaml.Marshal(i) - case MarshalModeJson: + case MarshalModeJSON: return json.MarshalIndent(i, "", " ") } @@ -27,9 +27,9 @@ func Marshal(i interface{}, mode MarshalMode) ([]byte, error) { func Unmarshal(data []byte, i interface{}, mode MarshalMode) error { switch mode { - case MarshalModeYaml: + case MarshalModeYAML: return yaml.Unmarshal(data, i) - case MarshalModeJson: + case MarshalModeJSON: return json.Unmarshal(data, i) } diff --git a/internal/editor/reflect.go b/internal/editor/reflect.go index 063150bebd..c8a0f44ff4 100644 --- a/internal/editor/reflect.go +++ b/internal/editor/reflect.go @@ -4,13 +4,6 @@ import ( "reflect" ) -func tryElem(v reflect.Value) reflect.Value { - if v.Kind() == reflect.Pointer { - return v.Elem() - } - return v -} - func areSameType(v1 reflect.Value, v2 reflect.Value) bool { v1t := v1.Type() v2t := v2.Type() @@ -30,7 +23,7 @@ func areSameType(v1 reflect.Value, v2 reflect.Value) bool { // valueMapper get all fields present both in src and dest and set them in dest // if argument is not zero-value in dest, it is not set -func valueMapper(dest reflect.Value, src reflect.Value) interface{} { +func valueMapper(dest reflect.Value, src reflect.Value) { if dest.Kind() == reflect.Pointer { dest = dest.Elem() } @@ -55,6 +48,4 @@ func valueMapper(dest reflect.Value, src reflect.Value) interface{} { destField.Set(srcField) } - - return dest } diff --git a/internal/editor/tempfile.go b/internal/editor/tempfile.go index 6ef5cb4093..299fcadaed 100644 --- a/internal/editor/tempfile.go +++ b/internal/editor/tempfile.go @@ -8,9 +8,9 @@ import ( func temporaryFileNamePattern(marshalMode MarshalMode) string { pattern := "scw-updateResourceEditor" switch marshalMode { - case MarshalModeYaml: + case MarshalModeYAML: pattern += "*.yml" - case MarshalModeJson: + case MarshalModeJSON: pattern += "*.json" } return pattern From 51e7aaecaf6c2903394d4db6d382f7758cb23708 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 17 Mar 2023 17:46:56 +0100 Subject: [PATCH 08/37] change yaml marshal to yamljson --- internal/editor/marshal.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/editor/marshal.go b/internal/editor/marshal.go index f5133785b5..559c0287cd 100644 --- a/internal/editor/marshal.go +++ b/internal/editor/marshal.go @@ -4,7 +4,7 @@ import ( "encoding/json" "fmt" - "gopkg.in/yaml.v3" + "github.com/ghodss/yaml" ) type MarshalMode string From ff7c3f58516ca34081158dd5bf6795f11fe2796c Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Mon, 20 Mar 2023 11:31:57 +0100 Subject: [PATCH 09/37] handle slices and refactor valueMapper --- internal/editor/reflect.go | 58 +++++++++++++++++++++++---------- internal/editor/reflect_test.go | 49 ++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/internal/editor/reflect.go b/internal/editor/reflect.go index c8a0f44ff4..ecad4e7158 100644 --- a/internal/editor/reflect.go +++ b/internal/editor/reflect.go @@ -11,6 +11,13 @@ func areSameType(v1 reflect.Value, v2 reflect.Value) bool { if v1t == v2t { return true } + + // If both are slice, compare underlying type + if v1t.Kind() == reflect.Slice && v2t.Kind() == reflect.Slice { + v1t = v1t.Elem() + v2t = v2t.Elem() + } + if v1t.Kind() == reflect.Pointer { v1t = v1t.Elem() } @@ -21,31 +28,48 @@ func areSameType(v1 reflect.Value, v2 reflect.Value) bool { return v1t == v2t } +func valueMapperScalar(dest reflect.Value, src reflect.Value) { + dest.Set(src) +} + // valueMapper get all fields present both in src and dest and set them in dest // if argument is not zero-value in dest, it is not set func valueMapper(dest reflect.Value, src reflect.Value) { - if dest.Kind() == reflect.Pointer { - dest = dest.Elem() - } - if src.Kind() == reflect.Pointer { - src = src.Elem() - } + switch dest.Kind() { + case reflect.Struct: + for i := 0; i < dest.NumField(); i++ { + destField := dest.Field(i) + fieldType := dest.Type().Field(i) + srcField := src.FieldByName(fieldType.Name) - for i := 0; i < dest.NumField(); i++ { - destField := dest.Field(i) - fieldType := dest.Type().Field(i) - srcField := src.FieldByName(fieldType.Name) + if !srcField.IsValid() || srcField.IsZero() || !areSameType(srcField, destField) { + continue + } - if !srcField.IsValid() || srcField.IsZero() || !areSameType(srcField, destField) { - continue + valueMapper(destField, srcField) + } + case reflect.Pointer: + // If source is not a pointer, we allocate destination if needed + if src.Kind() != reflect.Pointer && dest.IsZero() { + dest.Set(reflect.New(src.Type())) } - // If dest is a nil pointer (*string) and src is not, allocate a pointer for dest - if destField.Kind() == reflect.Pointer && srcField.Kind() != reflect.Pointer && destField.IsZero() { - destField.Set(reflect.New(srcField.Type())) - destField = destField.Elem() + if src.Kind() == reflect.Pointer { + src = src.Elem() } + dest = dest.Elem() - destField.Set(srcField) + valueMapper(dest, src) + case reflect.Slice: + // If destination is a slice, allocate the slice and map each value + srcLen := src.Len() + dest.Set(reflect.MakeSlice(dest.Type(), srcLen, srcLen)) + for i := 0; i < srcLen; i++ { + valueMapper(dest.Index(i), src.Index(i)) + } + default: + // Should be scalar types + dest.Set(src) } + } diff --git a/internal/editor/reflect_test.go b/internal/editor/reflect_test.go index 867358dfd3..dda8e48805 100644 --- a/internal/editor/reflect_test.go +++ b/internal/editor/reflect_test.go @@ -68,3 +68,52 @@ func Test_valueMapperPointers(t *testing.T) { assert.NotNil(t, dest.Arg2) assert.EqualValues(t, src.Arg2, *dest.Arg2) } + +func Test_valueMapperSlice(t *testing.T) { + src := struct { + Arg1 []string + Arg2 []int + }{ + []string{"1", "2", "3"}, + []int{1, 2, 3}, + } + dest := struct { + Arg1 []string + Arg2 []int + }{} + + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + assert.NotNil(t, dest.Arg1) + assert.EqualValues(t, src.Arg1, dest.Arg1) + assert.NotNil(t, dest.Arg2) + assert.EqualValues(t, src.Arg2, dest.Arg2) +} + +func Test_valueMapperSliceOfPointers(t *testing.T) { + src := struct { + Arg1 []string + Arg2 []int + }{ + []string{"1", "2", "3"}, + []int{1, 2, 3}, + } + dest := struct { + Arg1 []*string + Arg2 []*int + }{} + + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + assert.NotNil(t, dest.Arg1) + assert.Equal(t, len(src.Arg1), len(dest.Arg1)) + for i := range src.Arg1 { + assert.NotNil(t, dest.Arg1[i]) + assert.Equal(t, src.Arg1[i], *dest.Arg1[i]) + } + + assert.NotNil(t, dest.Arg2) + assert.Equal(t, len(src.Arg2), len(dest.Arg2)) + for i := range src.Arg2 { + assert.NotNil(t, dest.Arg2[i]) + assert.Equal(t, src.Arg2[i], *dest.Arg2[i]) + } +} From af91d338e09c8598a5e1869a10d2cce05763874d Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Mon, 20 Mar 2023 11:40:41 +0100 Subject: [PATCH 10/37] add putRequest bool and add TODO --- internal/editor/editor.go | 15 ++++++++++++--- internal/editor/editor_test.go | 8 ++++---- internal/editor/interceptor.go | 2 +- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index ad5909e2c4..312f641652 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -45,6 +45,8 @@ func edit(content []byte) ([]byte, error) { cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout + // TODO: always delete temp file to avoid credentials leak + err = cmd.Run() if err != nil { return nil, fmt.Errorf("failed to edit temporary file %q: %w", tmpFileName, err) @@ -60,10 +62,15 @@ func edit(content []byte) ([]byte, error) { // updateResourceEditor takes a complete resource and a partial updateRequest // will return a copy of updateRequest that has been edited -func updateResourceEditor(resource interface{}, updateRequest interface{}, editedResource ...string) (interface{}, error) { +func updateResourceEditor(resource interface{}, updateRequest interface{}, putRequest bool, editedResource ...string) (interface{}, error) { // Create a copy of updateRequest completed with resource content completeUpdateRequest := copyAndCompleteUpdateRequest(updateRequest, resource) + // TODO: fields present in updateRequest should be removed from marshal + // ex: namespace_id, region, zone + // Currently not an issue as fields that should be removed are mostly path parameter /{zone}/namespace/{namespace_id} + // Path parameter have "-" as json tag and are not marshaled + updateRequestMarshaled, err := Marshal(completeUpdateRequest, marshalMode) if err != nil { return nil, fmt.Errorf("failed to marshal update request: %w", err) @@ -85,6 +92,7 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, edite // Must be a new one to avoid merge of maps content updateRequestEdited := newRequest(updateRequest) + // TODO: if !putRequest // TODO: fill updateRequestEdited with only edited fields and fields present in updateRequest // TODO: fields should be compared with completeUpdateRequest to find edited ones @@ -100,7 +108,8 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, edite // - a partial UpdateRequest // - the type of the GetRequest // - a function that return the resource using GetRequest -func UpdateResourceEditor(updateRequest interface{}, getRequestType reflect.Type, getResource GetResourceFunc) (interface{}, error) { +// - a bool true if updateRequest is a PUT request +func UpdateResourceEditor(updateRequest interface{}, getRequestType reflect.Type, getResource GetResourceFunc, putRequest bool) (interface{}, error) { getRequest := createGetRequest(updateRequest, getRequestType) resource, err := getResource(getRequest) @@ -109,7 +118,7 @@ func UpdateResourceEditor(updateRequest interface{}, getRequestType reflect.Type } // Start edition of UpdateRequest - editedUpdateRequest, err := updateResourceEditor(resource, updateRequest) + editedUpdateRequest, err := updateResourceEditor(resource, updateRequest, putRequest) if err != nil { return nil, fmt.Errorf("failed to edit update arguments: %w", err) } diff --git a/internal/editor/editor_test.go b/internal/editor/editor_test.go index aff8ba142a..82ef3a7be8 100644 --- a/internal/editor/editor_test.go +++ b/internal/editor/editor_test.go @@ -26,7 +26,7 @@ func Test_updateResourceEditor(t *testing.T) { "", } - _, err := updateResourceEditor(resource, updateRequest) + _, err := updateResourceEditor(resource, updateRequest, false) assert.Nil(t, err) } @@ -50,7 +50,7 @@ func Test_updateResourceEditor_pointers(t *testing.T) { nil, } - editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest) + editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, false) assert.Nil(t, err) editedUpdateRequest := editedUpdateRequestI.(*UpdateRequest) @@ -81,7 +81,7 @@ func Test_updateResourceEditor_map(t *testing.T) { } SkipEditor = true - editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, ` + editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, false, ` id: uuid env: {} `) @@ -111,7 +111,7 @@ func TestUpdateResourceEditor(t *testing.T) { editedUpdateRequest, err := UpdateResourceEditor(updateRequest, reflect.TypeOf(*resource), func(i interface{}) (interface{}, error) { return resource, nil - }) + }, false) assert.Nil(t, err) log.Println(editedUpdateRequest) } diff --git a/internal/editor/interceptor.go b/internal/editor/interceptor.go index 9cf931dfba..ee34772c8a 100644 --- a/internal/editor/interceptor.go +++ b/internal/editor/interceptor.go @@ -14,7 +14,7 @@ func Interceptor(getter *core.Command) core.CommandInterceptor { editedArgs, err := UpdateResourceEditor(argsI, getter.ArgsType, func(i interface{}) (interface{}, error) { return getter.Run(ctx, i) - }) + }, false) if err != nil { return nil, fmt.Errorf("failed to edit args: %w", err) } From a5e66fa831ea1952b03ba361158809d4a8d4f61c Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Mon, 20 Mar 2023 18:54:35 +0100 Subject: [PATCH 11/37] better support of pointers --- internal/editor/reflect.go | 18 +++++---- internal/editor/reflect_test.go | 67 +++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/internal/editor/reflect.go b/internal/editor/reflect.go index ecad4e7158..c834c5428b 100644 --- a/internal/editor/reflect.go +++ b/internal/editor/reflect.go @@ -25,11 +25,12 @@ func areSameType(v1 reflect.Value, v2 reflect.Value) bool { v2t = v2t.Elem() } - return v1t == v2t -} + // If both are struct consider them equal, valueMapper will try to map fields + if v1t.Kind() == reflect.Struct && v2t.Kind() == reflect.Struct { + return true + } -func valueMapperScalar(dest reflect.Value, src reflect.Value) { - dest.Set(src) + return v1t == v2t } // valueMapper get all fields present both in src and dest and set them in dest @@ -42,6 +43,7 @@ func valueMapper(dest reflect.Value, src reflect.Value) { fieldType := dest.Type().Field(i) srcField := src.FieldByName(fieldType.Name) + // TODO: Move to default if !srcField.IsValid() || srcField.IsZero() || !areSameType(srcField, destField) { continue } @@ -49,14 +51,15 @@ func valueMapper(dest reflect.Value, src reflect.Value) { valueMapper(destField, srcField) } case reflect.Pointer: - // If source is not a pointer, we allocate destination if needed - if src.Kind() != reflect.Pointer && dest.IsZero() { - dest.Set(reflect.New(src.Type())) + // If destination is a pointer, we allocate destination if needed + if dest.IsZero() { + dest.Set(reflect.New(dest.Type().Elem())) } if src.Kind() == reflect.Pointer { src = src.Elem() } + dest = dest.Elem() valueMapper(dest, src) @@ -71,5 +74,4 @@ func valueMapper(dest reflect.Value, src reflect.Value) { // Should be scalar types dest.Set(src) } - } diff --git a/internal/editor/reflect_test.go b/internal/editor/reflect_test.go index dda8e48805..d3360b2388 100644 --- a/internal/editor/reflect_test.go +++ b/internal/editor/reflect_test.go @@ -1,10 +1,13 @@ package editor import ( + "net" "reflect" "testing" "github.com/alecthomas/assert" + "github.com/scaleway/scaleway-sdk-go/api/instance/v1" + "github.com/scaleway/scaleway-sdk-go/scw" ) func Test_valueMapper(t *testing.T) { @@ -69,6 +72,23 @@ func Test_valueMapperPointers(t *testing.T) { assert.EqualValues(t, src.Arg2, *dest.Arg2) } +func Test_valueMapperPointersWithPointers(t *testing.T) { + src := struct { + Arg1 *string + Arg2 *int32 + }{scw.StringPtr("1"), scw.Int32Ptr(1)} + dest := struct { + Arg1 *string + Arg2 *int32 + }{} + + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + assert.NotNil(t, dest.Arg1) + assert.EqualValues(t, src.Arg1, dest.Arg1) + assert.NotNil(t, dest.Arg2) + assert.EqualValues(t, src.Arg2, dest.Arg2) +} + func Test_valueMapperSlice(t *testing.T) { src := struct { Arg1 []string @@ -117,3 +137,50 @@ func Test_valueMapperSliceOfPointers(t *testing.T) { assert.Equal(t, src.Arg2[i], *dest.Arg2[i]) } } + +func Test_valueMapperSliceStructPointer(t *testing.T) { + _, ipnet, err := net.ParseCIDR("192.168.0.0/24") + assert.Nil(t, err) + + src := instance.ListSecurityGroupRulesResponse{ + TotalCount: 0, + Rules: []*instance.SecurityGroupRule{ + { + ID: "id", + Protocol: "protocol", + Direction: "direction", + Action: "action", + IPRange: scw.IPNet{*ipnet}, + DestPortFrom: scw.Uint32Ptr(1000), + DestPortTo: scw.Uint32Ptr(2000), + Position: 12, + Editable: true, + Zone: "zone", + }, + }, + } + dest := instance.SetSecurityGroupRulesRequest{ + Rules: nil, + } + + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + assert.NotNil(t, dest.Rules) + assert.Equal(t, 1, len(dest.Rules)) + expectedRule := src.Rules[0] + actualRule := dest.Rules[0] + assert.NotNil(t, actualRule.ID) + assert.Equal(t, expectedRule.ID, *actualRule.ID) + assert.Equal(t, expectedRule.Protocol, actualRule.Protocol) + assert.Equal(t, expectedRule.Direction, actualRule.Direction) + assert.Equal(t, expectedRule.Action, actualRule.Action) + assert.Equal(t, expectedRule.IPRange, actualRule.IPRange) + assert.NotNil(t, actualRule.DestPortFrom) + assert.Equal(t, expectedRule.DestPortFrom, actualRule.DestPortFrom) + assert.NotNil(t, actualRule.DestPortTo) + assert.Equal(t, expectedRule.DestPortTo, actualRule.DestPortTo) + assert.Equal(t, expectedRule.Position, actualRule.Position) + assert.NotNil(t, actualRule.Editable) + assert.Equal(t, expectedRule.Editable, *actualRule.Editable) + assert.NotNil(t, actualRule.Zone) + assert.Equal(t, expectedRule.Zone, *actualRule.Zone) +} From b296cd7e1f7fd7e5dd33232134e120946deaab85 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Tue, 21 Mar 2023 11:54:25 +0100 Subject: [PATCH 12/37] delete interceptor and only expect resource to be given to the editor --- internal/editor/editor.go | 27 ++++++--------------------- internal/editor/editor_test.go | 27 --------------------------- internal/editor/interceptor.go | 27 --------------------------- internal/editor/interceptor_test.go | 27 --------------------------- 4 files changed, 6 insertions(+), 102 deletions(-) delete mode 100644 internal/editor/interceptor.go delete mode 100644 internal/editor/interceptor_test.go diff --git a/internal/editor/editor.go b/internal/editor/editor.go index 312f641652..06611d09f2 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "os/exec" - "reflect" "strings" "github.com/scaleway/scaleway-cli/v2/internal/config" @@ -104,24 +103,10 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, putRe return updateRequestEdited, nil } -// UpdateResourceEditor takes: -// - a partial UpdateRequest -// - the type of the GetRequest -// - a function that return the resource using GetRequest -// - a bool true if updateRequest is a PUT request -func UpdateResourceEditor(updateRequest interface{}, getRequestType reflect.Type, getResource GetResourceFunc, putRequest bool) (interface{}, error) { - getRequest := createGetRequest(updateRequest, getRequestType) - - resource, err := getResource(getRequest) - if err != nil { - return nil, fmt.Errorf("failed to get resource: %w", err) - } - - // Start edition of UpdateRequest - editedUpdateRequest, err := updateResourceEditor(resource, updateRequest, putRequest) - if err != nil { - return nil, fmt.Errorf("failed to edit update arguments: %w", err) - } - - return editedUpdateRequest, nil +// UpdateResourceEditor takes a complete resource and a partial updateRequest +// will return a copy of updateRequest that has been edited +// Only edited fields will be present in returned updateRequest +// If putRequest is true, all fields will be present, edited or not +func UpdateResourceEditor(resource interface{}, updateRequest interface{}, putRequest bool) (interface{}, error) { + return updateResourceEditor(resource, updateRequest, putRequest) } diff --git a/internal/editor/editor_test.go b/internal/editor/editor_test.go index 82ef3a7be8..81d15ef405 100644 --- a/internal/editor/editor_test.go +++ b/internal/editor/editor_test.go @@ -1,8 +1,6 @@ package editor import ( - "log" - "reflect" "testing" "github.com/alecthomas/assert" @@ -90,28 +88,3 @@ env: {} assert.NotNil(t, editedUpdateRequest.Env) assert.True(t, len(*editedUpdateRequest.Env) == 0) } - -func TestUpdateResourceEditor(t *testing.T) { - SkipEditor = true - - resource := &struct { - ID string - Name string - }{ - "uuid", - "name", - } - updateRequest := &struct { - ID string - Name string - }{ - "uuid", - "", - } - - editedUpdateRequest, err := UpdateResourceEditor(updateRequest, reflect.TypeOf(*resource), func(i interface{}) (interface{}, error) { - return resource, nil - }, false) - assert.Nil(t, err) - log.Println(editedUpdateRequest) -} diff --git a/internal/editor/interceptor.go b/internal/editor/interceptor.go deleted file mode 100644 index ee34772c8a..0000000000 --- a/internal/editor/interceptor.go +++ /dev/null @@ -1,27 +0,0 @@ -package editor - -import ( - "context" - "fmt" - "reflect" - - "github.com/scaleway/scaleway-cli/v2/internal/core" -) - -func Interceptor(getter *core.Command) core.CommandInterceptor { - return func(ctx context.Context, argsI interface{}, runner core.CommandRunner) (interface{}, error) { - argsV := reflect.ValueOf(argsI) - - editedArgs, err := UpdateResourceEditor(argsI, getter.ArgsType, func(i interface{}) (interface{}, error) { - return getter.Run(ctx, i) - }, false) - if err != nil { - return nil, fmt.Errorf("failed to edit args: %w", err) - } - - // TODO: only map diff - valueMapper(argsV, reflect.ValueOf(editedArgs)) - - return runner(ctx, argsI) - } -} diff --git a/internal/editor/interceptor_test.go b/internal/editor/interceptor_test.go deleted file mode 100644 index be4019c9a1..0000000000 --- a/internal/editor/interceptor_test.go +++ /dev/null @@ -1,27 +0,0 @@ -package editor_test - -import ( - "context" - "log" - "testing" - - "github.com/alecthomas/assert" - "github.com/scaleway/scaleway-cli/v2/internal/editor" - "github.com/scaleway/scaleway-cli/v2/internal/namespaces" - container "github.com/scaleway/scaleway-sdk-go/api/container/v1beta1" -) - -func TestInterceptor(t *testing.T) { - t.Skip("Cannot be run here as there is no cli environment") - // TODO: fix - - cmds := namespaces.GetCommands() - interceptor := editor.Interceptor(cmds.MustFind("container", "namespace", "get")) - ctx := context.Background() - res, err := interceptor(ctx, &container.UpdateNamespaceRequest{ - Region: "fr-par", - NamespaceID: "47c8f0ad-3567-451e-a0a5-820318678ce3", - }, cmds.MustFind("container", "namespace", "update").Run) - assert.Nil(t, err) - log.Println(res) -} From 6477ff8614f78de767c4746d8c22fd2e47869c48 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Tue, 21 Mar 2023 11:55:47 +0100 Subject: [PATCH 13/37] always delete temporary file --- internal/editor/editor.go | 9 ++++----- internal/editor/tempfile.go | 14 -------------- 2 files changed, 4 insertions(+), 19 deletions(-) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index 06611d09f2..3c8a4cf100 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -38,22 +38,21 @@ func edit(content []byte) ([]byte, error) { if err != nil { return nil, fmt.Errorf("failed to create temporary file: %w", err) } + defer os.Remove(tmpFileName) editorPath, args := editorPathAndArgs(tmpFileName) cmd := exec.Command(editorPath, args...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout - - // TODO: always delete temp file to avoid credentials leak - + err = cmd.Run() if err != nil { return nil, fmt.Errorf("failed to edit temporary file %q: %w", tmpFileName, err) } - editedContent, err := readAndDeleteFile(tmpFileName) + editedContent, err := os.ReadFile(tmpFileName) if err != nil { - return nil, fmt.Errorf("failed to read and delete temporary file: %w", err) + return nil, fmt.Errorf("failed to read temporary file %q: %w", tmpFileName, err) } return editedContent, nil diff --git a/internal/editor/tempfile.go b/internal/editor/tempfile.go index 299fcadaed..87b31f560d 100644 --- a/internal/editor/tempfile.go +++ b/internal/editor/tempfile.go @@ -32,17 +32,3 @@ func createTemporaryFile(content []byte, marshalMode MarshalMode) (string, error return tmpFile.Name(), nil } - -func readAndDeleteFile(name string) ([]byte, error) { - content, err := os.ReadFile(name) - if err != nil { - return nil, fmt.Errorf("failed to read file %q: %w", name, err) - } - - err = os.Remove(name) - if err != nil { - return nil, fmt.Errorf("failed to delete file %q: %w", name, err) - } - - return content, nil -} From d8eab74c07f071cb0a1265792eac3a88b175b6a8 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Tue, 21 Mar 2023 14:37:51 +0100 Subject: [PATCH 14/37] fix linter --- internal/editor/editor.go | 11 ++++++----- internal/editor/editor_test.go | 4 ++-- internal/editor/reflect_test.go | 12 +++++++----- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index 3c8a4cf100..ae8dc652a1 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -44,7 +44,7 @@ func edit(content []byte) ([]byte, error) { cmd := exec.Command(editorPath, args...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout - + err = cmd.Run() if err != nil { return nil, fmt.Errorf("failed to edit temporary file %q: %w", tmpFileName, err) @@ -60,7 +60,7 @@ func edit(content []byte) ([]byte, error) { // updateResourceEditor takes a complete resource and a partial updateRequest // will return a copy of updateRequest that has been edited -func updateResourceEditor(resource interface{}, updateRequest interface{}, putRequest bool, editedResource ...string) (interface{}, error) { +func updateResourceEditor(resource interface{}, updateRequest interface{}, putRequest bool, editedResource string) (interface{}, error) { // Create a copy of updateRequest completed with resource content completeUpdateRequest := copyAndCompleteUpdateRequest(updateRequest, resource) @@ -82,14 +82,15 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, putRe // If editedResource is present, override edited resource // This is useful for testing purpose - if len(editedResource) == 1 { - updateRequestMarshaled = []byte(editedResource[0]) + if editedResource != "" { + updateRequestMarshaled = []byte(editedResource) } // Create a new updateRequest as destination for edited yaml/json // Must be a new one to avoid merge of maps content updateRequestEdited := newRequest(updateRequest) + _ = putRequest // TODO: if !putRequest // TODO: fill updateRequestEdited with only edited fields and fields present in updateRequest // TODO: fields should be compared with completeUpdateRequest to find edited ones @@ -107,5 +108,5 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, putRe // Only edited fields will be present in returned updateRequest // If putRequest is true, all fields will be present, edited or not func UpdateResourceEditor(resource interface{}, updateRequest interface{}, putRequest bool) (interface{}, error) { - return updateResourceEditor(resource, updateRequest, putRequest) + return updateResourceEditor(resource, updateRequest, putRequest, "") } diff --git a/internal/editor/editor_test.go b/internal/editor/editor_test.go index 81d15ef405..99391125a9 100644 --- a/internal/editor/editor_test.go +++ b/internal/editor/editor_test.go @@ -24,7 +24,7 @@ func Test_updateResourceEditor(t *testing.T) { "", } - _, err := updateResourceEditor(resource, updateRequest, false) + _, err := updateResourceEditor(resource, updateRequest, false, "") assert.Nil(t, err) } @@ -48,7 +48,7 @@ func Test_updateResourceEditor_pointers(t *testing.T) { nil, } - editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, false) + editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, false, "") assert.Nil(t, err) editedUpdateRequest := editedUpdateRequestI.(*UpdateRequest) diff --git a/internal/editor/reflect_test.go b/internal/editor/reflect_test.go index d3360b2388..31ece9e73b 100644 --- a/internal/editor/reflect_test.go +++ b/internal/editor/reflect_test.go @@ -146,11 +146,13 @@ func Test_valueMapperSliceStructPointer(t *testing.T) { TotalCount: 0, Rules: []*instance.SecurityGroupRule{ { - ID: "id", - Protocol: "protocol", - Direction: "direction", - Action: "action", - IPRange: scw.IPNet{*ipnet}, + ID: "id", + Protocol: "protocol", + Direction: "direction", + Action: "action", + IPRange: scw.IPNet{ + IPNet: *ipnet, + }, DestPortFrom: scw.Uint32Ptr(1000), DestPortTo: scw.Uint32Ptr(2000), Position: 12, From 2b2bea71460c7ef6a86ad5941b1d955845887c10 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Tue, 21 Mar 2023 15:50:19 +0100 Subject: [PATCH 15/37] take editor arguments as struct --- internal/editor/doc.go | 5 +++++ internal/editor/editor.go | 27 +++++++++++++++++++-------- internal/editor/editor_test.go | 11 ++++++----- internal/editor/marshal.go | 25 +++++++++++++++++++++---- 4 files changed, 51 insertions(+), 17 deletions(-) create mode 100644 internal/editor/doc.go diff --git a/internal/editor/doc.go b/internal/editor/doc.go new file mode 100644 index 0000000000..d7c49fdf16 --- /dev/null +++ b/internal/editor/doc.go @@ -0,0 +1,5 @@ +package editor + +var LongDescription = `This command starts your default editor to edit a marshaled version of your resource +Default editor will be taken from $VISUAL, then $EDITOR or will be 'vi' +` diff --git a/internal/editor/editor.go b/internal/editor/editor.go index ae8dc652a1..aaf120e3a5 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -13,6 +13,18 @@ var SkipEditor = false var marshalMode = MarshalModeYAML type GetResourceFunc func(interface{}) (interface{}, error) +type Config struct { + // PutRequest means that the request replace all fields + // If false, fields that were not edited will not be sent + // If true, all fields will be sent + PutRequest bool + + MarshalMode MarshalMode + + // If not empty, this will replace edited text as if it was edited in the terminal + // Should be paired with global SkipEditor as true, useful for tests + editedResource string +} func editorPathAndArgs(fileName string) (string, []string) { defaultEditor := config.GetDefaultEditor() @@ -60,7 +72,7 @@ func edit(content []byte) ([]byte, error) { // updateResourceEditor takes a complete resource and a partial updateRequest // will return a copy of updateRequest that has been edited -func updateResourceEditor(resource interface{}, updateRequest interface{}, putRequest bool, editedResource string) (interface{}, error) { +func updateResourceEditor(resource interface{}, updateRequest interface{}, cfg *Config) (interface{}, error) { // Create a copy of updateRequest completed with resource content completeUpdateRequest := copyAndCompleteUpdateRequest(updateRequest, resource) @@ -69,7 +81,7 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, putRe // Currently not an issue as fields that should be removed are mostly path parameter /{zone}/namespace/{namespace_id} // Path parameter have "-" as json tag and are not marshaled - updateRequestMarshaled, err := Marshal(completeUpdateRequest, marshalMode) + updateRequestMarshaled, err := marshal(completeUpdateRequest, cfg.MarshalMode) if err != nil { return nil, fmt.Errorf("failed to marshal update request: %w", err) } @@ -82,20 +94,19 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, putRe // If editedResource is present, override edited resource // This is useful for testing purpose - if editedResource != "" { - updateRequestMarshaled = []byte(editedResource) + if cfg.editedResource != "" { + updateRequestMarshaled = []byte(cfg.editedResource) } // Create a new updateRequest as destination for edited yaml/json // Must be a new one to avoid merge of maps content updateRequestEdited := newRequest(updateRequest) - _ = putRequest // TODO: if !putRequest // TODO: fill updateRequestEdited with only edited fields and fields present in updateRequest // TODO: fields should be compared with completeUpdateRequest to find edited ones - err = Unmarshal(updateRequestMarshaled, updateRequestEdited, marshalMode) + err = unmarshal(updateRequestMarshaled, updateRequestEdited, cfg.MarshalMode) if err != nil { return nil, fmt.Errorf("failed to unmarshal edited data: %w", err) } @@ -107,6 +118,6 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, putRe // will return a copy of updateRequest that has been edited // Only edited fields will be present in returned updateRequest // If putRequest is true, all fields will be present, edited or not -func UpdateResourceEditor(resource interface{}, updateRequest interface{}, putRequest bool) (interface{}, error) { - return updateResourceEditor(resource, updateRequest, putRequest, "") +func UpdateResourceEditor(resource interface{}, updateRequest interface{}, cfg *Config) (interface{}, error) { + return updateResourceEditor(resource, updateRequest, cfg) } diff --git a/internal/editor/editor_test.go b/internal/editor/editor_test.go index 99391125a9..6b7e13ef1e 100644 --- a/internal/editor/editor_test.go +++ b/internal/editor/editor_test.go @@ -24,7 +24,7 @@ func Test_updateResourceEditor(t *testing.T) { "", } - _, err := updateResourceEditor(resource, updateRequest, false, "") + _, err := updateResourceEditor(resource, updateRequest, &Config{}) assert.Nil(t, err) } @@ -48,7 +48,7 @@ func Test_updateResourceEditor_pointers(t *testing.T) { nil, } - editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, false, "") + editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, &Config{}) assert.Nil(t, err) editedUpdateRequest := editedUpdateRequestI.(*UpdateRequest) @@ -78,11 +78,12 @@ func Test_updateResourceEditor_map(t *testing.T) { nil, } - SkipEditor = true - editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, false, ` + editedUpdateRequestI, err := updateResourceEditor(resource, updateRequest, &Config{ + editedResource: ` id: uuid env: {} -`) +`, + }) assert.Nil(t, err) editedUpdateRequest := editedUpdateRequestI.(*UpdateRequest) assert.NotNil(t, editedUpdateRequest.Env) diff --git a/internal/editor/marshal.go b/internal/editor/marshal.go index 559c0287cd..0253fcee1f 100644 --- a/internal/editor/marshal.go +++ b/internal/editor/marshal.go @@ -14,18 +14,35 @@ const ( MarshalModeJSON = MarshalMode("json") ) -func Marshal(i interface{}, mode MarshalMode) ([]byte, error) { +func marshal(i interface{}, mode MarshalMode) ([]byte, error) { + if mode == "" { + mode = MarshalModeYAML + } + + var marshaledData []byte + var err error + switch mode { case MarshalModeYAML: - return yaml.Marshal(i) + marshaledData, err = yaml.Marshal(i) case MarshalModeJSON: - return json.MarshalIndent(i, "", " ") + marshaledData, err = json.MarshalIndent(i, "", " ") + } + if err != nil { + return marshaledData, err + } + if marshaledData != nil { + return marshaledData, err } return nil, fmt.Errorf("unknown marshal mode %q", mode) } -func Unmarshal(data []byte, i interface{}, mode MarshalMode) error { +func unmarshal(data []byte, i interface{}, mode MarshalMode) error { + if mode == "" { + mode = MarshalModeYAML + } + switch mode { case MarshalModeYAML: return yaml.Unmarshal(data, i) From 8da487b9290b0eb52a240c20e02d8f1c049b44d6 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Tue, 21 Mar 2023 16:56:04 +0100 Subject: [PATCH 16/37] add back path parameter to edited request --- internal/editor/editor.go | 3 +++ internal/editor/reflect.go | 28 ++++++++++++++++++++++------ internal/editor/request.go | 11 ++++++++--- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index aaf120e3a5..782a4561ea 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -106,6 +106,9 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, cfg * // TODO: fill updateRequestEdited with only edited fields and fields present in updateRequest // TODO: fields should be compared with completeUpdateRequest to find edited ones + // Add back required non-marshaled fields (zone, ID) + copyRequestPathParameters(updateRequestEdited, updateRequest) + err = unmarshal(updateRequestMarshaled, updateRequestEdited, cfg.MarshalMode) if err != nil { return nil, fmt.Errorf("failed to unmarshal edited data: %w", err) diff --git a/internal/editor/reflect.go b/internal/editor/reflect.go index c834c5428b..4c59a08651 100644 --- a/internal/editor/reflect.go +++ b/internal/editor/reflect.go @@ -33,9 +33,19 @@ func areSameType(v1 reflect.Value, v2 reflect.Value) bool { return v1t == v2t } +func hasTag(tags []string, actualTag string) bool { + for _, tag := range tags { + if tag == actualTag { + return true + } + } + return false +} + // valueMapper get all fields present both in src and dest and set them in dest // if argument is not zero-value in dest, it is not set -func valueMapper(dest reflect.Value, src reflect.Value) { +// fields is a list of jsonTags, if not nil, only fields with a tag in this list will be mapped +func valueMapper(dest reflect.Value, src reflect.Value, fields []string) { switch dest.Kind() { case reflect.Struct: for i := 0; i < dest.NumField(); i++ { @@ -43,12 +53,17 @@ func valueMapper(dest reflect.Value, src reflect.Value) { fieldType := dest.Type().Field(i) srcField := src.FieldByName(fieldType.Name) + // If field is not in list, do not set it + if fields != nil && !hasTag(fields, fieldType.Tag.Get("json")) { + continue + } + // TODO: Move to default if !srcField.IsValid() || srcField.IsZero() || !areSameType(srcField, destField) { continue } - valueMapper(destField, srcField) + valueMapper(destField, srcField, fields) } case reflect.Pointer: // If destination is a pointer, we allocate destination if needed @@ -59,19 +74,20 @@ func valueMapper(dest reflect.Value, src reflect.Value) { if src.Kind() == reflect.Pointer { src = src.Elem() } - dest = dest.Elem() - valueMapper(dest, src) + valueMapper(dest, src, fields) + // TODO: clean pointer if not filled + // If dest is nil and src is a struct with fields disabled because of json tags + // Then dest should not be allocated and should remain nil case reflect.Slice: // If destination is a slice, allocate the slice and map each value srcLen := src.Len() dest.Set(reflect.MakeSlice(dest.Type(), srcLen, srcLen)) for i := 0; i < srcLen; i++ { - valueMapper(dest.Index(i), src.Index(i)) + valueMapper(dest.Index(i), src.Index(i), fields) } default: - // Should be scalar types dest.Set(src) } } diff --git a/internal/editor/request.go b/internal/editor/request.go index 454c3f4eed..e203d99c35 100644 --- a/internal/editor/request.go +++ b/internal/editor/request.go @@ -11,7 +11,7 @@ func createGetRequest(updateRequest interface{}, getRequestType reflect.Type) in // Fill GetRequest args using Update arg content // This should copy important argument like ID, zone - valueMapper(getRequestV, updateRequestV) + valueMapper(getRequestV, updateRequestV, nil) return getRequest } @@ -24,8 +24,8 @@ func copyAndCompleteUpdateRequest(updateRequest interface{}, resource interface{ // Create a new updateRequest that will be edited // It will allow user to edit it, then we will extract diff to perform update newUpdateRequestV := reflect.New(updateRequestV.Type().Elem()) - valueMapper(newUpdateRequestV, updateRequestV) - valueMapper(newUpdateRequestV, resourceV) + valueMapper(newUpdateRequestV, updateRequestV, nil) + valueMapper(newUpdateRequestV, resourceV, nil) return newUpdateRequestV.Interface() } @@ -39,3 +39,8 @@ func newRequest(request interface{}) interface{} { return reflect.New(requestType).Interface() } + +// copyRequestPathParameters will copy all path parameters present in src to their correct fields in dest +func copyRequestPathParameters(dest interface{}, src interface{}) { + valueMapper(reflect.ValueOf(dest), reflect.ValueOf(src), []string{"-"}) +} From f45320658e290ca423dde58ffaf7535329739cee Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Wed, 22 Mar 2023 10:37:06 +0100 Subject: [PATCH 17/37] fix tests --- internal/editor/reflect_test.go | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/internal/editor/reflect_test.go b/internal/editor/reflect_test.go index 31ece9e73b..096e294651 100644 --- a/internal/editor/reflect_test.go +++ b/internal/editor/reflect_test.go @@ -20,7 +20,7 @@ func Test_valueMapper(t *testing.T) { Arg2 int }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) assert.Equal(t, src.Arg1, dest.Arg1) assert.Equal(t, src.Arg2, dest.Arg2) } @@ -35,7 +35,7 @@ func Test_valueMapperInvalidType(t *testing.T) { Arg2 bool }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) assert.Equal(t, src.Arg1, dest.Arg1) assert.Zero(t, dest.Arg2) } @@ -50,7 +50,7 @@ func Test_valueMapperDifferentFields(t *testing.T) { Arg4 bool }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) assert.Zero(t, dest.Arg3) assert.Zero(t, dest.Arg4) } @@ -65,7 +65,7 @@ func Test_valueMapperPointers(t *testing.T) { Arg2 *int }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) assert.NotNil(t, dest.Arg1) assert.EqualValues(t, src.Arg1, *dest.Arg1) assert.NotNil(t, dest.Arg2) @@ -82,7 +82,7 @@ func Test_valueMapperPointersWithPointers(t *testing.T) { Arg2 *int32 }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) assert.NotNil(t, dest.Arg1) assert.EqualValues(t, src.Arg1, dest.Arg1) assert.NotNil(t, dest.Arg2) @@ -102,7 +102,7 @@ func Test_valueMapperSlice(t *testing.T) { Arg2 []int }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) assert.NotNil(t, dest.Arg1) assert.EqualValues(t, src.Arg1, dest.Arg1) assert.NotNil(t, dest.Arg2) @@ -122,7 +122,7 @@ func Test_valueMapperSliceOfPointers(t *testing.T) { Arg2 []*int }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) assert.NotNil(t, dest.Arg1) assert.Equal(t, len(src.Arg1), len(dest.Arg1)) for i := range src.Arg1 { @@ -165,7 +165,7 @@ func Test_valueMapperSliceStructPointer(t *testing.T) { Rules: nil, } - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) assert.NotNil(t, dest.Rules) assert.Equal(t, 1, len(dest.Rules)) expectedRule := src.Rules[0] @@ -186,3 +186,18 @@ func Test_valueMapperSliceStructPointer(t *testing.T) { assert.NotNil(t, actualRule.Zone) assert.Equal(t, expectedRule.Zone, *actualRule.Zone) } + +func Test_valueMapperTags(t *testing.T) { + src := struct { + Arg1 string `json:"map"` + Arg2 int `json:"nomap"` + }{"1", 1} + dest := struct { + Arg1 string `json:"map"` + Arg2 int `json:"nomap"` + }{} + + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), []string{"map"}) + assert.Equal(t, src.Arg1, dest.Arg1) + assert.NotEqual(t, src.Arg2, dest.Arg2) +} From 1e6774b0adb9fe738bbaa43d392300928face5eb Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Wed, 22 Mar 2023 11:01:15 +0100 Subject: [PATCH 18/37] fix todo --- internal/editor/editor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index 782a4561ea..dbcbc74b46 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -103,8 +103,8 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, cfg * updateRequestEdited := newRequest(updateRequest) // TODO: if !putRequest - // TODO: fill updateRequestEdited with only edited fields and fields present in updateRequest - // TODO: fields should be compared with completeUpdateRequest to find edited ones + // fill updateRequestEdited with only edited fields and fields present in updateRequest + // fields should be compared with completeUpdateRequest to find edited ones // Add back required non-marshaled fields (zone, ID) copyRequestPathParameters(updateRequestEdited, updateRequest) From d36b1c231506dae2c5cc8d0263914b30f3b9e577 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Wed, 22 Mar 2023 11:03:42 +0100 Subject: [PATCH 19/37] add security-group edit command --- internal/namespaces/instance/v1/custom.go | 1 + .../instance/v1/custom_security_group.go | 64 +++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/internal/namespaces/instance/v1/custom.go b/internal/namespaces/instance/v1/custom.go index 1221b5c869..7d1076e958 100644 --- a/internal/namespaces/instance/v1/custom.go +++ b/internal/namespaces/instance/v1/custom.go @@ -142,6 +142,7 @@ func GetCommands() *core.Commands { cmds.Merge(core.NewCommands( securityGroupClearCommand(), securityGroupUpdateCommand(), + securityGroupEditCommand(), )) // diff --git a/internal/namespaces/instance/v1/custom_security_group.go b/internal/namespaces/instance/v1/custom_security_group.go index e30d2bacce..f51b6692b3 100644 --- a/internal/namespaces/instance/v1/custom_security_group.go +++ b/internal/namespaces/instance/v1/custom_security_group.go @@ -10,6 +10,7 @@ import ( "github.com/fatih/color" "github.com/scaleway/scaleway-cli/v2/internal/core" + "github.com/scaleway/scaleway-cli/v2/internal/editor" "github.com/scaleway/scaleway-cli/v2/internal/human" "github.com/scaleway/scaleway-cli/v2/internal/interactive" "github.com/scaleway/scaleway-cli/v2/internal/terminal" @@ -488,6 +489,69 @@ func securityGroupUpdateCommand() *core.Command { } } +type instanceSecurityGroupEditArgs struct { + Zone scw.Zone + SecurityGroupID string +} + +func securityGroupEditCommand() *core.Command { + return &core.Command{ + Short: `Edit all rules of a security group`, + Long: editor.LongDescription, + Namespace: "instance", + Resource: "security-group", + Verb: "edit", + ArgsType: reflect.TypeOf(instanceSecurityGroupEditArgs{}), + ArgSpecs: core.ArgSpecs{ + { + Name: "security-group-id", + Short: `ID of the security group to reset.`, + Required: true, + Positional: true, + }, + core.ZoneArgSpec(), + }, + Run: func(ctx context.Context, argsI interface{}) (i interface{}, e error) { + args := argsI.(*instanceSecurityGroupEditArgs) + + client := core.ExtractClient(ctx) + api := instance.NewAPI(client) + + updateRequest := &instance.SetSecurityGroupRulesRequest{ + Zone: args.Zone, + SecurityGroupID: args.SecurityGroupID, + } + + rules, err := api.ListSecurityGroupRules(&instance.ListSecurityGroupRulesRequest{ + Zone: args.Zone, + SecurityGroupID: args.SecurityGroupID, + }, scw.WithAllPages(), scw.WithContext(ctx)) + if err != nil { + return nil, fmt.Errorf("failed to list security-group rules: %w", err) + } + + // Get only rules that can be edited + editableRules := []*instance.SecurityGroupRule(nil) + for _, rule := range rules.Rules { + if rule.Editable == true { + editableRules = append(editableRules, rule) + } + } + rules.Rules = editableRules + + updateRequestI, err := editor.UpdateResourceEditor(rules, updateRequest, &editor.Config{ + PutRequest: true, + }) + if err != nil { + return nil, err + } + updateRequest = updateRequestI.(*instance.SetSecurityGroupRulesRequest) + + return api.SetSecurityGroupRules(updateRequest, scw.WithContext(ctx)) + }, + } +} + func getDefaultProjectSecurityGroup(ctx context.Context, zone scw.Zone) (*instance.SecurityGroup, error) { api := instance.NewAPI(core.ExtractClient(ctx)) From 2bd0579a70b4ebdfd7a7f4a180a7ac886d3d7adb Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Wed, 22 Mar 2023 11:12:59 +0100 Subject: [PATCH 20/37] add marshal-mode argument --- internal/editor/doc.go | 12 ++++++++++ internal/editor/marshal.go | 9 +++++--- .../instance/v1/custom_security_group.go | 22 +++++++++++-------- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/internal/editor/doc.go b/internal/editor/doc.go index d7c49fdf16..cd3a711c64 100644 --- a/internal/editor/doc.go +++ b/internal/editor/doc.go @@ -1,5 +1,17 @@ package editor +import "github.com/scaleway/scaleway-cli/v2/internal/core" + var LongDescription = `This command starts your default editor to edit a marshaled version of your resource Default editor will be taken from $VISUAL, then $EDITOR or will be 'vi' ` + +func MarshalModeArgSpec() *core.ArgSpec { + return &core.ArgSpec{ + Name: "mode", + Short: "marshaling used when editing data", + Required: false, + Default: core.DefaultValueSetter(MarshalModeDefault), + EnumValues: MarshalModeEnum, + } +} diff --git a/internal/editor/marshal.go b/internal/editor/marshal.go index 0253fcee1f..0cb78c7da8 100644 --- a/internal/editor/marshal.go +++ b/internal/editor/marshal.go @@ -7,16 +7,19 @@ import ( "github.com/ghodss/yaml" ) -type MarshalMode string +type MarshalMode = string const ( MarshalModeYAML = MarshalMode("yaml") MarshalModeJSON = MarshalMode("json") ) +var MarshalModeDefault = MarshalModeYAML +var MarshalModeEnum = []MarshalMode{MarshalModeYAML, MarshalModeJSON} + func marshal(i interface{}, mode MarshalMode) ([]byte, error) { if mode == "" { - mode = MarshalModeYAML + mode = MarshalModeDefault } var marshaledData []byte @@ -40,7 +43,7 @@ func marshal(i interface{}, mode MarshalMode) ([]byte, error) { func unmarshal(data []byte, i interface{}, mode MarshalMode) error { if mode == "" { - mode = MarshalModeYAML + mode = MarshalModeDefault } switch mode { diff --git a/internal/namespaces/instance/v1/custom_security_group.go b/internal/namespaces/instance/v1/custom_security_group.go index f51b6692b3..5152fdc00e 100644 --- a/internal/namespaces/instance/v1/custom_security_group.go +++ b/internal/namespaces/instance/v1/custom_security_group.go @@ -492,6 +492,7 @@ func securityGroupUpdateCommand() *core.Command { type instanceSecurityGroupEditArgs struct { Zone scw.Zone SecurityGroupID string + Mode editor.MarshalMode } func securityGroupEditCommand() *core.Command { @@ -509,6 +510,7 @@ func securityGroupEditCommand() *core.Command { Required: true, Positional: true, }, + editor.MarshalModeArgSpec(), core.ZoneArgSpec(), }, Run: func(ctx context.Context, argsI interface{}) (i interface{}, e error) { @@ -517,11 +519,6 @@ func securityGroupEditCommand() *core.Command { client := core.ExtractClient(ctx) api := instance.NewAPI(client) - updateRequest := &instance.SetSecurityGroupRulesRequest{ - Zone: args.Zone, - SecurityGroupID: args.SecurityGroupID, - } - rules, err := api.ListSecurityGroupRules(&instance.ListSecurityGroupRulesRequest{ Zone: args.Zone, SecurityGroupID: args.SecurityGroupID, @@ -539,15 +536,22 @@ func securityGroupEditCommand() *core.Command { } rules.Rules = editableRules - updateRequestI, err := editor.UpdateResourceEditor(rules, updateRequest, &editor.Config{ - PutRequest: true, + setRequest := &instance.SetSecurityGroupRulesRequest{ + Zone: args.Zone, + SecurityGroupID: args.SecurityGroupID, + } + + editedSetRequest, err := editor.UpdateResourceEditor(rules, setRequest, &editor.Config{ + PutRequest: true, + MarshalMode: args.Mode, }) if err != nil { return nil, err } - updateRequest = updateRequestI.(*instance.SetSecurityGroupRulesRequest) - return api.SetSecurityGroupRules(updateRequest, scw.WithContext(ctx)) + setRequest = editedSetRequest.(*instance.SetSecurityGroupRulesRequest) + + return api.SetSecurityGroupRules(setRequest, scw.WithContext(ctx)) }, } } From 065644e8275383a76ae075d6d423dd41aef2a9e9 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Wed, 22 Mar 2023 11:13:49 +0100 Subject: [PATCH 21/37] update goldens --- ...-instance-security-group-edit-usage.golden | 21 +++++++++++++++++++ ...usage-instance-security-group-usage.golden | 1 + 2 files changed, 22 insertions(+) create mode 100644 cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden diff --git a/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden b/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden new file mode 100644 index 0000000000..4d6b112a2d --- /dev/null +++ b/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden @@ -0,0 +1,21 @@ +🎲🎲🎲 EXIT CODE: 0 🎲🎲🎲 +πŸŸ₯πŸŸ₯πŸŸ₯ STDERR️️ πŸŸ₯πŸŸ₯πŸŸ₯️ +This command starts your default editor to edit a marshaled version of your resource +Default editor will be taken from $VISUAL, then $EDITOR or will be 'vi' + +USAGE: + scw instance security-group edit [arg=value ...] + +ARGS: + security-group-id ID of the security group to reset. + [mode=yaml] marshaling used when editing data (yaml | json) + [zone=fr-par-1] Zone to target. If none is passed will use default zone from the config + +FLAGS: + -h, --help help for edit + +GLOBAL FLAGS: + -c, --config string The path to the config file + -D, --debug Enable debug mode + -o, --output string Output format: json or human, see 'scw help output' for more info (default "human") + -p, --profile string The config profile to use diff --git a/cmd/scw/testdata/test-all-usage-instance-security-group-usage.golden b/cmd/scw/testdata/test-all-usage-instance-security-group-usage.golden index 8081b2705a..b6aff66dc0 100644 --- a/cmd/scw/testdata/test-all-usage-instance-security-group-usage.golden +++ b/cmd/scw/testdata/test-all-usage-instance-security-group-usage.golden @@ -15,6 +15,7 @@ AVAILABLE COMMANDS: create-rule Create rule delete Delete a security group delete-rule Delete rule + edit Edit all rules of a security group get Get a security group get-rule Get rule list List security groups From 36fa633fc38e039cebd2580ec615da3d1c234e90 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Wed, 22 Mar 2023 11:14:20 +0100 Subject: [PATCH 22/37] update doc --- docs/commands/instance.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/docs/commands/instance.md b/docs/commands/instance.md index 621b15b694..1bc119b29c 100644 --- a/docs/commands/instance.md +++ b/docs/commands/instance.md @@ -39,6 +39,7 @@ Instance API. - [Create rule](#create-rule) - [Delete a security group](#delete-a-security-group) - [Delete rule](#delete-rule) + - [Edit all rules of a security group](#edit-all-rules-of-a-security-group) - [Get a security group](#get-a-security-group) - [Get rule](#get-rule) - [List security groups](#list-security-groups) @@ -1292,6 +1293,29 @@ scw instance security-group delete-rule security-group-id=a01a36e5-5c0c-42c1-ae0 +### Edit all rules of a security group + +This command starts your default editor to edit a marshaled version of your resource +Default editor will be taken from $VISUAL, then $EDITOR or will be 'vi' + + +**Usage:** + +``` +scw instance security-group edit [arg=value ...] +``` + + +**Args:** + +| Name | | Description | +|------|---|-------------| +| security-group-id | Required | ID of the security group to reset. | +| mode | Default: `yaml`
One of: `yaml`, `json` | marshaling used when editing data | +| zone | Default: `fr-par-1` | Zone to target. If none is passed will use default zone from the config | + + + ### Get a security group Get the details of a Security Group with the given ID. From 003dace001e777bb5086ac62afc4f89b02baa03a Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Wed, 22 Mar 2023 11:20:20 +0100 Subject: [PATCH 23/37] linter --- internal/namespaces/instance/v1/custom_security_group.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/namespaces/instance/v1/custom_security_group.go b/internal/namespaces/instance/v1/custom_security_group.go index 5152fdc00e..8723c4cb0c 100644 --- a/internal/namespaces/instance/v1/custom_security_group.go +++ b/internal/namespaces/instance/v1/custom_security_group.go @@ -530,7 +530,7 @@ func securityGroupEditCommand() *core.Command { // Get only rules that can be edited editableRules := []*instance.SecurityGroupRule(nil) for _, rule := range rules.Rules { - if rule.Editable == true { + if rule.Editable { editableRules = append(editableRules, rule) } } From 76202e56e2e04057ce70ad10944fd51cf9e2addb Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 09:31:05 +0100 Subject: [PATCH 24/37] add default editor for windows --- internal/config/editor.go | 16 ++++++++++++++-- internal/editor/doc.go | 12 ++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/internal/config/editor.go b/internal/config/editor.go index 916055e991..b876813adf 100644 --- a/internal/config/editor.go +++ b/internal/config/editor.go @@ -1,6 +1,9 @@ package config -import "os" +import ( + "os" + "runtime" +) var ( // List of env variables where to find the editor to use @@ -8,6 +11,15 @@ var ( editorEnvVariables = []string{"EDITOR", "VISUAL"} ) +func GetSystemDefaultEditor() string { + switch runtime.GOOS { + case "windows": + return "notepad" + default: + return "vi" + } +} + func GetDefaultEditor() string { editor := "" for _, envVar := range editorEnvVariables { @@ -18,7 +30,7 @@ func GetDefaultEditor() string { } if editor == "" { - return "vi" + return GetSystemDefaultEditor() } return editor diff --git a/internal/editor/doc.go b/internal/editor/doc.go index cd3a711c64..7c0fa6b126 100644 --- a/internal/editor/doc.go +++ b/internal/editor/doc.go @@ -1,10 +1,14 @@ package editor -import "github.com/scaleway/scaleway-cli/v2/internal/core" +import ( + "fmt" -var LongDescription = `This command starts your default editor to edit a marshaled version of your resource -Default editor will be taken from $VISUAL, then $EDITOR or will be 'vi' -` + "github.com/scaleway/scaleway-cli/v2/internal/config" + "github.com/scaleway/scaleway-cli/v2/internal/core" +) + +var LongDescription = fmt.Sprintf(`This command starts your default editor to edit a marshaled version of your resource +Default editor will be taken from $VISUAL, then $EDITOR or will be %q`, config.GetDefaultEditor()) func MarshalModeArgSpec() *core.ArgSpec { return &core.ArgSpec{ From eeab091ac1bf37bf2e75e3bb05147f082a01d970 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 12:06:56 +0100 Subject: [PATCH 25/37] add examples when editing --- internal/editor/editor.go | 7 +++++++ internal/editor/marshal.go | 16 ++++++++++++++++ .../instance/v1/custom_security_group.go | 19 +++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index dbcbc74b46..6003c830e3 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -21,6 +21,9 @@ type Config struct { MarshalMode MarshalMode + // Template is a template that will be shown before marshaled data in edited file + Template string + // If not empty, this will replace edited text as if it was edited in the terminal // Should be paired with global SkipEditor as true, useful for tests editedResource string @@ -86,6 +89,10 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, cfg * return nil, fmt.Errorf("failed to marshal update request: %w", err) } + if cfg.Template != "" { + updateRequestMarshaled = addTemplate(updateRequestMarshaled, cfg.Template, cfg.MarshalMode) + } + // Start text editor to edit marshaled request updateRequestMarshaled, err = edit(updateRequestMarshaled) if err != nil { diff --git a/internal/editor/marshal.go b/internal/editor/marshal.go index 0cb78c7da8..8da7095253 100644 --- a/internal/editor/marshal.go +++ b/internal/editor/marshal.go @@ -3,6 +3,7 @@ package editor import ( "encoding/json" "fmt" + "strings" "github.com/ghodss/yaml" ) @@ -55,3 +56,18 @@ func unmarshal(data []byte, i interface{}, mode MarshalMode) error { return fmt.Errorf("unknown marshal mode %q", mode) } + +func addTemplate(content []byte, template string, mode MarshalMode) []byte { + if mode != MarshalModeYAML || len(template) == 0 { + return content + } + newContent := []byte(nil) + + for _, line := range strings.Split(template, "\n") { + newContent = append(newContent, []byte("#"+line+"\n")...) + } + + newContent = append(newContent, content...) + + return newContent +} diff --git a/internal/namespaces/instance/v1/custom_security_group.go b/internal/namespaces/instance/v1/custom_security_group.go index 8723c4cb0c..cdf4856a2e 100644 --- a/internal/namespaces/instance/v1/custom_security_group.go +++ b/internal/namespaces/instance/v1/custom_security_group.go @@ -489,6 +489,24 @@ func securityGroupUpdateCommand() *core.Command { } } +var instanceSecurityGroupEditYamlExample = ` +- action: drop + dest_port_from: 1200 + dest_port_to: 1300 + direction: inbound + ip_range: 192.168.0.0/24 + protocol: TCP +- action: drop + direction: inbound + protocol: ICMP + ip_range: 0.0.0.0/0 +- action: accept + dest_port_from: 25565 + direction: outbound + ip_range: 0.0.0.0/0 + protocol: UDP +` + type instanceSecurityGroupEditArgs struct { Zone scw.Zone SecurityGroupID string @@ -544,6 +562,7 @@ func securityGroupEditCommand() *core.Command { editedSetRequest, err := editor.UpdateResourceEditor(rules, setRequest, &editor.Config{ PutRequest: true, MarshalMode: args.Mode, + Template: instanceSecurityGroupEditYamlExample, }) if err != nil { return nil, err From dda30deaedf5f8121730afc6737f6ff615f217c4 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 12:08:56 +0100 Subject: [PATCH 26/37] add rules in example --- internal/namespaces/instance/v1/custom_security_group.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/namespaces/instance/v1/custom_security_group.go b/internal/namespaces/instance/v1/custom_security_group.go index cdf4856a2e..166ede5615 100644 --- a/internal/namespaces/instance/v1/custom_security_group.go +++ b/internal/namespaces/instance/v1/custom_security_group.go @@ -489,7 +489,7 @@ func securityGroupUpdateCommand() *core.Command { } } -var instanceSecurityGroupEditYamlExample = ` +var instanceSecurityGroupEditYamlExample = `rules: - action: drop dest_port_from: 1200 dest_port_to: 1300 From 22fee65be531e2e9be92d8701e83d617f971997a Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 14:16:26 +0100 Subject: [PATCH 27/37] change reflect valueMapper arguments to options --- internal/editor/reflect.go | 49 ++++++++++++++++++++++++++------- internal/editor/reflect_test.go | 18 ++++++------ internal/editor/request.go | 8 +++--- 3 files changed, 52 insertions(+), 23 deletions(-) diff --git a/internal/editor/reflect.go b/internal/editor/reflect.go index 4c59a08651..a18cba1d58 100644 --- a/internal/editor/reflect.go +++ b/internal/editor/reflect.go @@ -25,7 +25,7 @@ func areSameType(v1 reflect.Value, v2 reflect.Value) bool { v2t = v2t.Elem() } - // If both are struct consider them equal, valueMapper will try to map fields + // If both are struct consider them equal, valueMapperWithoutOpt will try to map fields if v1t.Kind() == reflect.Struct && v2t.Kind() == reflect.Struct { return true } @@ -42,10 +42,7 @@ func hasTag(tags []string, actualTag string) bool { return false } -// valueMapper get all fields present both in src and dest and set them in dest -// if argument is not zero-value in dest, it is not set -// fields is a list of jsonTags, if not nil, only fields with a tag in this list will be mapped -func valueMapper(dest reflect.Value, src reflect.Value, fields []string) { +func valueMapperWithoutOpt(dest reflect.Value, src reflect.Value, includeFields []string, excludeFields []string) { switch dest.Kind() { case reflect.Struct: for i := 0; i < dest.NumField(); i++ { @@ -54,7 +51,8 @@ func valueMapper(dest reflect.Value, src reflect.Value, fields []string) { srcField := src.FieldByName(fieldType.Name) // If field is not in list, do not set it - if fields != nil && !hasTag(fields, fieldType.Tag.Get("json")) { + if includeFields != nil && !hasTag(includeFields, fieldType.Tag.Get("json")) || + excludeFields != nil && hasTag(excludeFields, fieldType.Tag.Get("json")) { continue } @@ -63,7 +61,7 @@ func valueMapper(dest reflect.Value, src reflect.Value, fields []string) { continue } - valueMapper(destField, srcField, fields) + valueMapperWithoutOpt(destField, srcField, includeFields, excludeFields) } case reflect.Pointer: // If destination is a pointer, we allocate destination if needed @@ -76,18 +74,49 @@ func valueMapper(dest reflect.Value, src reflect.Value, fields []string) { } dest = dest.Elem() - valueMapper(dest, src, fields) + valueMapperWithoutOpt(dest, src, includeFields, excludeFields) // TODO: clean pointer if not filled - // If dest is nil and src is a struct with fields disabled because of json tags + // If dest is nil and src is a struct with includeFields disabled because of json tags // Then dest should not be allocated and should remain nil case reflect.Slice: // If destination is a slice, allocate the slice and map each value srcLen := src.Len() dest.Set(reflect.MakeSlice(dest.Type(), srcLen, srcLen)) for i := 0; i < srcLen; i++ { - valueMapper(dest.Index(i), src.Index(i), fields) + valueMapperWithoutOpt(dest.Index(i), src.Index(i), includeFields, excludeFields) } default: dest.Set(src) } } + +type valueMapperConfig struct { + includeFields []string + excludeFields []string +} +type valueMapperOpt func(cfg *valueMapperConfig) + +// mapWithTag will map only fields that have one of these tags as json tag +func mapWithTag(includeFields ...string) valueMapperOpt { + return func(cfg *valueMapperConfig) { + cfg.includeFields = append(cfg.includeFields, includeFields...) + } +} + +// mapWithTag will map only fields that don't have one of these tags as json tag +func mapWithoutTag(excludeFields ...string) valueMapperOpt { + return func(cfg *valueMapperConfig) { + cfg.excludeFields = append(cfg.excludeFields, excludeFields...) + } +} + +// valueMapper get all fields present both in src and dest and set them in dest +// if argument is not zero-value in dest, it is not set +// fields is a list of jsonTags, if not nil, only fields with a tag in this list will be mapped +func valueMapper(dest reflect.Value, src reflect.Value, opts ...valueMapperOpt) { + cfg := valueMapperConfig{} + for _, opt := range opts { + opt(&cfg) + } + valueMapperWithoutOpt(dest, src, cfg.includeFields, cfg.excludeFields) +} diff --git a/internal/editor/reflect_test.go b/internal/editor/reflect_test.go index 096e294651..417b32f7fb 100644 --- a/internal/editor/reflect_test.go +++ b/internal/editor/reflect_test.go @@ -20,7 +20,7 @@ func Test_valueMapper(t *testing.T) { Arg2 int }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.Equal(t, src.Arg1, dest.Arg1) assert.Equal(t, src.Arg2, dest.Arg2) } @@ -35,7 +35,7 @@ func Test_valueMapperInvalidType(t *testing.T) { Arg2 bool }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.Equal(t, src.Arg1, dest.Arg1) assert.Zero(t, dest.Arg2) } @@ -50,7 +50,7 @@ func Test_valueMapperDifferentFields(t *testing.T) { Arg4 bool }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.Zero(t, dest.Arg3) assert.Zero(t, dest.Arg4) } @@ -65,7 +65,7 @@ func Test_valueMapperPointers(t *testing.T) { Arg2 *int }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.NotNil(t, dest.Arg1) assert.EqualValues(t, src.Arg1, *dest.Arg1) assert.NotNil(t, dest.Arg2) @@ -82,7 +82,7 @@ func Test_valueMapperPointersWithPointers(t *testing.T) { Arg2 *int32 }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.NotNil(t, dest.Arg1) assert.EqualValues(t, src.Arg1, dest.Arg1) assert.NotNil(t, dest.Arg2) @@ -102,7 +102,7 @@ func Test_valueMapperSlice(t *testing.T) { Arg2 []int }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.NotNil(t, dest.Arg1) assert.EqualValues(t, src.Arg1, dest.Arg1) assert.NotNil(t, dest.Arg2) @@ -122,7 +122,7 @@ func Test_valueMapperSliceOfPointers(t *testing.T) { Arg2 []*int }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.NotNil(t, dest.Arg1) assert.Equal(t, len(src.Arg1), len(dest.Arg1)) for i := range src.Arg1 { @@ -165,7 +165,7 @@ func Test_valueMapperSliceStructPointer(t *testing.T) { Rules: nil, } - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), nil) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src)) assert.NotNil(t, dest.Rules) assert.Equal(t, 1, len(dest.Rules)) expectedRule := src.Rules[0] @@ -197,7 +197,7 @@ func Test_valueMapperTags(t *testing.T) { Arg2 int `json:"nomap"` }{} - valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), []string{"map"}) + valueMapper(reflect.ValueOf(&dest), reflect.ValueOf(&src), mapWithTag("map")) assert.Equal(t, src.Arg1, dest.Arg1) assert.NotEqual(t, src.Arg2, dest.Arg2) } diff --git a/internal/editor/request.go b/internal/editor/request.go index e203d99c35..f3b43e78b2 100644 --- a/internal/editor/request.go +++ b/internal/editor/request.go @@ -11,7 +11,7 @@ func createGetRequest(updateRequest interface{}, getRequestType reflect.Type) in // Fill GetRequest args using Update arg content // This should copy important argument like ID, zone - valueMapper(getRequestV, updateRequestV, nil) + valueMapper(getRequestV, updateRequestV) return getRequest } @@ -24,8 +24,8 @@ func copyAndCompleteUpdateRequest(updateRequest interface{}, resource interface{ // Create a new updateRequest that will be edited // It will allow user to edit it, then we will extract diff to perform update newUpdateRequestV := reflect.New(updateRequestV.Type().Elem()) - valueMapper(newUpdateRequestV, updateRequestV, nil) - valueMapper(newUpdateRequestV, resourceV, nil) + valueMapper(newUpdateRequestV, updateRequestV) + valueMapper(newUpdateRequestV, resourceV) return newUpdateRequestV.Interface() } @@ -42,5 +42,5 @@ func newRequest(request interface{}) interface{} { // copyRequestPathParameters will copy all path parameters present in src to their correct fields in dest func copyRequestPathParameters(dest interface{}, src interface{}) { - valueMapper(reflect.ValueOf(dest), reflect.ValueOf(src), []string{"-"}) + valueMapper(reflect.ValueOf(dest), reflect.ValueOf(src), mapWithTag("-")) } From c8e0dfa9b6be938f9d65ef5d1a8259d47e56ac66 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 14:40:42 +0100 Subject: [PATCH 28/37] add ignoredFields --- internal/editor/editor.go | 11 ++++++ internal/editor/marshal.go | 12 +++++++ internal/editor/reflect.go | 23 ++++++++++++ internal/editor/reflect_test.go | 35 +++++++++++++++++++ .../instance/v1/custom_security_group.go | 7 ++-- 5 files changed, 85 insertions(+), 3 deletions(-) diff --git a/internal/editor/editor.go b/internal/editor/editor.go index 6003c830e3..9cdf415c0e 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -24,6 +24,10 @@ type Config struct { // Template is a template that will be shown before marshaled data in edited file Template string + // IgnoreFields is a list of json tags that will be removed from marshaled data + // The content of these fields will be lost in edited data + IgnoreFields []string + // If not empty, this will replace edited text as if it was edited in the terminal // Should be paired with global SkipEditor as true, useful for tests editedResource string @@ -89,6 +93,13 @@ func updateResourceEditor(resource interface{}, updateRequest interface{}, cfg * return nil, fmt.Errorf("failed to marshal update request: %w", err) } + if len(cfg.IgnoreFields) > 0 { + updateRequestMarshaled, err = removeFields(updateRequestMarshaled, cfg.MarshalMode, cfg.IgnoreFields) + if err != nil { + return nil, fmt.Errorf("failed to remove ignored fields: %w", err) + } + } + if cfg.Template != "" { updateRequestMarshaled = addTemplate(updateRequestMarshaled, cfg.Template, cfg.MarshalMode) } diff --git a/internal/editor/marshal.go b/internal/editor/marshal.go index 8da7095253..e04e3d284d 100644 --- a/internal/editor/marshal.go +++ b/internal/editor/marshal.go @@ -57,6 +57,18 @@ func unmarshal(data []byte, i interface{}, mode MarshalMode) error { return fmt.Errorf("unknown marshal mode %q", mode) } +// removeFields remove some fields from marshaled data +func removeFields(data []byte, mode MarshalMode, fields []string) ([]byte, error) { + i := map[string]interface{}{} + err := unmarshal(data, &i, mode) + if err != nil { + return nil, fmt.Errorf("failed to unmarshal: %w", err) + } + deleteRecursive(i, fields...) + + return marshal(i, mode) +} + func addTemplate(content []byte, template string, mode MarshalMode) []byte { if mode != MarshalModeYAML || len(template) == 0 { return content diff --git a/internal/editor/reflect.go b/internal/editor/reflect.go index a18cba1d58..9bcff19d2f 100644 --- a/internal/editor/reflect.go +++ b/internal/editor/reflect.go @@ -120,3 +120,26 @@ func valueMapper(dest reflect.Value, src reflect.Value, opts ...valueMapperOpt) } valueMapperWithoutOpt(dest, src, cfg.includeFields, cfg.excludeFields) } + +func deleteRecursiveMap(m map[string]interface{}, keys ...string) { + for _, key := range keys { + delete(m, key) + } + + for _, val := range m { + deleteRecursive(val, keys...) + } +} + +func deleteRecursive(elem interface{}, keys ...string) { + value := reflect.ValueOf(elem) + + switch value.Kind() { + case reflect.Map: + deleteRecursiveMap(elem.(map[string]interface{}), keys...) + case reflect.Slice: + for i := 0; i < value.Len(); i++ { + deleteRecursive(value.Index(i).Interface(), keys...) + } + } +} diff --git a/internal/editor/reflect_test.go b/internal/editor/reflect_test.go index 417b32f7fb..d6df41f9f5 100644 --- a/internal/editor/reflect_test.go +++ b/internal/editor/reflect_test.go @@ -201,3 +201,38 @@ func Test_valueMapperTags(t *testing.T) { assert.Equal(t, src.Arg1, dest.Arg1) assert.NotEqual(t, src.Arg2, dest.Arg2) } + +func Test_deleteRecursive(t *testing.T) { + m := map[string]interface{}{ + "delete": "1", + "nodelete": 1, + } + + deleteRecursive(m, "delete") + + _, deleteExists := m["delete"] + _, nodeleteExists := m["nodelete"] + + assert.False(t, deleteExists) + assert.True(t, nodeleteExists) +} + +func Test_deleteRecursiveSlice(t *testing.T) { + m := map[string]interface{}{ + "slice": []map[string]interface{}{ + { + "delete": "1", + "nodelete": 1, + }, + }, + } + + deleteRecursive(m, "delete") + + slice := m["slice"].([]map[string]interface{}) + _, deleteExists := slice[0]["delete"] + _, nodeleteExists := slice[0]["nodelete"] + + assert.False(t, deleteExists) + assert.True(t, nodeleteExists) +} diff --git a/internal/namespaces/instance/v1/custom_security_group.go b/internal/namespaces/instance/v1/custom_security_group.go index 166ede5615..10222320b5 100644 --- a/internal/namespaces/instance/v1/custom_security_group.go +++ b/internal/namespaces/instance/v1/custom_security_group.go @@ -560,9 +560,10 @@ func securityGroupEditCommand() *core.Command { } editedSetRequest, err := editor.UpdateResourceEditor(rules, setRequest, &editor.Config{ - PutRequest: true, - MarshalMode: args.Mode, - Template: instanceSecurityGroupEditYamlExample, + PutRequest: true, + MarshalMode: args.Mode, + Template: instanceSecurityGroupEditYamlExample, + IgnoreFields: []string{"editable"}, }) if err != nil { return nil, err From 9259502a17ac9401fcd68e6a38442d1e84254636 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 15:46:06 +0100 Subject: [PATCH 29/37] improve security-group edit output --- internal/namespaces/instance/v1/custom_security_group.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/namespaces/instance/v1/custom_security_group.go b/internal/namespaces/instance/v1/custom_security_group.go index 10222320b5..d2383b04ba 100644 --- a/internal/namespaces/instance/v1/custom_security_group.go +++ b/internal/namespaces/instance/v1/custom_security_group.go @@ -571,7 +571,12 @@ func securityGroupEditCommand() *core.Command { setRequest = editedSetRequest.(*instance.SetSecurityGroupRulesRequest) - return api.SetSecurityGroupRules(setRequest, scw.WithContext(ctx)) + resp, err := api.SetSecurityGroupRules(setRequest, scw.WithContext(ctx)) + if err != nil { + return nil, err + } + + return resp.Rules, nil }, } } From 71525d96395c23840397b3443936a4470a01bc04 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 15:47:29 +0100 Subject: [PATCH 30/37] fix editor doc with correct system default editor --- internal/editor/doc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/editor/doc.go b/internal/editor/doc.go index 7c0fa6b126..20059af26a 100644 --- a/internal/editor/doc.go +++ b/internal/editor/doc.go @@ -8,7 +8,7 @@ import ( ) var LongDescription = fmt.Sprintf(`This command starts your default editor to edit a marshaled version of your resource -Default editor will be taken from $VISUAL, then $EDITOR or will be %q`, config.GetDefaultEditor()) +Default editor will be taken from $VISUAL, then $EDITOR or will be %q`, config.GetSystemDefaultEditor()) func MarshalModeArgSpec() *core.ArgSpec { return &core.ArgSpec{ From ece5527d03e3bf9e8a0fa78d2ea86cc309c4011c Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 15:50:32 +0100 Subject: [PATCH 31/37] fix linter --- internal/editor/reflect.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/editor/reflect.go b/internal/editor/reflect.go index 9bcff19d2f..10747f9368 100644 --- a/internal/editor/reflect.go +++ b/internal/editor/reflect.go @@ -104,6 +104,8 @@ func mapWithTag(includeFields ...string) valueMapperOpt { } // mapWithTag will map only fields that don't have one of these tags as json tag +// +//nolint:deadcode func mapWithoutTag(excludeFields ...string) valueMapperOpt { return func(cfg *valueMapperConfig) { cfg.excludeFields = append(cfg.excludeFields, excludeFields...) From d192c945c805aa93ff8bbb9579b628b63038a7a8 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 15:51:21 +0100 Subject: [PATCH 32/37] update golden --- .../test-all-usage-instance-security-group-edit-usage.golden | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden b/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden index 4d6b112a2d..15c0ff7be4 100644 --- a/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden +++ b/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden @@ -1,7 +1,7 @@ 🎲🎲🎲 EXIT CODE: 0 🎲🎲🎲 πŸŸ₯πŸŸ₯πŸŸ₯ STDERR️️ πŸŸ₯πŸŸ₯πŸŸ₯️ This command starts your default editor to edit a marshaled version of your resource -Default editor will be taken from $VISUAL, then $EDITOR or will be 'vi' +Default editor will be taken from $VISUAL, then $EDITOR or will be "vi" USAGE: scw instance security-group edit [arg=value ...] From b94fdee887e51d2b62f54eefef85017162c594c7 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 15:52:48 +0100 Subject: [PATCH 33/37] gen doc --- docs/commands/instance.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/commands/instance.md b/docs/commands/instance.md index 1bc119b29c..c1b046d188 100644 --- a/docs/commands/instance.md +++ b/docs/commands/instance.md @@ -1296,8 +1296,7 @@ scw instance security-group delete-rule security-group-id=a01a36e5-5c0c-42c1-ae0 ### Edit all rules of a security group This command starts your default editor to edit a marshaled version of your resource -Default editor will be taken from $VISUAL, then $EDITOR or will be 'vi' - +Default editor will be taken from $VISUAL, then $EDITOR or will be "vi" **Usage:** From 4d6f4d77ee0c42757027cf7151c4903ed30d9e2b Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 15:53:21 +0100 Subject: [PATCH 34/37] fix linter --- internal/editor/reflect.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/editor/reflect.go b/internal/editor/reflect.go index 10747f9368..d083696250 100644 --- a/internal/editor/reflect.go +++ b/internal/editor/reflect.go @@ -105,7 +105,7 @@ func mapWithTag(includeFields ...string) valueMapperOpt { // mapWithTag will map only fields that don't have one of these tags as json tag // -//nolint:deadcode +//nolint:deadcode,unused func mapWithoutTag(excludeFields ...string) valueMapperOpt { return func(cfg *valueMapperConfig) { cfg.excludeFields = append(cfg.excludeFields, excludeFields...) From 246f825f07600df39ca3154d17a2435beada29d9 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 16:17:44 +0100 Subject: [PATCH 35/37] fix edit test on windows --- .../namespaces/instance/v1/custom_security_group_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/namespaces/instance/v1/custom_security_group_test.go b/internal/namespaces/instance/v1/custom_security_group_test.go index 3fe3508451..1ad7a0669a 100644 --- a/internal/namespaces/instance/v1/custom_security_group_test.go +++ b/internal/namespaces/instance/v1/custom_security_group_test.go @@ -1,6 +1,7 @@ package instance import ( + "regexp" "testing" "github.com/scaleway/scaleway-cli/v2/internal/core" @@ -14,7 +15,11 @@ func Test_SecurityGroupGet(t *testing.T) { ), Cmd: "scw instance security-group get {{ .SecurityGroup.ID }}", Check: core.TestCheckCombine( - core.TestCheckGolden(), + core.TestCheckGoldenAndReplacePatterns(core.GoldenReplacement{ + // For windows tests + Pattern: regexp.MustCompile("notepad|vi"), + Replacement: "vi", + }), core.TestCheckExitCode(0), ), AfterFunc: deleteSecurityGroup("SecurityGroup"), From 041846b3a7ab2fb53fe13c96b04ab18ea45c6a86 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 16:26:34 +0100 Subject: [PATCH 36/37] revert changes on wrong test --- .../namespaces/instance/v1/custom_security_group_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/internal/namespaces/instance/v1/custom_security_group_test.go b/internal/namespaces/instance/v1/custom_security_group_test.go index 1ad7a0669a..3fe3508451 100644 --- a/internal/namespaces/instance/v1/custom_security_group_test.go +++ b/internal/namespaces/instance/v1/custom_security_group_test.go @@ -1,7 +1,6 @@ package instance import ( - "regexp" "testing" "github.com/scaleway/scaleway-cli/v2/internal/core" @@ -15,11 +14,7 @@ func Test_SecurityGroupGet(t *testing.T) { ), Cmd: "scw instance security-group get {{ .SecurityGroup.ID }}", Check: core.TestCheckCombine( - core.TestCheckGoldenAndReplacePatterns(core.GoldenReplacement{ - // For windows tests - Pattern: regexp.MustCompile("notepad|vi"), - Replacement: "vi", - }), + core.TestCheckGolden(), core.TestCheckExitCode(0), ), AfterFunc: deleteSecurityGroup("SecurityGroup"), From 93f28afa584eb81d6a6a46e5cb52c5331c6fe0a6 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Fri, 24 Mar 2023 16:28:39 +0100 Subject: [PATCH 37/37] change edit help --- ...est-all-usage-instance-security-group-edit-usage.golden | 2 +- docs/commands/instance.md | 2 +- internal/editor/doc.go | 7 ++----- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden b/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden index 15c0ff7be4..f76f642996 100644 --- a/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden +++ b/cmd/scw/testdata/test-all-usage-instance-security-group-edit-usage.golden @@ -1,7 +1,7 @@ 🎲🎲🎲 EXIT CODE: 0 🎲🎲🎲 πŸŸ₯πŸŸ₯πŸŸ₯ STDERR️️ πŸŸ₯πŸŸ₯πŸŸ₯️ This command starts your default editor to edit a marshaled version of your resource -Default editor will be taken from $VISUAL, then $EDITOR or will be "vi" +Default editor will be taken from $VISUAL, then $EDITOR or an editor based on your system USAGE: scw instance security-group edit [arg=value ...] diff --git a/docs/commands/instance.md b/docs/commands/instance.md index c1b046d188..1ba75c1831 100644 --- a/docs/commands/instance.md +++ b/docs/commands/instance.md @@ -1296,7 +1296,7 @@ scw instance security-group delete-rule security-group-id=a01a36e5-5c0c-42c1-ae0 ### Edit all rules of a security group This command starts your default editor to edit a marshaled version of your resource -Default editor will be taken from $VISUAL, then $EDITOR or will be "vi" +Default editor will be taken from $VISUAL, then $EDITOR or an editor based on your system **Usage:** diff --git a/internal/editor/doc.go b/internal/editor/doc.go index 20059af26a..bbcc517d9b 100644 --- a/internal/editor/doc.go +++ b/internal/editor/doc.go @@ -1,14 +1,11 @@ package editor import ( - "fmt" - - "github.com/scaleway/scaleway-cli/v2/internal/config" "github.com/scaleway/scaleway-cli/v2/internal/core" ) -var LongDescription = fmt.Sprintf(`This command starts your default editor to edit a marshaled version of your resource -Default editor will be taken from $VISUAL, then $EDITOR or will be %q`, config.GetSystemDefaultEditor()) +var LongDescription = `This command starts your default editor to edit a marshaled version of your resource +Default editor will be taken from $VISUAL, then $EDITOR or an editor based on your system` func MarshalModeArgSpec() *core.ArgSpec { return &core.ArgSpec{