From 11053e98885f525fc0e79c14a4310f5b74690707 Mon Sep 17 00:00:00 2001 From: nakabonne Date: Fri, 5 Nov 2021 18:59:47 +0900 Subject: [PATCH 1/2] Replace Tags with Labels to idendify applications/deployments --- pkg/app/api/grpcapi/api.go | 1 - pkg/app/api/grpcapi/web_api.go | 21 +++++----- pkg/app/api/service/apiservice/service.proto | 3 +- .../api/service/pipedservice/service.proto | 2 + pkg/app/api/service/webservice/service.proto | 6 +-- pkg/app/pipectl/cmd/application/add.go | 1 - pkg/app/piped/trigger/deployment.go | 2 +- .../web/src/__fixtures__/dummy-application.ts | 2 +- .../web/src/__fixtures__/dummy-deployment.ts | 2 +- .../src/components/application-form/index.tsx | 2 - .../add-application-drawer/index.test.tsx | 1 - .../edit-application-drawer/index.tsx | 1 - pkg/app/web/src/modules/applications/index.ts | 6 +-- pkg/app/web/src/modules/deployments/index.ts | 2 +- .../src/modules/update-application/index.ts | 2 - pkg/model/application.go | 19 +++++---- pkg/model/application.proto | 4 +- pkg/model/application_test.go | 42 +++++++++++++------ pkg/model/deployment.go | 19 +++++---- pkg/model/deployment.proto | 4 +- pkg/model/deployment_test.go | 42 +++++++++++++------ 21 files changed, 104 insertions(+), 80 deletions(-) diff --git a/pkg/app/api/grpcapi/api.go b/pkg/app/api/grpcapi/api.go index 0b3e61beba..cff17ca62a 100644 --- a/pkg/app/api/grpcapi/api.go +++ b/pkg/app/api/grpcapi/api.go @@ -118,7 +118,6 @@ func (a *API) AddApplication(ctx context.Context, req *apiservice.AddApplication Kind: req.Kind, CloudProvider: req.CloudProvider, Description: req.Description, - Tags: req.Tags, } err = a.applicationStore.AddApplication(ctx, &app) if errors.Is(err, datastore.ErrAlreadyExists) { diff --git a/pkg/app/api/grpcapi/web_api.go b/pkg/app/api/grpcapi/web_api.go index 453fd6eff5..1d8eb81c59 100644 --- a/pkg/app/api/grpcapi/web_api.go +++ b/pkg/app/api/grpcapi/web_api.go @@ -637,7 +637,6 @@ func (a *WebAPI) AddApplication(ctx context.Context, req *webservice.AddApplicat Kind: req.Kind, CloudProvider: req.CloudProvider, Description: req.Description, - Tags: req.Tags, } err = a.applicationStore.AddApplication(ctx, &app) if errors.Is(err, datastore.ErrAlreadyExists) { @@ -647,6 +646,7 @@ func (a *WebAPI) AddApplication(ctx context.Context, req *webservice.AddApplicat a.logger.Error("failed to create application", zap.Error(err)) return nil, status.Error(codes.Internal, "Failed to create application") } + // TODO: Create a command to send an application information defined in the application config return &webservice.AddApplicationResponse{ ApplicationId: app.Id, @@ -660,7 +660,6 @@ func (a *WebAPI) UpdateApplication(ctx context.Context, req *webservice.UpdateAp app.PipedId = req.PipedId app.Kind = req.Kind app.CloudProvider = req.CloudProvider - app.Tags = req.Tags return nil } @@ -861,15 +860,16 @@ func (a *WebAPI) ListApplications(ctx context.Context, req *webservice.ListAppli return nil, status.Error(codes.Internal, "Failed to get applications") } - if len(req.Options.Tags) == 0 { + if len(req.Options.Labels) == 0 { return &webservice.ListApplicationsResponse{ Applications: apps, }, nil } + // NOTE: Filtering by labels is done by the application-side because we need to create composite indexes for every combination in the filter. filtered := make([]*model.Application, 0, len(apps)) for _, a := range apps { - if a.ContainTags(req.Options.Tags) { + if a.ContainLabels(req.Options.Labels) { filtered = append(filtered, a) } } @@ -1071,20 +1071,21 @@ func (a *WebAPI) ListDeployments(ctx context.Context, req *webservice.ListDeploy a.logger.Error("failed to get deployments", zap.Error(err)) return nil, status.Error(codes.Internal, "Failed to get deployments") } - tags := req.Options.Tags - if len(tags) == 0 || len(deployments) == 0 { + labels := req.Options.Labels + if len(labels) == 0 || len(deployments) == 0 { return &webservice.ListDeploymentsResponse{ Deployments: deployments, Cursor: cursor, }, nil } - // Start filtering them by tags. - // NOTE: For document-oriented databases, it's hard to look for deployments that have all the tags we specified. + // Start filtering them by labels. + // + // NOTE: Filtering by labels is done by the application-side because we need to create composite indexes for every combination in the filter. // We don't want to depend on any other search engine, that's why it filters here. filtered := make([]*model.Deployment, 0, len(deployments)) for _, d := range deployments { - if d.ContainTags(tags) { + if d.ContainLabels(labels) { filtered = append(filtered, d) } } @@ -1109,7 +1110,7 @@ func (a *WebAPI) ListDeployments(ctx context.Context, req *webservice.ListDeploy break } for _, d := range deployments { - if d.ContainTags(tags) { + if d.ContainLabels(labels) { filtered = append(filtered, d) } } diff --git a/pkg/app/api/service/apiservice/service.proto b/pkg/app/api/service/apiservice/service.proto index ec0ece792b..0bada5349c 100644 --- a/pkg/app/api/service/apiservice/service.proto +++ b/pkg/app/api/service/apiservice/service.proto @@ -55,7 +55,6 @@ message AddApplicationRequest { model.ApplicationKind kind = 5 [(validate.rules).enum.defined_only = true]; string cloud_provider = 6 [(validate.rules).string.min_len = 1]; string description = 7; - repeated string tags = 8; } message AddApplicationResponse { @@ -84,7 +83,7 @@ message ListApplicationsRequest { string env_id = 3; bool disabled = 4; string env_name = 5; - // TODO: Enable to filter by tags + // TODO: Enable to filter by labels string cursor = 10; } diff --git a/pkg/app/api/service/pipedservice/service.proto b/pkg/app/api/service/pipedservice/service.proto index 6835c0c478..8c19c261bd 100644 --- a/pkg/app/api/service/pipedservice/service.proto +++ b/pkg/app/api/service/pipedservice/service.proto @@ -161,6 +161,8 @@ service PipedService { // GetDesiredVersion returns the desired version of the given Piped. rpc GetDesiredVersion(GetDesiredVersionRequest) returns (GetDesiredVersionResponse) {} + + // TODO: Add an rpc to update application info based on one defined in the application config } enum ListOrder { diff --git a/pkg/app/api/service/webservice/service.proto b/pkg/app/api/service/webservice/service.proto index a1430d671c..7511aa643b 100644 --- a/pkg/app/api/service/webservice/service.proto +++ b/pkg/app/api/service/webservice/service.proto @@ -227,7 +227,6 @@ message AddApplicationRequest { model.ApplicationKind kind = 5 [(validate.rules).enum.defined_only = true]; string cloud_provider = 6 [(validate.rules).string.min_len = 1]; string description = 7; - repeated string tags = 8; } message AddApplicationResponse { @@ -241,7 +240,6 @@ message UpdateApplicationRequest { string piped_id = 4 [(validate.rules).string.min_len = 1]; model.ApplicationKind kind = 6 [(validate.rules).enum.defined_only = true]; string cloud_provider = 7 [(validate.rules).string.min_len = 1]; - repeated string tags = 8; } message UpdateApplicationResponse { @@ -283,7 +281,7 @@ message ListApplicationsRequest { repeated model.ApplicationSyncStatus sync_statuses = 3; repeated string env_ids = 4; string name = 5; - repeated string tags = 6; + map labels = 6; } Options options = 1; } @@ -327,7 +325,7 @@ message ListDeploymentsRequest { repeated string application_ids = 3; repeated string env_ids = 4; string application_name = 5; - repeated string tags = 6; + map labels = 6; } Options options = 1; int32 page_size = 2; diff --git a/pkg/app/pipectl/cmd/application/add.go b/pkg/app/pipectl/cmd/application/add.go index 655617055e..07dbe1006d 100644 --- a/pkg/app/pipectl/cmd/application/add.go +++ b/pkg/app/pipectl/cmd/application/add.go @@ -34,7 +34,6 @@ type add struct { pipedID string cloudProvider string description string - tags []string repoID string appDir string diff --git a/pkg/app/piped/trigger/deployment.go b/pkg/app/piped/trigger/deployment.go index 2d5a2d8d53..2a087e1fd4 100644 --- a/pkg/app/piped/trigger/deployment.go +++ b/pkg/app/piped/trigger/deployment.go @@ -172,7 +172,7 @@ func buildDeployment( }, GitPath: app.GitPath, CloudProvider: app.CloudProvider, - Tags: app.Tags, + Labels: app.Labels, Status: model.DeploymentStatus_DEPLOYMENT_PENDING, StatusReason: "The deployment is waiting to be planned", Metadata: metadata, diff --git a/pkg/app/web/src/__fixtures__/dummy-application.ts b/pkg/app/web/src/__fixtures__/dummy-application.ts index 6f05957206..96680fe8a3 100644 --- a/pkg/app/web/src/__fixtures__/dummy-application.ts +++ b/pkg/app/web/src/__fixtures__/dummy-application.ts @@ -39,7 +39,7 @@ export const dummyApplication: Application.AsObject = { pipedId: dummyPiped.id, projectId: "project-1", description: "", - tagsList: [], + labelsMap: [], mostRecentlySuccessfulDeployment: { deploymentId: "deployment-1", completedAt: 0, diff --git a/pkg/app/web/src/__fixtures__/dummy-deployment.ts b/pkg/app/web/src/__fixtures__/dummy-deployment.ts index 0908033a61..881f94b2ab 100644 --- a/pkg/app/web/src/__fixtures__/dummy-deployment.ts +++ b/pkg/app/web/src/__fixtures__/dummy-deployment.ts @@ -23,7 +23,7 @@ export const dummyDeployment: Deployment.AsObject = { trigger: dummyTrigger, version: "0.0.0", cloudProvider: "kube-1", - tagsList: [], + labelsMap: [], createdAt: createdAt.unix(), updatedAt: completedAt.unix(), completedAt: completedAt.unix(), diff --git a/pkg/app/web/src/components/application-form/index.tsx b/pkg/app/web/src/components/application-form/index.tsx index a29f9c88c9..8b1b6791ed 100644 --- a/pkg/app/web/src/components/application-form/index.tsx +++ b/pkg/app/web/src/components/application-form/index.tsx @@ -156,7 +156,6 @@ export interface ApplicationFormValue { remote: string; branch: string; }; - tagsList: string[], } export type ApplicationFormProps = FormikProps & { @@ -178,7 +177,6 @@ export const emptyFormValues: ApplicationFormValue = { remote: "", branch: "", }, - tagsList: [], }; export const ApplicationForm: FC = memo( diff --git a/pkg/app/web/src/components/applications-page/add-application-drawer/index.test.tsx b/pkg/app/web/src/components/applications-page/add-application-drawer/index.test.tsx index 7f3d9976e3..f000610fdc 100644 --- a/pkg/app/web/src/components/applications-page/add-application-drawer/index.test.tsx +++ b/pkg/app/web/src/components/applications-page/add-application-drawer/index.test.tsx @@ -82,7 +82,6 @@ describe("AddApplicationDrawer", () => { remote: "git@github.com:pipe-cd/debug.git", }, repoPath: "path", - tagsList: [], }, }), }, diff --git a/pkg/app/web/src/components/applications-page/edit-application-drawer/index.tsx b/pkg/app/web/src/components/applications-page/edit-application-drawer/index.tsx index 6bd4ddd08d..097e15bc6f 100644 --- a/pkg/app/web/src/components/applications-page/edit-application-drawer/index.tsx +++ b/pkg/app/web/src/components/applications-page/edit-application-drawer/index.tsx @@ -42,7 +42,6 @@ export const EditApplicationDrawer: FC = memo( repo: app.gitPath?.repo || { id: "", remote: "", branch: "" }, configFilename: app.gitPath?.configFilename || "", cloudProvider: app.cloudProvider, - tagsList: app.tagsList, } : emptyFormValues, validationSchema, diff --git a/pkg/app/web/src/modules/applications/index.ts b/pkg/app/web/src/modules/applications/index.ts index dc29e4b74d..a581477693 100644 --- a/pkg/app/web/src/modules/applications/index.ts +++ b/pkg/app/web/src/modules/applications/index.ts @@ -55,7 +55,7 @@ export const fetchApplications = createAsyncThunk< enabled: options.activeStatus ? { value: options.activeStatus === "enabled" } : undefined, - tagsList: [], // TODO: Specify tags for ListApplications + labelsMap: [], // TODO: Specify labels for ListApplications }, }); return applicationsList as Application.AsObject[]; @@ -71,7 +71,7 @@ export const fetchApplicationsByEnv = createAsyncThunk< kindsList: [], name: "", syncStatusesList: [], - tagsList: [], + labelsMap: [], }, }); return applicationsList as Application.AsObject[]; @@ -108,7 +108,6 @@ export const addApplication = createAsyncThunk< configFilename?: string; kind: ApplicationKind; cloudProvider: string; - tagsList: string[]; } >(`${MODULE_NAME}/add`, async (props) => { const { applicationId } = await applicationsAPI.addApplication({ @@ -125,7 +124,6 @@ export const addApplication = createAsyncThunk< cloudProvider: props.cloudProvider, kind: props.kind, description: "", - tagsList: props.tagsList, }); return applicationId; diff --git a/pkg/app/web/src/modules/deployments/index.ts b/pkg/app/web/src/modules/deployments/index.ts index e1e4838a81..f3c33f7852 100644 --- a/pkg/app/web/src/modules/deployments/index.ts +++ b/pkg/app/web/src/modules/deployments/index.ts @@ -102,7 +102,7 @@ const convertFilterOptions = ( statusesList: options.status ? [parseInt(options.status, 10) as DeploymentStatus] : [], - tagsList: [], // TODO: Specify tags for ListDeployments + labelsMap: [], // TODO: Specify labels for ListDeployments }; }; diff --git a/pkg/app/web/src/modules/update-application/index.ts b/pkg/app/web/src/modules/update-application/index.ts index 9407ec7e63..96f733c4b4 100644 --- a/pkg/app/web/src/modules/update-application/index.ts +++ b/pkg/app/web/src/modules/update-application/index.ts @@ -28,7 +28,6 @@ export const updateApplication = createAsyncThunk< configFilename?: string; kind: ApplicationKind; cloudProvider: string; - tagsList: string[]; } >(`${MODULE_NAME}/update`, async (values) => { await applicationAPI.updateApplication({ @@ -38,7 +37,6 @@ export const updateApplication = createAsyncThunk< pipedId: values.pipedId, cloudProvider: values.cloudProvider, kind: values.kind, - tagsList: values.tagsList, }); }); diff --git a/pkg/model/application.go b/pkg/model/application.go index 6b2916e5ae..57ad96b827 100644 --- a/pkg/model/application.go +++ b/pkg/model/application.go @@ -50,17 +50,18 @@ func MakeApplicationURL(baseURL, applicationID string) string { return fmt.Sprintf("%s/applications/%s", strings.TrimSuffix(baseURL, "/"), applicationID) } -// ContainTags checks if it has all the given tags. -func (d *Application) ContainTags(tags []string) bool { - if len(d.Tags) < len(tags) { +// ContainLabels checks if it has all the given labels. +func (a *Application) ContainLabels(labels map[string]string) bool { + if len(a.Labels) < len(labels) { return false } - tagMap := make(map[string]struct{}, len(d.Tags)) - for i := range d.Tags { - tagMap[d.Tags[i]] = struct{}{} - } - for _, tag := range tags { - if _, ok := tagMap[tag]; !ok { + + for k, v := range labels { + value, ok := a.Labels[k] + if !ok { + return false + } + if value != v { return false } } diff --git a/pkg/model/application.proto b/pkg/model/application.proto index 01f160f4fe..3755ebfaf8 100644 --- a/pkg/model/application.proto +++ b/pkg/model/application.proto @@ -42,8 +42,8 @@ message Application { string cloud_provider = 8 [(validate.rules).string.min_len = 1]; // Additional description about application. string description = 9; - // Custom attributes for filtering. - repeated string tags = 10; + // Custom attributes to identify applications. + map labels = 10; // Basic information about the most recently successful deployment. // This also shows information about current running workloads. diff --git a/pkg/model/application_test.go b/pkg/model/application_test.go index c3ed6aae83..fb8ab62a0c 100644 --- a/pkg/model/application_test.go +++ b/pkg/model/application_test.go @@ -48,35 +48,51 @@ func TestMakeApplicationURL(t *testing.T) { } } -func TestApplication_ContainTags(t *testing.T) { +func TestApplication_ContainLabels(t *testing.T) { testcases := []struct { - name string - app *Application - tags []string - want bool + name string + app *Application + labels map[string]string + want bool }{ { name: "all given tags aren't contained", - app: &Application{Tags: []string{"foo"}}, - tags: []string{"foo", "bar"}, + app: &Application{Labels: map[string]string{"key1": "value1"}}, + labels: map[string]string{ + "key1": "value1", + "key2": "value2", + }, want: false, }, { - name: "a tag is contained", - app: &Application{Tags: []string{"foo", "bar"}}, - tags: []string{"foo"}, + name: "a label is contained", + app: &Application{Labels: map[string]string{ + "key1": "value1", + "key2": "value2", + }}, + labels: map[string]string{ + "key1": "value1", + }, want: true, }, { name: "all tags are contained", - app: &Application{Tags: []string{"foo", "bar", "baz"}}, - tags: []string{"baz", "foo", "bar"}, + app: &Application{Labels: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }}, + labels: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, want: true, }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - got := tc.app.ContainTags(tc.tags) + got := tc.app.ContainLabels(tc.labels) assert.Equal(t, tc.want, got) }) } diff --git a/pkg/model/deployment.go b/pkg/model/deployment.go index b73a0e7ca8..baf5b489be 100644 --- a/pkg/model/deployment.go +++ b/pkg/model/deployment.go @@ -193,17 +193,18 @@ func DeploymentStatusStrings() []string { return out } -// ContainTags checks if it has all the given tags. -func (d *Deployment) ContainTags(tags []string) bool { - if len(d.Tags) < len(tags) { +// ContainLabels checks if it has all the given labels. +func (d *Deployment) ContainLabels(labels map[string]string) bool { + if len(d.Labels) < len(labels) { return false } - tagMap := make(map[string]struct{}, len(d.Tags)) - for i := range d.Tags { - tagMap[d.Tags[i]] = struct{}{} - } - for _, tag := range tags { - if _, ok := tagMap[tag]; !ok { + + for k, v := range labels { + value, ok := d.Labels[k] + if !ok { + return false + } + if value != v { return false } } diff --git a/pkg/model/deployment.proto b/pkg/model/deployment.proto index ec4ce06017..a842fff6ce 100644 --- a/pkg/model/deployment.proto +++ b/pkg/model/deployment.proto @@ -68,8 +68,8 @@ message Deployment { // This must be one of the provider names registered in the piped. string cloud_provider = 9 [(validate.rules).string.min_len = 1]; - // Custom attributes for filtering. - repeated string tags = 10; + // Custom attributes to identify applications. + map labels = 10; DeploymentTrigger trigger = 20 [(validate.rules).message.required = true]; // Hash value of the most recently successfully deployed commit. diff --git a/pkg/model/deployment_test.go b/pkg/model/deployment_test.go index 6e0ce2ebe8..223625db77 100644 --- a/pkg/model/deployment_test.go +++ b/pkg/model/deployment_test.go @@ -24,31 +24,47 @@ func TestDeployment_ContainTags(t *testing.T) { testcases := []struct { name string deployment *Deployment - tags []string + labels map[string]string want bool }{ { name: "all given tags aren't contained", - deployment: &Deployment{Tags: []string{"foo"}}, - tags: []string{"foo", "bar"}, - want: false, + deployment: &Deployment{Labels: map[string]string{"key1": "value1"}}, + labels: map[string]string{ + "key1": "value1", + "key2": "value2", + }, + want: false, }, { - name: "a tag is contained", - deployment: &Deployment{Tags: []string{"foo", "bar"}}, - tags: []string{"foo"}, - want: true, + name: "a label is contained", + deployment: &Deployment{Labels: map[string]string{ + "key1": "value1", + "key2": "value2", + }}, + labels: map[string]string{ + "key1": "value1", + }, + want: true, }, { - name: "all tags are contained", - deployment: &Deployment{Tags: []string{"foo", "bar", "baz"}}, - tags: []string{"baz", "foo", "bar"}, - want: true, + name: "all tags are contained", + deployment: &Deployment{Labels: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }}, + labels: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + want: true, }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - got := tc.deployment.ContainTags(tc.tags) + got := tc.deployment.ContainLabels(tc.labels) assert.Equal(t, tc.want, got) }) } From 30721381d8477ea4bf931b374cffe8292f2c676d Mon Sep 17 00:00:00 2001 From: nakabonne Date: Fri, 5 Nov 2021 19:29:57 +0900 Subject: [PATCH 2/2] Fix web-test --- .../deployments-detail-page/pipeline/index.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/app/web/src/components/deployments-detail-page/pipeline/index.stories.tsx b/pkg/app/web/src/components/deployments-detail-page/pipeline/index.stories.tsx index b4d299a129..9e9c3e4407 100644 --- a/pkg/app/web/src/components/deployments-detail-page/pipeline/index.stories.tsx +++ b/pkg/app/web/src/components/deployments-detail-page/pipeline/index.stories.tsx @@ -27,7 +27,7 @@ const fakeDeployment: Deployment.AsObject = { }, version: "0.0.1", cloudProvider: "", - tagsList: [], + labelsMap: [], trigger: { commit: { hash: "3808585b46f1e90196d7ffe8dd04c807a251febc",