From b97dcf207ca2ef7f78dfc144a797df3cf2de84ce Mon Sep 17 00:00:00 2001 From: nakabonne Date: Fri, 26 Nov 2021 18:06:15 +0900 Subject: [PATCH 1/5] Stop unnecessarily scanning all files to report app configs --- pkg/app/api/grpcapi/piped_api.go | 1 - .../appconfigreporter/appconfigreporter.go | 247 ++++++++---------- .../appconfigreporter_test.go | 184 ++++++------- 3 files changed, 194 insertions(+), 238 deletions(-) diff --git a/pkg/app/api/grpcapi/piped_api.go b/pkg/app/api/grpcapi/piped_api.go index d6a2f6e4f9..a307459d62 100644 --- a/pkg/app/api/grpcapi/piped_api.go +++ b/pkg/app/api/grpcapi/piped_api.go @@ -977,7 +977,6 @@ func (a *PipedAPI) UpdateApplicationConfigurations(ctx context.Context, req *pip return nil, err } } - // TODO: Consider bulk-updating multiple apps for _, appInfo := range req.Applications { updater := func(app *model.Application) error { app.Name = appInfo.Name diff --git a/pkg/app/piped/appconfigreporter/appconfigreporter.go b/pkg/app/piped/appconfigreporter/appconfigreporter.go index f07c5d759c..11ac7febf7 100644 --- a/pkg/app/piped/appconfigreporter/appconfigreporter.go +++ b/pkg/app/piped/appconfigreporter/appconfigreporter.go @@ -67,6 +67,8 @@ type Reporter struct { // Whether it already swept all unregistered apps from control-plane. sweptUnregisteredApps bool + // The latest unregistered apps for each repository. + latestUnregisteredApps map[string][]*model.ApplicationInfo } func NewReporter( @@ -78,14 +80,15 @@ func NewReporter( logger *zap.Logger, ) *Reporter { return &Reporter{ - apiClient: apiClient, - gitClient: gitClient, - applicationLister: appLister, - config: cfg, - gracePeriod: gracePeriod, - lastScannedCommits: make(map[string]string), - fileSystem: &fileSystem{}, - logger: logger.Named("app-config-reporter"), + apiClient: apiClient, + gitClient: gitClient, + applicationLister: appLister, + config: cfg, + gracePeriod: gracePeriod, + lastScannedCommits: make(map[string]string), + fileSystem: &fileSystem{}, + logger: logger.Named("app-config-reporter"), + latestUnregisteredApps: make(map[string][]*model.ApplicationInfo), } } @@ -131,6 +134,7 @@ func (r *Reporter) scanAppConfigs(ctx context.Context) error { } // Make all repos up-to-date. + headCommits := make(map[string]string, len(r.gitRepos)) for repoID, repo := range r.gitRepos { if err := repo.Pull(ctx, repo.GetClonedBranch()); err != nil { r.logger.Error("failed to update repo to latest", @@ -139,156 +143,85 @@ func (r *Reporter) scanAppConfigs(ctx context.Context) error { ) return err } + headCommit, err := repo.GetLatestCommit(ctx) + if err != nil { + return fmt.Errorf("failed to get the latest commit of %s: %w", repoID, err) + } + headCommits[repoID] = headCommit.Hash } - apps := r.applicationLister.List() - // Create a map from Git path key to app id, to determine if the application is registered. - registeredAppPaths := make(map[string]string, len(apps)) - for _, app := range apps { - key := makeGitPathKey(app.GitPath.Repo.Id, filepath.Join(app.GitPath.Path, app.GitPath.ConfigFilename)) - registeredAppPaths[key] = app.Id - } - - if err := r.updateRegisteredApps(ctx, registeredAppPaths); err != nil { - return fmt.Errorf("failed to update registered applications: %w", err) + if err := r.updateRegisteredApps(ctx, headCommits); err != nil { + return err } - if err := r.updateUnregisteredApps(ctx, registeredAppPaths); err != nil { - return fmt.Errorf("failed to update unregistered applications: %w", err) + if err := r.updateUnregisteredApps(ctx, headCommits); err != nil { + return err } + for repoID, hash := range headCommits { + r.lastScannedCommits[repoID] = hash + } return nil } // updateRegisteredApps sends application configurations that have changed since the last time to the control-plane. -func (r *Reporter) updateRegisteredApps(ctx context.Context, registeredAppPaths map[string]string) (err error) { - apps := make([]*model.ApplicationInfo, 0) - headCommits := make(map[string]string, len(r.gitRepos)) +func (r *Reporter) updateRegisteredApps(ctx context.Context, headCommits map[string]string) error { + registeredApps := make([]*model.ApplicationInfo, 0) for repoID, repo := range r.gitRepos { - var headCommit git.Commit - headCommit, err = repo.GetLatestCommit(ctx) - if err != nil { - return fmt.Errorf("failed to get the latest commit of %s: %w", repoID, err) - } + headCommit := headCommits[repoID] // Skip if the head commit is already scanned. - lastScannedCommit, ok := r.lastScannedCommits[repoID] - if ok && headCommit.Hash == lastScannedCommit { + if lastScannedCommit, ok := r.lastScannedCommits[repoID]; ok && headCommit == lastScannedCommit { continue } - var as []*model.ApplicationInfo - as, err = r.findRegisteredApps(ctx, repoID, repo, lastScannedCommit, headCommit.Hash, registeredAppPaths) + + rs, err := r.findRegisteredApps(repo.GetPath(), repoID) if err != nil { return err } - apps = append(apps, as...) - headCommits[repoID] = headCommit.Hash + r.logger.Info(fmt.Sprintf("found out %d registered applications that config has been changed in repository %q", len(rs), repoID)) + registeredApps = append(registeredApps, rs...) } - defer func() { - if err == nil { - for repoID, hash := range headCommits { - r.lastScannedCommits[repoID] = hash - } - } - }() - if len(apps) == 0 { - return + if len(registeredApps) == 0 { + return nil } - _, err = r.apiClient.UpdateApplicationConfigurations( + _, err := r.apiClient.UpdateApplicationConfigurations( ctx, &pipedservice.UpdateApplicationConfigurationsRequest{ - Applications: apps, + Applications: registeredApps, }, ) if err != nil { return fmt.Errorf("failed to update application configurations: %w", err) } - return + return nil } -// findRegisteredApps finds out registered application info in the given git repository. -func (r *Reporter) findRegisteredApps(ctx context.Context, repoID string, repo gitRepo, lastScannedCommit, headCommitHash string, registeredAppPaths map[string]string) ([]*model.ApplicationInfo, error) { - if lastScannedCommit == "" { - // Scan all files because it seems to be the first commit since Piped starts. - apps := make([]*model.ApplicationInfo, 0) - err := fs.WalkDir(r.fileSystem, repo.GetPath(), func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err - } - if d.IsDir() { - return nil - } - cfgRelPath, err := filepath.Rel(repo.GetPath(), path) - if err != nil { - return err - } - gitPathKey := makeGitPathKey(repoID, cfgRelPath) - appID, registered := registeredAppPaths[gitPathKey] - if !registered { - return nil - } - - appInfo, err := r.readApplicationInfo(repo.GetPath(), repoID, filepath.Dir(cfgRelPath), filepath.Base(cfgRelPath)) - if err != nil { - r.logger.Error("failed to read application info", - zap.String("repo-id", repoID), - zap.String("config-file-path", cfgRelPath), - zap.Error(err), - ) - return nil +// updateUnregisteredApps sends all unregistered application configurations to the control-plane. +func (r *Reporter) updateUnregisteredApps(ctx context.Context, headCommits map[string]string) error { + unregisteredApps := make([]*model.ApplicationInfo, 0) + for repoID, repo := range r.gitRepos { + headCommit := headCommits[repoID] + if lastScannedCommit, ok := r.lastScannedCommits[repoID]; ok && headCommit == lastScannedCommit { + // Use the cached apps if the head commit is already scanned. + // The unregistered apps sent previously aren't persisted, that's why it has to send them again even if it's scanned one. + if apps, ok := r.latestUnregisteredApps[repoID]; ok { + unregisteredApps = append(unregisteredApps, apps...) } - appInfo.Id = appID - apps = append(apps, appInfo) - // Continue reading so that it can return apps as much as possible. - return nil - }) - if err != nil { - return nil, fmt.Errorf("failed to inspect files under %s: %w", repo.GetPath(), err) - } - return apps, nil - } - - filePaths, err := repo.ChangedFiles(ctx, lastScannedCommit, headCommitHash) - if err != nil { - return nil, fmt.Errorf("failed to get files those were touched between two commits: %w", err) - } - if len(filePaths) == 0 { - // The case where all changes have been fully reverted. - return []*model.ApplicationInfo{}, nil - } - apps := make([]*model.ApplicationInfo, 0) - for _, path := range filePaths { - gitPathKey := makeGitPathKey(repoID, path) - appID, registered := registeredAppPaths[gitPathKey] - if !registered { continue } - appInfo, err := r.readApplicationInfo(repo.GetPath(), repoID, filepath.Dir(path), filepath.Base(path)) - if err != nil { - r.logger.Error("failed to read application info", - zap.String("repo-id", repoID), - zap.String("config-file-path", path), - zap.Error(err), - ) - continue - } - appInfo.Id = appID - apps = append(apps, appInfo) - } - return apps, nil -} -// updateUnregisteredApps sends all unregistered application configurations to the control-plane. -func (r *Reporter) updateUnregisteredApps(ctx context.Context, registeredAppPaths map[string]string) error { - apps := make([]*model.ApplicationInfo, 0) - for repoID, repo := range r.gitRepos { - as, err := r.findUnregisteredApps(repo.GetPath(), repoID, registeredAppPaths) + us, err := r.findUnregisteredApps(repo.GetPath(), repoID) if err != nil { return err } - r.logger.Info(fmt.Sprintf("found out %d unregistered applications in repository %s", len(as), repoID)) - apps = append(apps, as...) + r.logger.Info(fmt.Sprintf("found out %d unregistered applications in repository %q", len(us), repoID)) + unregisteredApps = append(unregisteredApps, us...) + r.latestUnregisteredApps[repoID] = us } - if len(apps) == 0 { + + // Even if the result is zero, we need to report at least once. + // However, it should return after the second time which is unnecessary. + if len(unregisteredApps) == 0 { if r.sweptUnregisteredApps { return nil } @@ -300,7 +233,7 @@ func (r *Reporter) updateUnregisteredApps(ctx context.Context, registeredAppPath _, err := r.apiClient.ReportUnregisteredApplicationConfigurations( ctx, &pipedservice.ReportUnregisteredApplicationConfigurationsRequest{ - Applications: apps, + Applications: unregisteredApps, }, ) if err != nil { @@ -309,10 +242,61 @@ func (r *Reporter) updateUnregisteredApps(ctx context.Context, registeredAppPath return nil } +// findRegisteredApps finds out registered application info that should be updated in the given git repository. +func (r *Reporter) findRegisteredApps(repoPath, repoID string) ([]*model.ApplicationInfo, error) { + // Compare the apps registered on Control-plane with the latest config file + // and return only the ones that have been changed. + apps := make([]*model.ApplicationInfo, 0) + for _, app := range r.applicationLister.List() { + if app.GitPath.Repo.Id != repoID { + continue + } + appCfg, err := r.readApplicationInfo(repoPath, repoID, app.GitPath.Path, app.GitPath.ConfigFilename) + if err != nil { + return nil, fmt.Errorf("failed to read application config file: %w", err) + } + if isSynced(appCfg, app) { + continue + } + appCfg.Id = app.Id + apps = append(apps, appCfg) + } + return apps, nil +} + +func isSynced(appInfo *model.ApplicationInfo, app *model.Application) bool { + // TODO: Make it possible to follow the ApplicationInfo field changes + if appInfo.Name != app.Name { + return false + } + if appInfo.Kind != app.Kind { + return false + } + if len(appInfo.Labels) != len(app.Labels) { + return false + } + for key, value := range appInfo.Labels { + if value != app.Labels[key] { + return false + } + } + return true +} + // findUnregisteredApps finds out unregistered application info in the given git repository. // The file name must be default name in order to be recognized as an Application config. -func (r *Reporter) findUnregisteredApps(repoPath, repoID string, registeredAppPaths map[string]string) ([]*model.ApplicationInfo, error) { - apps := make([]*model.ApplicationInfo, 0) +func (r *Reporter) findUnregisteredApps(repoPath, repoID string) ([]*model.ApplicationInfo, error) { + apps := r.applicationLister.List() + // Create a map to determine the app is registered by GitPath. + registeredAppPaths := make(map[string]struct{}, len(apps)) + for _, app := range apps { + if app.GitPath.Repo.Id != repoID { + continue + } + registeredAppPaths[filepath.Join(app.GitPath.Path, app.GitPath.ConfigFilename)] = struct{}{} + } + + out := make([]*model.ApplicationInfo, 0) // Scan all files under the repository. err := fs.WalkDir(r.fileSystem, repoPath, func(path string, d fs.DirEntry, err error) error { if err != nil { @@ -328,8 +312,7 @@ func (r *Reporter) findUnregisteredApps(repoPath, repoID string, registeredAppPa if !model.IsApplicationConfigFile(filepath.Base(cfgRelPath)) { return nil } - gitPathKey := makeGitPathKey(repoID, cfgRelPath) - if _, registered := registeredAppPaths[gitPathKey]; registered { + if _, registered := registeredAppPaths[cfgRelPath]; registered { return nil } @@ -342,14 +325,14 @@ func (r *Reporter) findUnregisteredApps(repoPath, repoID string, registeredAppPa ) return nil } - apps = append(apps, appInfo) + out = append(out, appInfo) // Continue reading so that it can return apps as much as possible. return nil }) if err != nil { return nil, fmt.Errorf("failed to inspect files under %s: %w", repoPath, err) } - return apps, nil + return out, nil } func (r *Reporter) readApplicationInfo(repoDir, repoID, appDirRelPath, cfgFilename string) (*model.ApplicationInfo, error) { @@ -383,9 +366,3 @@ func (r *Reporter) readApplicationInfo(repoDir, repoID, appDirRelPath, cfgFilena ConfigFilename: cfgFilename, }, nil } - -// makeGitPathKey builds a unique path between repositories. -// cfgFilePath is a relative path from the repo root. -func makeGitPathKey(repoID, cfgFilePath string) string { - return fmt.Sprintf("%s:%s", repoID, cfgFilePath) -} diff --git a/pkg/app/piped/appconfigreporter/appconfigreporter_test.go b/pkg/app/piped/appconfigreporter/appconfigreporter_test.go index 7ba8dcb183..29d7ad76eb 100644 --- a/pkg/app/piped/appconfigreporter/appconfigreporter_test.go +++ b/pkg/app/piped/appconfigreporter/appconfigreporter_test.go @@ -15,7 +15,6 @@ package appconfigreporter import ( - "context" "testing" "testing/fstest" @@ -25,10 +24,18 @@ import ( "github.com/pipe-cd/pipe/pkg/model" ) -func TestReporter_findUnregisteredApps(t *testing.T) { +type fakeApplicationLister struct { + apps []*model.Application +} + +func (f *fakeApplicationLister) List() []*model.Application { + return f.apps +} + +func TestReporter_findRegisteredApps(t *testing.T) { type args struct { - registeredAppPaths map[string]string - repoPath, repoID string + repoPath string + repoID string } testcases := []struct { name string @@ -38,35 +45,29 @@ func TestReporter_findUnregisteredApps(t *testing.T) { wantErr bool }{ { - name: "file not found", + name: "no app registered", reporter: &Reporter{ - fileSystem: fstest.MapFS{ - "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte("")}, - }, - logger: zap.NewNop(), + applicationLister: &fakeApplicationLister{}, + logger: zap.NewNop(), }, args: args{ - repoPath: "invalid", - repoID: "repo-1", - registeredAppPaths: map[string]string{}, + repoPath: "path/to/repo-1", + repoID: "repo-1", }, - want: nil, - wantErr: true, + want: []*model.ApplicationInfo{}, + wantErr: false, }, { - name: "all are registered", + name: "no app registered in the repo", reporter: &Reporter{ - fileSystem: fstest.MapFS{ - "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte("")}, - }, + applicationLister: &fakeApplicationLister{apps: []*model.Application{ + {Id: "id-1", Name: "app-1", Labels: map[string]string{"key-1": "value-1"}, GitPath: &model.ApplicationGitPath{Repo: &model.ApplicationGitRepository{Id: "different-repo"}, Path: "app-1", ConfigFilename: ".pipe.yaml"}}, + }}, logger: zap.NewNop(), }, args: args{ repoPath: "path/to/repo-1", repoID: "repo-1", - registeredAppPaths: map[string]string{ - "repo-1:app-1/.pipe.yaml": "id-1", - }, }, want: []*model.ApplicationInfo{}, wantErr: false, @@ -74,22 +75,27 @@ func TestReporter_findUnregisteredApps(t *testing.T) { { name: "invalid app config is contained", reporter: &Reporter{ + applicationLister: &fakeApplicationLister{apps: []*model.Application{ + {Id: "id-1", Name: "app-1", Labels: map[string]string{"key-1": "value-1"}, GitPath: &model.ApplicationGitPath{Repo: &model.ApplicationGitRepository{Id: "repo-1"}, Path: "app-1", ConfigFilename: ".pipe.yaml"}}, + }}, fileSystem: fstest.MapFS{ "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte("invalid-text")}, }, logger: zap.NewNop(), }, args: args{ - repoPath: "path/to/repo-1", - repoID: "repo-1", - registeredAppPaths: map[string]string{}, + repoPath: "path/to/repo-1", + repoID: "repo-1", }, - want: []*model.ApplicationInfo{}, - wantErr: false, + want: nil, + wantErr: true, }, { - name: "valid app config that is unregistered", + name: "app not changed", reporter: &Reporter{ + applicationLister: &fakeApplicationLister{apps: []*model.Application{ + {Id: "id-1", Name: "app-1", Labels: map[string]string{"key-1": "value-1"}, GitPath: &model.ApplicationGitPath{Repo: &model.ApplicationGitRepository{Id: "repo-1"}, Path: "app-1", ConfigFilename: ".pipe.yaml"}}, + }}, fileSystem: fstest.MapFS{ "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte(` apiVersion: pipecd.dev/v1beta1 @@ -102,47 +108,41 @@ spec: logger: zap.NewNop(), }, args: args{ - repoPath: "path/to/repo-1", - repoID: "repo-1", - registeredAppPaths: map[string]string{}, - }, - want: []*model.ApplicationInfo{ - { - Name: "app-1", - Labels: map[string]string{"key-1": "value-1"}, - RepoId: "repo-1", - Path: "app-1", - ConfigFilename: ".pipe.yaml", - }, + repoPath: "path/to/repo-1", + repoID: "repo-1", }, + want: []*model.ApplicationInfo{}, wantErr: false, }, { - name: "valid app config that name isn't default", + name: "app changed", reporter: &Reporter{ + applicationLister: &fakeApplicationLister{apps: []*model.Application{ + {Id: "id-1", Name: "app-1", Labels: map[string]string{"key-1": "value-1"}, GitPath: &model.ApplicationGitPath{Repo: &model.ApplicationGitRepository{Id: "repo-1"}, Path: "app-1", ConfigFilename: ".pipe.yaml"}}, + }}, fileSystem: fstest.MapFS{ - "path/to/repo-1/app-1/dev.pipecd.yaml": &fstest.MapFile{Data: []byte(` + "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte(` apiVersion: pipecd.dev/v1beta1 kind: KubernetesApp spec: - name: app-1 + name: new-app-1 labels: key-1: value-1`)}, }, logger: zap.NewNop(), }, args: args{ - repoPath: "path/to/repo-1", - repoID: "repo-1", - registeredAppPaths: map[string]string{}, + repoPath: "path/to/repo-1", + repoID: "repo-1", }, want: []*model.ApplicationInfo{ { - Name: "app-1", + Id: "id-1", + Name: "new-app-1", Labels: map[string]string{"key-1": "value-1"}, RepoId: "repo-1", Path: "app-1", - ConfigFilename: "dev.pipecd.yaml", + ConfigFilename: ".pipe.yaml", }, }, wantErr: false, @@ -150,33 +150,17 @@ spec: } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - got, err := tc.reporter.findUnregisteredApps(tc.args.repoPath, tc.args.repoID, tc.args.registeredAppPaths) + got, err := tc.reporter.findRegisteredApps(tc.args.repoPath, tc.args.repoID) assert.Equal(t, tc.wantErr, err != nil) assert.Equal(t, tc.want, got) }) } } -type fakeGitRepo struct { - path string - changedFiles []string - err error -} - -func (f *fakeGitRepo) GetPath() string { - return f.path -} - -func (f *fakeGitRepo) ChangedFiles(_ context.Context, _, _ string) ([]string, error) { - return f.changedFiles, f.err -} - -func TestReporter_findRegisteredApps(t *testing.T) { +func TestReporter_findUnregisteredApps(t *testing.T) { type args struct { - repoID string - repo gitRepo - lastScannedCommit string registeredAppPaths map[string]string + repoPath, repoID string } testcases := []struct { name string @@ -186,33 +170,37 @@ func TestReporter_findRegisteredApps(t *testing.T) { wantErr bool }{ { - name: "no changed file", + name: "file not found", reporter: &Reporter{ + applicationLister: &fakeApplicationLister{}, fileSystem: fstest.MapFS{ - "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte("invalid-text")}, + "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte("")}, }, logger: zap.NewNop(), }, args: args{ - repoID: "repo-1", - repo: &fakeGitRepo{path: "path/to/repo-1", changedFiles: nil}, - lastScannedCommit: "xxx", + repoPath: "invalid", + repoID: "repo-1", + registeredAppPaths: map[string]string{}, }, - want: []*model.ApplicationInfo{}, - wantErr: false, + want: nil, + wantErr: true, }, { - name: "all are unregistered", + name: "all are registered", reporter: &Reporter{ + applicationLister: &fakeApplicationLister{}, fileSystem: fstest.MapFS{ "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte("")}, }, logger: zap.NewNop(), }, args: args{ - repoID: "repo-1", - repo: &fakeGitRepo{path: "path/to/repo-1", changedFiles: []string{"app-1/.pipe.yaml"}}, - lastScannedCommit: "xxx", + repoPath: "path/to/repo-1", + repoID: "repo-1", + registeredAppPaths: map[string]string{ + "repo-1:app-1/.pipe.yaml": "id-1", + }, }, want: []*model.ApplicationInfo{}, wantErr: false, @@ -220,25 +208,24 @@ func TestReporter_findRegisteredApps(t *testing.T) { { name: "invalid app config is contained", reporter: &Reporter{ + applicationLister: &fakeApplicationLister{}, fileSystem: fstest.MapFS{ "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte("invalid-text")}, }, logger: zap.NewNop(), }, args: args{ - repoID: "repo-1", - repo: &fakeGitRepo{path: "path/to/repo-1", changedFiles: []string{"app-1/.pipe.yaml"}}, - lastScannedCommit: "xxx", - registeredAppPaths: map[string]string{ - "repo-1:app-1/.pipe.yaml": "id-1", - }, + repoPath: "path/to/repo-1", + repoID: "repo-1", + registeredAppPaths: map[string]string{}, }, want: []*model.ApplicationInfo{}, wantErr: false, }, { - name: "valid app config that is registered", + name: "valid app config that is unregistered", reporter: &Reporter{ + applicationLister: &fakeApplicationLister{}, fileSystem: fstest.MapFS{ "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte(` apiVersion: pipecd.dev/v1beta1 @@ -251,16 +238,12 @@ spec: logger: zap.NewNop(), }, args: args{ - repoID: "repo-1", - repo: &fakeGitRepo{path: "path/to/repo-1", changedFiles: []string{"app-1/.pipe.yaml"}}, - lastScannedCommit: "xxx", - registeredAppPaths: map[string]string{ - "repo-1:app-1/.pipe.yaml": "id-1", - }, + repoPath: "path/to/repo-1", + repoID: "repo-1", + registeredAppPaths: map[string]string{}, }, want: []*model.ApplicationInfo{ { - Id: "id-1", Name: "app-1", Labels: map[string]string{"key-1": "value-1"}, RepoId: "repo-1", @@ -271,10 +254,11 @@ spec: wantErr: false, }, { - name: "last scanned commit is empty", + name: "valid app config that name isn't default", reporter: &Reporter{ + applicationLister: &fakeApplicationLister{}, fileSystem: fstest.MapFS{ - "path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte(` + "path/to/repo-1/app-1/dev.pipecd.yaml": &fstest.MapFile{Data: []byte(` apiVersion: pipecd.dev/v1beta1 kind: KubernetesApp spec: @@ -285,21 +269,17 @@ spec: logger: zap.NewNop(), }, args: args{ - repoID: "repo-1", - repo: &fakeGitRepo{path: "path/to/repo-1"}, - lastScannedCommit: "", - registeredAppPaths: map[string]string{ - "repo-1:app-1/.pipe.yaml": "id-1", - }, + repoPath: "path/to/repo-1", + repoID: "repo-1", + registeredAppPaths: map[string]string{}, }, want: []*model.ApplicationInfo{ { - Id: "id-1", Name: "app-1", Labels: map[string]string{"key-1": "value-1"}, RepoId: "repo-1", Path: "app-1", - ConfigFilename: ".pipe.yaml", + ConfigFilename: "dev.pipecd.yaml", }, }, wantErr: false, @@ -307,7 +287,7 @@ spec: } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - got, err := tc.reporter.findRegisteredApps(context.Background(), tc.args.repoID, tc.args.repo, tc.args.lastScannedCommit, "", tc.args.registeredAppPaths) + got, err := tc.reporter.findUnregisteredApps(tc.args.repoPath, tc.args.repoID) assert.Equal(t, tc.wantErr, err != nil) assert.Equal(t, tc.want, got) }) From 33fc1060d038e837d1a71ccfa9b750351e4bde03 Mon Sep 17 00:00:00 2001 From: Ryo Nakao Date: Mon, 29 Nov 2021 11:48:50 +0900 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Le Van Nghia --- pkg/app/piped/appconfigreporter/appconfigreporter.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/appconfigreporter/appconfigreporter.go b/pkg/app/piped/appconfigreporter/appconfigreporter.go index 11ac7febf7..525ab23224 100644 --- a/pkg/app/piped/appconfigreporter/appconfigreporter.go +++ b/pkg/app/piped/appconfigreporter/appconfigreporter.go @@ -169,7 +169,7 @@ func (r *Reporter) updateRegisteredApps(ctx context.Context, headCommits map[str for repoID, repo := range r.gitRepos { headCommit := headCommits[repoID] // Skip if the head commit is already scanned. - if lastScannedCommit, ok := r.lastScannedCommits[repoID]; ok && headCommit == lastScannedCommit { + if lc, ok := r.lastScannedCommits[repoID]; ok && headCommit == lc { continue } @@ -201,7 +201,7 @@ func (r *Reporter) updateUnregisteredApps(ctx context.Context, headCommits map[s unregisteredApps := make([]*model.ApplicationInfo, 0) for repoID, repo := range r.gitRepos { headCommit := headCommits[repoID] - if lastScannedCommit, ok := r.lastScannedCommits[repoID]; ok && headCommit == lastScannedCommit { + if lc, ok := r.lastScannedCommits[repoID]; ok && headCommit == lc { // Use the cached apps if the head commit is already scanned. // The unregistered apps sent previously aren't persisted, that's why it has to send them again even if it's scanned one. if apps, ok := r.latestUnregisteredApps[repoID]; ok { From f05df126832bc3da22988d3788313b19199162b3 Mon Sep 17 00:00:00 2001 From: nakabonne Date: Mon, 29 Nov 2021 11:57:30 +0900 Subject: [PATCH 3/5] Disable Kind to be changed --- pkg/app/piped/appconfigreporter/appconfigreporter.go | 3 --- pkg/model/common.proto | 4 ++++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/app/piped/appconfigreporter/appconfigreporter.go b/pkg/app/piped/appconfigreporter/appconfigreporter.go index 525ab23224..f7e5f4e6db 100644 --- a/pkg/app/piped/appconfigreporter/appconfigreporter.go +++ b/pkg/app/piped/appconfigreporter/appconfigreporter.go @@ -269,9 +269,6 @@ func isSynced(appInfo *model.ApplicationInfo, app *model.Application) bool { if appInfo.Name != app.Name { return false } - if appInfo.Kind != app.Kind { - return false - } if len(appInfo.Labels) != len(app.Labels) { return false } diff --git a/pkg/model/common.proto b/pkg/model/common.proto index 022727c96a..1d4b2d65c2 100644 --- a/pkg/model/common.proto +++ b/pkg/model/common.proto @@ -59,9 +59,13 @@ enum SyncStrategy { message ApplicationInfo { string id = 1; string name = 2 [(validate.rules).string.min_len = 1]; + // This field is not allowed to be changed. ApplicationKind kind = 3 [(validate.rules).enum.defined_only = true]; map labels = 4; + // This field is not allowed to be changed. string repo_id = 5 [(validate.rules).string.min_len = 1]; + // This field is not allowed to be changed. string path = 6 [(validate.rules).string.pattern = "^[^/].+$"]; + // This field is not allowed to be changed. string config_filename = 7; } From bb8483359a18c71736a24dac80c5fcbeba154d79 Mon Sep 17 00:00:00 2001 From: nakabonne Date: Mon, 29 Nov 2021 12:05:32 +0900 Subject: [PATCH 4/5] Emit log if kind has been changed --- .../piped/appconfigreporter/appconfigreporter.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/appconfigreporter/appconfigreporter.go b/pkg/app/piped/appconfigreporter/appconfigreporter.go index f7e5f4e6db..e3f9c57d2e 100644 --- a/pkg/app/piped/appconfigreporter/appconfigreporter.go +++ b/pkg/app/piped/appconfigreporter/appconfigreporter.go @@ -255,7 +255,7 @@ func (r *Reporter) findRegisteredApps(repoPath, repoID string) ([]*model.Applica if err != nil { return nil, fmt.Errorf("failed to read application config file: %w", err) } - if isSynced(appCfg, app) { + if r.isSynced(appCfg, app) { continue } appCfg.Id = app.Id @@ -264,7 +264,16 @@ func (r *Reporter) findRegisteredApps(repoPath, repoID string) ([]*model.Applica return apps, nil } -func isSynced(appInfo *model.ApplicationInfo, app *model.Application) bool { +func (r *Reporter) isSynced(appInfo *model.ApplicationInfo, app *model.Application) bool { + if appInfo.Kind != app.Kind { + r.logger.Warn("kind in application config has been changed which isn't allowed", + zap.String("app-id", app.Id), + zap.String("repo-id", app.GitPath.Repo.Id), + zap.String("path", app.GitPath.Path), + zap.String("config-filename", app.GitPath.ConfigFilename), + ) + } + // TODO: Make it possible to follow the ApplicationInfo field changes if appInfo.Name != app.Name { return false From 1acc33ea60aa55e6e99f7da0dcf820238872a11e Mon Sep 17 00:00:00 2001 From: nakabonne Date: Mon, 29 Nov 2021 12:14:10 +0900 Subject: [PATCH 5/5] Don't update kind --- pkg/app/api/grpcapi/piped_api.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/app/api/grpcapi/piped_api.go b/pkg/app/api/grpcapi/piped_api.go index a307459d62..79eed1aed4 100644 --- a/pkg/app/api/grpcapi/piped_api.go +++ b/pkg/app/api/grpcapi/piped_api.go @@ -980,7 +980,6 @@ func (a *PipedAPI) UpdateApplicationConfigurations(ctx context.Context, req *pip for _, appInfo := range req.Applications { updater := func(app *model.Application) error { app.Name = appInfo.Name - app.Kind = appInfo.Kind app.Labels = appInfo.Labels return nil }