From 0b183e9e43bfebec26203273dfefd2818fb0e1d1 Mon Sep 17 00:00:00 2001 From: Jinghan Ying Date: Thu, 18 Nov 2021 12:27:08 +0800 Subject: [PATCH 1/3] refactor(pkg/oomstore): use GroupName in ImportOpt --- pkg/oomstore/import.go | 19 ++++++++++--------- pkg/oomstore/types/options.go | 2 +- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/pkg/oomstore/import.go b/pkg/oomstore/import.go index 25a68c394..1e62977aa 100644 --- a/pkg/oomstore/import.go +++ b/pkg/oomstore/import.go @@ -42,20 +42,21 @@ func stringSliceEqual(a, b []string) bool { } func (s *OomStore) Import(ctx context.Context, opt types.ImportOpt) (int, error) { - // get columns of the group - features := s.metadata.ListFeature(ctx, metadata.ListFeatureOpt{GroupID: &opt.GroupID}) - if features == nil { - return 0, fmt.Errorf("no features under group id: '%d'", opt.GroupID) - } - - // get entity info - group, err := s.GetGroup(ctx, opt.GroupID) + group, err := s.metadata.GetGroupByName(ctx, opt.GroupName) if err != nil { return 0, err } + + features := s.metadata.ListFeature(ctx, metadata.ListFeatureOpt{ + GroupID: &group.ID, + }) + if features == nil { + return 0, fmt.Errorf("no features under group: %s", opt.GroupName) + } + entity := group.Entity if entity == nil { - return 0, fmt.Errorf("no entity found by group id: '%d'", opt.GroupID) + return 0, fmt.Errorf("no entity found by group: %s", opt.GroupName) } // make sure csv data source has all defined columns diff --git a/pkg/oomstore/types/options.go b/pkg/oomstore/types/options.go index 79fbcb147..f14da9885 100644 --- a/pkg/oomstore/types/options.go +++ b/pkg/oomstore/types/options.go @@ -41,7 +41,7 @@ type ExportFeatureValuesOpt struct { } type ImportOpt struct { - GroupID int + GroupName string Description string DataSource CsvDataSource Revision *int64 From ff6f68eba275da4f74d183ba1aee90b8a9bde98d Mon Sep 17 00:00:00 2001 From: Jinghan Ying Date: Thu, 18 Nov 2021 12:27:34 +0800 Subject: [PATCH 2/3] test(pkg/oomstore): fix unit tests for Import --- pkg/oomstore/import_test.go | 80 ++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 46 deletions(-) diff --git a/pkg/oomstore/import_test.go b/pkg/oomstore/import_test.go index 1e2f90bd2..9960d8852 100644 --- a/pkg/oomstore/import_test.go +++ b/pkg/oomstore/import_test.go @@ -31,43 +31,39 @@ func TestImportWithDependencyError(t *testing.T) { wantError error }{ { - description: "ListFeature failed", - opt: types.ImportOpt{GroupID: 1}, + description: "GetGroup failed", + opt: types.ImportOpt{ + GroupName: "device_info", + }, mockFunc: func() { - metadataStore.EXPECT(). - ListFeature(gomock.Any(), metadata.ListFeatureOpt{GroupID: intPtr(1)}). - Return(nil) + metadataStore.EXPECT().GetGroupByName(gomock.Any(), "device_info").Return(nil, fmt.Errorf("error")) }, wantRevisionID: 0, - wantError: fmt.Errorf("no features under group id: '1'"), + wantError: fmt.Errorf("error"), }, { - description: "GetGroup failed", - opt: types.ImportOpt{GroupID: 1}, + description: "ListFeature failed", + opt: types.ImportOpt{ + GroupName: "device_info", + }, mockFunc: func() { - metadataStore.EXPECT(). - ListFeature(gomock.Any(), metadata.ListFeatureOpt{GroupID: intPtr(1)}). - Return(types.FeatureList{}) - metadataStore.EXPECT(). - GetGroup(gomock.Any(), 1). - Return(nil, fmt.Errorf("error")) + metadataStore.EXPECT().GetGroupByName(gomock.Any(), "device_info").Return(&types.Group{ID: 1}, nil) + metadataStore.EXPECT().ListFeature(gomock.Any(), metadata.ListFeatureOpt{GroupID: intPtr(1)}).Return(nil) }, wantRevisionID: 0, - wantError: fmt.Errorf("error"), + wantError: fmt.Errorf("no features under group: device_info"), }, { description: "GetEntity failed", - opt: types.ImportOpt{GroupID: 1}, + opt: types.ImportOpt{ + GroupName: "device_info", + }, mockFunc: func() { - metadataStore.EXPECT(). - ListFeature(gomock.Any(), gomock.Any()). - Return(types.FeatureList{}) - metadataStore.EXPECT(). - GetGroup(gomock.Any(), 1). - Return(&types.Group{ID: 1, EntityID: 1}, nil) + metadataStore.EXPECT().GetGroupByName(gomock.Any(), "device_info").Return(&types.Group{ID: 1, EntityID: 1}, nil) + metadataStore.EXPECT().ListFeature(gomock.Any(), gomock.Any()).Return(types.FeatureList{}) }, wantRevisionID: 0, - wantError: fmt.Errorf("no entity found by group id: '1'"), + wantError: fmt.Errorf("no entity found by group: device_info"), }, { description: "Create Revision failed", @@ -80,11 +76,11 @@ device,model,price `), Delimiter: ",", }, - GroupID: 1, + GroupName: "device_info", }, mockFunc: func() { - metadataStore.EXPECT(). - ListFeature(gomock.Any(), metadata.ListFeatureOpt{GroupID: intPtr(1)}). + metadataStore.EXPECT().GetGroupByName(gomock.Any(), "device_info").Return(&types.Group{ID: 1, EntityID: 1, Entity: &types.Entity{Name: "device"}}, nil) + metadataStore.EXPECT().ListFeature(gomock.Any(), metadata.ListFeatureOpt{GroupID: intPtr(1)}). Return(types.FeatureList{ { Name: "model", @@ -93,17 +89,9 @@ device,model,price Name: "price", }, }) - metadataStore.EXPECT(). - GetGroup(gomock.Any(), 1). - Return(&types.Group{ID: 1, EntityID: 1, Entity: &types.Entity{Name: "device"}}, nil) - offlineStore. - EXPECT(). - Import(gomock.Any(), gomock.Any()). - AnyTimes().Return(int64(1), nil) + offlineStore.EXPECT().Import(gomock.Any(), gomock.Any()).AnyTimes().Return(int64(1), nil) - metadataStore.EXPECT(). - CreateRevision(gomock.Any(), gomock.Any()). - Return(0, "", fmt.Errorf("error")) + metadataStore.EXPECT().CreateRevision(gomock.Any(), gomock.Any()).Return(0, "", fmt.Errorf("error")) }, wantRevisionID: 0, wantError: fmt.Errorf("error"), @@ -114,7 +102,7 @@ device,model,price t.Run(tc.description, func(t *testing.T) { tc.mockFunc() revisionID, err := store.Import(context.Background(), tc.opt) - assert.Equal(t, tc.wantError, err) + assert.EqualError(t, err, tc.wantError.Error()) assert.Equal(t, tc.wantRevisionID, revisionID) }) } @@ -168,7 +156,7 @@ func TestImport(t *testing.T) { { description: "import batch feature, csv data source has duplicated columns", opt: types.ImportOpt{ - GroupID: 1, + GroupName: "device", DataSource: types.CsvDataSource{ Reader: strings.NewReader(` device,model,model @@ -221,18 +209,18 @@ device,model,price for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - metadataStore.EXPECT().GetGroup(ctx, tc.opt.GroupID). - Return(&types.Group{ - ID: tc.opt.GroupID, - EntityID: tc.entityID, - Entity: &tc.Entity, - }, nil) + metadataStore.EXPECT().GetGroupByName(ctx, tc.opt.GroupName).Return(&types.Group{ + ID: 1, + Name: tc.opt.GroupName, + EntityID: tc.entityID, + Entity: &tc.Entity, + }, nil) - metadataStore.EXPECT().ListFeature(ctx, metadata.ListFeatureOpt{GroupID: &tc.opt.GroupID}).Return(tc.features) + metadataStore.EXPECT().ListFeature(ctx, metadata.ListFeatureOpt{GroupID: intPtr(1)}).Return(tc.features) metadataStore.EXPECT().CreateRevision(ctx, metadata.CreateRevisionOpt{ Revision: 0, - GroupID: tc.opt.GroupID, + GroupID: 1, Description: tc.opt.Description, }).Return(tc.revisionID, "offline_1_1", nil).AnyTimes() From 23c9a5212f23adb08ac2c97a74929e7e2293f751 Mon Sep 17 00:00:00 2001 From: Jinghan Ying Date: Thu, 18 Nov 2021 12:27:48 +0800 Subject: [PATCH 3/3] refactor(featctl): fix featctl command join --- featctl/cmd/import.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/featctl/cmd/import.go b/featctl/cmd/import.go index 9a5835453..396aa966c 100644 --- a/featctl/cmd/import.go +++ b/featctl/cmd/import.go @@ -12,8 +12,7 @@ import ( type importOption struct { types.ImportOpt - FilePath string - groupName string + FilePath string } var importOpt importOption @@ -31,12 +30,6 @@ var importCmd = &cobra.Command{ oomStore := mustOpenOomStore(ctx, oomStoreCfg) defer oomStore.Close() - if group, err := oomStore.GetGroupByName(ctx, importOpt.groupName); err != nil { - log.Fatalf("failed to get feature group name=%s: %v", importOpt.groupName, err) - } else { - importOpt.GroupID = group.ID - } - file, err := os.Open(importOpt.FilePath) if err != nil { log.Fatalf("read file %s failed: %v", importOpt.FilePath, err) @@ -60,7 +53,7 @@ func init() { flags := importCmd.Flags() - flags.StringVarP(&importOpt.groupName, "group", "g", "", "feature group") + flags.StringVarP(&importOpt.GroupName, "group", "g", "", "feature group") _ = importCmd.MarkFlagRequired("group") flags.StringVar(&importOpt.Description, "description", "", "revision description")