From 47e037f3b7b3ba87aab7aad26dda3caa6dc0e41f Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 13:56:03 +0900 Subject: [PATCH 01/20] Add plugins field to the application config spec Signed-off-by: Yoshiki Fujikane --- pkg/configv1/application.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/configv1/application.go b/pkg/configv1/application.go index f1d69f0a04..46cd0801ec 100644 --- a/pkg/configv1/application.go +++ b/pkg/configv1/application.go @@ -58,6 +58,8 @@ type GenericApplicationSpec struct { EventWatcher []EventWatcherConfig `json:"eventWatcher"` // Configuration for drift detection DriftDetection *DriftDetection `json:"driftDetection"` + // List of the plugin name + Plugins []string `json:"plugins"` } type DeploymentPlanner struct { From f6d66dd663f8ea632d2cb3e179871a5cff1a84bc Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 13:56:26 +0900 Subject: [PATCH 02/20] Add plugin registry Signed-off-by: Yoshiki Fujikane --- pkg/plugin/registry/registry.go | 89 ++++++++ pkg/plugin/registry/registry_test.go | 300 +++++++++++++++++++++++++++ 2 files changed, 389 insertions(+) create mode 100644 pkg/plugin/registry/registry.go create mode 100644 pkg/plugin/registry/registry_test.go diff --git a/pkg/plugin/registry/registry.go b/pkg/plugin/registry/registry.go new file mode 100644 index 0000000000..9d78ac362e --- /dev/null +++ b/pkg/plugin/registry/registry.go @@ -0,0 +1,89 @@ +package registry + +import ( + "fmt" + + config "github.com/pipe-cd/pipecd/pkg/configv1" + pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" +) + +// PluginRegistry is the interface that provides methods to get plugin clients. +type PluginRegistry interface { + GetPluginClientByStageName(name string) (pluginapi.PluginClient, error) + GetPluginsByAppConfig(cfg *config.GenericApplicationSpec) ([]pluginapi.PluginClient, error) +} + +type pluginRegistry struct { + nameBasedPlugins map[string]pluginapi.PluginClient + stageBasedPlugins map[string]pluginapi.PluginClient + + // TODO: add more fields if needed (e.g. deploymentBasedPlugins, livestateBasedPlugins) +} + +// GetPluginClientByStageName returns the plugin client based on the given stage name. +func (pr *pluginRegistry) GetPluginClientByStageName(name string) (pluginapi.PluginClient, error) { + plugin, ok := pr.stageBasedPlugins[name] + if !ok { + return nil, fmt.Errorf("no plugin found for the specified stage") + } + + return plugin, nil +} + +// GetPluginsByAppConfig returns the plugin clients based on the given configuration. +// The priority of determining plugins is as follows: +// 1. If the pipeline is specified, it will determine the plugins based on the pipeline stages. +// 2. If the plugins are specified, it will determine the plugins based on the plugin names. +// 3. If neither the pipeline nor the plugins are specified, it will return an error. +func (pr *pluginRegistry) GetPluginsByAppConfig(cfg *config.GenericApplicationSpec) ([]pluginapi.PluginClient, error) { + if cfg.Pipeline != nil { + return pr.getPluginsByPipeline(cfg.Pipeline) + } + + if cfg.Plugins != nil { + return pr.getPluginsByPluginNames(cfg.Plugins) + } + + return nil, fmt.Errorf("no plugin specified") +} + +func (pr *pluginRegistry) getPluginsByPipeline(pipeline *config.DeploymentPipeline) ([]pluginapi.PluginClient, error) { + plugins := make([]pluginapi.PluginClient, 0, len(pipeline.Stages)) + + if len(pipeline.Stages) == 0 { + return nil, fmt.Errorf("no plugin found for the specified stages") + } + + for _, stage := range pipeline.Stages { + plugin, ok := pr.stageBasedPlugins[stage.Name.String()] + if ok { + plugins = append(plugins, plugin) + } + } + + if len(plugins) == 0 { + return nil, fmt.Errorf("no plugin found for the specified stages") + } + + return plugins, nil +} + +func (pr *pluginRegistry) getPluginsByPluginNames(names []string) ([]pluginapi.PluginClient, error) { + if len(names) == 0 { + return nil, fmt.Errorf("no plugin specified") + } + + plugins := make([]pluginapi.PluginClient, 0, len(names)) + for _, name := range names { + plugin, ok := pr.nameBasedPlugins[name] + if ok { + plugins = append(plugins, plugin) + } + } + + if len(plugins) == 0 { + return nil, fmt.Errorf("no plugin found for the specified stages") + } + + return plugins, nil +} diff --git a/pkg/plugin/registry/registry_test.go b/pkg/plugin/registry/registry_test.go new file mode 100644 index 0000000000..db51088472 --- /dev/null +++ b/pkg/plugin/registry/registry_test.go @@ -0,0 +1,300 @@ +package registry + +import ( + "testing" + + config "github.com/pipe-cd/pipecd/pkg/configv1" + pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" + "github.com/stretchr/testify/assert" +) + +type mockPluginClient struct { + pluginapi.PluginClient + name string +} + +func createMockPluginClient(name string) pluginapi.PluginClient { + return &mockPluginClient{ + name: name, + } +} +func TestPluginDeterminer_GetPluginsByAppConfig(t *testing.T) { + tests := []struct { + name string + cfg *config.GenericApplicationSpec + setup func() *pluginRegistry + expected []pluginapi.PluginClient + wantErr bool + }{ + { + name: "get plugins by pipeline", + cfg: &config.GenericApplicationSpec{ + Pipeline: &config.DeploymentPipeline{ + Stages: []config.PipelineStage{ + {Name: "stage1"}, + {Name: "stage2"}, + }, + }, + Plugins: nil, + }, + setup: func() *pluginRegistry { + return &pluginRegistry{ + stageBasedPlugins: map[string]pluginapi.PluginClient{ + "stage1": createMockPluginClient("stage1"), + "stage2": createMockPluginClient("stage2"), + }, + } + }, + expected: []pluginapi.PluginClient{ + createMockPluginClient("stage1"), + createMockPluginClient("stage2"), + }, + wantErr: false, + }, + { + name: "get plugins by plugin names", + cfg: &config.GenericApplicationSpec{ + Pipeline: nil, + Plugins: []string{"plugin1", "plugin2"}, + }, + setup: func() *pluginRegistry { + return &pluginRegistry{ + nameBasedPlugins: map[string]pluginapi.PluginClient{ + "plugin1": createMockPluginClient("plugin1"), + "plugin2": createMockPluginClient("plugin2"), + }, + } + }, + expected: []pluginapi.PluginClient{ + createMockPluginClient("plugin1"), + createMockPluginClient("plugin2"), + }, + wantErr: false, + }, + { + name: "get plugins by pipeline when both pipeline and plugins are specified", + cfg: &config.GenericApplicationSpec{ + Pipeline: &config.DeploymentPipeline{ + Stages: []config.PipelineStage{ + {Name: "stage1"}, + {Name: "stage2"}, + }, + }, + Plugins: []string{"plugin1", "plugin2"}, + }, + setup: func() *pluginRegistry { + return &pluginRegistry{ + stageBasedPlugins: map[string]pluginapi.PluginClient{ + "stage1": createMockPluginClient("stage1"), + "stage2": createMockPluginClient("stage2"), + }, + nameBasedPlugins: map[string]pluginapi.PluginClient{ + "plugin1": createMockPluginClient("plugin1"), + "plugin2": createMockPluginClient("plugin2"), + }, + } + }, + expected: []pluginapi.PluginClient{ + createMockPluginClient("stage1"), + createMockPluginClient("stage2"), + }, + wantErr: false, + }, + { + name: "no plugins found when no pipeline or plugins specified", + cfg: &config.GenericApplicationSpec{}, + setup: func() *pluginRegistry { + return &pluginRegistry{ + nameBasedPlugins: map[string]pluginapi.PluginClient{}, + } + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pr := tt.setup() + plugins, err := pr.GetPluginsByAppConfig(tt.cfg) + assert.Equal(t, tt.expected, plugins) + assert.Equal(t, tt.wantErr, err != nil) + }) + } +} +func TestPluginRegistry_getPluginsByPipeline(t *testing.T) { + tests := []struct { + name string + pipeline *config.DeploymentPipeline + setup func() *pluginRegistry + expected []pluginapi.PluginClient + wantErr bool + }{ + { + name: "get plugins by valid pipeline stages", + pipeline: &config.DeploymentPipeline{ + Stages: []config.PipelineStage{ + {Name: "stage1"}, + {Name: "stage2"}, + }, + }, + setup: func() *pluginRegistry { + return &pluginRegistry{ + stageBasedPlugins: map[string]pluginapi.PluginClient{ + "stage1": createMockPluginClient("stage1"), + "stage2": createMockPluginClient("stage2"), + }, + } + }, + expected: []pluginapi.PluginClient{ + createMockPluginClient("stage1"), + createMockPluginClient("stage2"), + }, + wantErr: false, + }, + { + name: "no plugins found for empty pipeline stages", + pipeline: &config.DeploymentPipeline{ + Stages: []config.PipelineStage{}, + }, + setup: func() *pluginRegistry { + return &pluginRegistry{ + stageBasedPlugins: map[string]pluginapi.PluginClient{}, + } + }, + expected: nil, + wantErr: true, + }, + { + name: "no plugins found for non-existent pipeline stages", + pipeline: &config.DeploymentPipeline{ + Stages: []config.PipelineStage{ + {Name: "stage1"}, + {Name: "stage2"}, + }, + }, + setup: func() *pluginRegistry { + return &pluginRegistry{ + stageBasedPlugins: map[string]pluginapi.PluginClient{}, + } + }, + expected: nil, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pr := tt.setup() + plugins, err := pr.getPluginsByPipeline(tt.pipeline) + assert.Equal(t, tt.expected, plugins) + assert.Equal(t, tt.wantErr, err != nil) + }) + } +} +func TestPluginRegistry_getPluginsByPluginNames(t *testing.T) { + tests := []struct { + name string + pluginNames []string + setup func() *pluginRegistry + expected []pluginapi.PluginClient + wantErr bool + }{ + { + name: "get plugins by valid plugin names", + pluginNames: []string{"plugin1", "plugin2"}, + setup: func() *pluginRegistry { + return &pluginRegistry{ + nameBasedPlugins: map[string]pluginapi.PluginClient{ + "plugin1": createMockPluginClient("plugin1"), + "plugin2": createMockPluginClient("plugin2"), + }, + } + }, + expected: []pluginapi.PluginClient{ + createMockPluginClient("plugin1"), + createMockPluginClient("plugin2"), + }, + wantErr: false, + }, + { + name: "no plugins found for empty plugin names", + pluginNames: []string{}, + setup: func() *pluginRegistry { + return &pluginRegistry{ + nameBasedPlugins: map[string]pluginapi.PluginClient{ + "plugin1": createMockPluginClient("plugin1"), + "plugin2": createMockPluginClient("plugin2"), + }, + } + }, + wantErr: true, + }, + { + name: "no plugins found for non-existent plugin names", + pluginNames: []string{"plugin1", "plugin2"}, + setup: func() *pluginRegistry { + return &pluginRegistry{ + nameBasedPlugins: map[string]pluginapi.PluginClient{ + "plugin3": createMockPluginClient("plugin3"), + "plugin4": createMockPluginClient("plugin4"), + }, + } + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pr := tt.setup() + plugins, err := pr.getPluginsByPluginNames(tt.pluginNames) + assert.Equal(t, tt.expected, plugins) + assert.Equal(t, tt.wantErr, err != nil) + }) + } +} +func TestPluginRegistry_GetPluginClientByStageName(t *testing.T) { + tests := []struct { + name string + stage string + setup func() *pluginRegistry + expected pluginapi.PluginClient + wantErr bool + }{ + { + name: "get plugin by valid stage name", + stage: "stage1", + setup: func() *pluginRegistry { + return &pluginRegistry{ + stageBasedPlugins: map[string]pluginapi.PluginClient{ + "stage1": createMockPluginClient("stage1"), + }, + } + }, + expected: createMockPluginClient("stage1"), + wantErr: false, + }, + { + name: "no plugin found for non-existent stage name", + stage: "stage2", + setup: func() *pluginRegistry { + return &pluginRegistry{ + stageBasedPlugins: map[string]pluginapi.PluginClient{ + "stage1": createMockPluginClient("stage1"), + }, + } + }, + expected: nil, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pr := tt.setup() + plugin, err := pr.GetPluginClientByStageName(tt.stage) + assert.Equal(t, tt.expected, plugin) + assert.Equal(t, tt.wantErr, err != nil) + }) + } +} From ba7e8119b64447cfd6f7d9c250afec7b9d9fe3de Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 15:23:34 +0900 Subject: [PATCH 03/20] Use plugin registry Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/cmd/piped/piped.go | 18 +- pkg/app/pipedv1/controller/controller.go | 7 + pkg/app/pipedv1/controller/planner.go | 26 +- pkg/app/pipedv1/controller/planner_test.go | 350 ++++++++++++--------- pkg/plugin/registry/registry.go | 35 ++- 5 files changed, 290 insertions(+), 146 deletions(-) diff --git a/pkg/app/pipedv1/cmd/piped/piped.go b/pkg/app/pipedv1/cmd/piped/piped.go index 3270095681..be98e1b637 100644 --- a/pkg/app/pipedv1/cmd/piped/piped.go +++ b/pkg/app/pipedv1/cmd/piped/piped.go @@ -71,6 +71,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/lifecycle" "github.com/pipe-cd/pipecd/pkg/model" pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" + pluginRegistry "github.com/pipe-cd/pipecd/pkg/plugin/registry" "github.com/pipe-cd/pipecd/pkg/rpc" "github.com/pipe-cd/pipecd/pkg/rpc/rpcauth" "github.com/pipe-cd/pipecd/pkg/rpc/rpcclient" @@ -349,6 +350,8 @@ func (p *piped) run(ctx context.Context, input cli.Input) (runErr error) { // Make grpc clients to connect to plugins. pluginClis := make([]pluginapi.PluginClient, 0, len(cfg.Plugins)) + + plugins := make([]pluginRegistry.Plugin, 0, len(cfg.Plugins)) options := []rpcclient.DialOption{ rpcclient.WithBlock(), rpcclient.WithInsecure(), @@ -356,9 +359,19 @@ func (p *piped) run(ctx context.Context, input cli.Input) (runErr error) { for _, plg := range cfg.Plugins { cli, err := pluginapi.NewClient(ctx, net.JoinHostPort("localhost", strconv.Itoa(plg.Port)), options...) if err != nil { - input.Logger.Error("failed to create client to connect plugin", zap.String("plugin", plg.Name), zap.Error(err)) + return err } - pluginClis = append(pluginClis, cli) + + plugins = append(plugins, pluginRegistry.Plugin{ + Name: plg.Name, + Cli: cli, + }) + } + + pluginRegistry, err := pluginRegistry.NewPluginRegistry(ctx, plugins) + if err != nil { + input.Logger.Error("failed to create plugin registry", zap.Error(err)) + return err } // Start running application live state reporter. @@ -384,6 +397,7 @@ func (p *piped) run(ctx context.Context, input cli.Input) (runErr error) { apiClient, gitClient, pluginClis, + pluginRegistry, deploymentLister, commandLister, notifier, diff --git a/pkg/app/pipedv1/controller/controller.go b/pkg/app/pipedv1/controller/controller.go index 51f1ce7634..54b7b31556 100644 --- a/pkg/app/pipedv1/controller/controller.go +++ b/pkg/app/pipedv1/controller/controller.go @@ -39,6 +39,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/model" pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment" + "github.com/pipe-cd/pipecd/pkg/plugin/registry" ) type apiClient interface { @@ -98,6 +99,9 @@ type controller struct { // gRPC clients to communicate with plugins. pluginClients []pluginapi.PluginClient + // The registry of all plugins. + pluginRegistry registry.PluginRegistry + // Map from stage name to the plugin client. stageBasedPluginsMap map[string]pluginapi.PluginClient // Map from application ID to the planner @@ -132,6 +136,7 @@ func NewController( apiClient apiClient, gitClient gitClient, pluginClients []pluginapi.PluginClient, + pluginRegistry registry.PluginRegistry, deploymentLister deploymentLister, commandLister commandLister, notifier notifier, @@ -145,6 +150,7 @@ func NewController( apiClient: apiClient, gitClient: gitClient, pluginClients: pluginClients, + pluginRegistry: pluginRegistry, deploymentLister: deploymentLister, commandLister: commandLister, notifier: notifier, @@ -449,6 +455,7 @@ func (c *controller) startNewPlanner(ctx context.Context, d *model.Deployment) ( configFilename, workingDir, c.pluginClients, // FIXME: Find a way to ensure the plugins only related to deployment. + c.pluginRegistry, c.stageBasedPluginsMap, c.apiClient, c.gitClient, diff --git a/pkg/app/pipedv1/controller/planner.go b/pkg/app/pipedv1/controller/planner.go index ce5140f88f..b90dca6c9e 100644 --- a/pkg/app/pipedv1/controller/planner.go +++ b/pkg/app/pipedv1/controller/planner.go @@ -36,6 +36,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/model" pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment" + "github.com/pipe-cd/pipecd/pkg/plugin/registry" "github.com/pipe-cd/pipecd/pkg/regexpool" ) @@ -79,6 +80,9 @@ type planner struct { // which encrypted using PipeCD built-in secret management. secretDecrypter secretDecrypter + // The pluginRegistry is used to determine which plugins to be used + pluginRegistry registry.PluginRegistry + logger *zap.Logger tracer trace.Tracer @@ -97,6 +101,7 @@ func newPlanner( lastSuccessfulConfigFilename string, workingDir string, pluginClients []pluginapi.PluginClient, + pluginRegistry registry.PluginRegistry, stageBasedPluginsMap map[string]pluginapi.PluginClient, apiClient apiClient, gitClient gitClient, @@ -121,6 +126,7 @@ func newPlanner( workingDir: workingDir, stageBasedPluginsMap: stageBasedPluginsMap, plugins: pluginClients, + pluginRegistry: pluginRegistry, apiClient: apiClient, gitClient: gitClient, metadataStore: metadatastore.NewMetadataStore(apiClient, d), @@ -226,6 +232,20 @@ func (p *planner) Run(ctx context.Context) error { runningDS = rds.ToPluginDeploySource() } + // determine plugin based on the application config from targetDSP. + cfg, err := config.DecodeYAML[*config.GenericApplicationSpec](targetDS.GetApplicationConfig()) + if err != nil { + p.logger.Error("unable to parse application config", zap.Error(err)) + return err + } + + plugins, err := p.pluginRegistry.GetPluginsByAppConfig(cfg.Spec) + if err != nil { + p.logger.Error("unable to determine plugin clients", zap.Error(err)) + return err + } + p.plugins = plugins + out, err := p.buildPlan(ctx, runningDS, targetDS) // If the deployment was already cancelled, we ignore the plan result. @@ -462,9 +482,9 @@ func (p *planner) buildPipelineSyncStages(ctx context.Context, cfg *config.Gener // Build stages config for each plugin. for i := range stagesCfg { stageCfg := stagesCfg[i] - plg, ok := p.stageBasedPluginsMap[stageCfg.Name.String()] - if !ok { - return nil, fmt.Errorf("unable to find plugin for stage %q", stageCfg.Name.String()) + plg, err := p.pluginRegistry.GetPluginClientByStageName(stageCfg.Name.String()) + if err != nil { + return nil, err } stagesCfgPerPlugin[plg] = append(stagesCfgPerPlugin[plg], &deployment.BuildPipelineSyncStagesRequest_StageConfig{ diff --git a/pkg/app/pipedv1/controller/planner_test.go b/pkg/app/pipedv1/controller/planner_test.go index 9fb2e97eb6..9cbc8dbe13 100644 --- a/pkg/app/pipedv1/controller/planner_test.go +++ b/pkg/app/pipedv1/controller/planner_test.go @@ -29,6 +29,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/model" pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment" + "github.com/pipe-cd/pipecd/pkg/plugin/registry" ) type fakePlugin struct { @@ -262,37 +263,45 @@ func TestBuildPipelineSyncStages(t *testing.T) { testcases := []struct { name string - plugins []pluginapi.PluginClient + pluginRegistry registry.PluginRegistry cfg *config.GenericApplicationSpec wantErr bool expectedStages []*model.PipelineStage }{ { name: "only one plugin", - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - pipelineStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - Index: 0, - Name: "plugin-1-stage-1", - }, - { - Id: "plugin-1-stage-2", - Index: 1, - Name: "plugin-1-stage-2", - }, - }, - rollbackStages: []*model.PipelineStage{ - { - Id: "plugin-1-rollback", - Index: 0, - Name: "plugin-1-rollback", - Rollback: true, - }, - }, - }, - }, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Index: 0, + Name: "plugin-1-stage-1", + }, + { + Id: "plugin-1-stage-2", + Index: 1, + Name: "plugin-1-stage-2", + }, + }, + rollbackStages: []*model.PipelineStage{ + { + Id: "plugin-1-rollback", + Index: 0, + Name: "plugin-1-rollback", + Rollback: true, + }, + }, + }, + }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), @@ -333,44 +342,60 @@ func TestBuildPipelineSyncStages(t *testing.T) { }, { name: "multi plugins single rollback", - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - pipelineStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - Name: "plugin-1-stage-1", - }, - { - Id: "plugin-1-stage-2", - Name: "plugin-1-stage-2", - }, - { - Id: "plugin-1-stage-3", - Name: "plugin-1-stage-3", - }, - }, - rollbackStages: []*model.PipelineStage{ - { - Id: "plugin-1-rollback", - Index: 0, - Name: "plugin-1-rollback", - Rollback: true, - }, - }, - }, - &fakePlugin{ - pipelineStages: []*model.PipelineStage{ - { - Id: "plugin-2-stage-1", - Name: "plugin-2-stage-1", - }, - { - Id: "plugin-2-stage-2", - Name: "plugin-2-stage-2", + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Index: 0, + Name: "plugin-1-stage-1", + }, + { + Id: "plugin-1-stage-2", + Index: 1, + Name: "plugin-1-stage-2", + }, + { + Id: "plugin-1-stage-3", + Index: 2, + Name: "plugin-1-stage-3", + }, + }, + rollbackStages: []*model.PipelineStage{ + { + Id: "plugin-1-rollback", + Index: 0, + Name: "plugin-1-rollback", + Rollback: true, + }, + }, }, }, - }, - }, + { + Name: "plugin-2", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "plugin-2-stage-1", + Index: 0, + Name: "plugin-2-stage-1", + }, + { + Id: "plugin-2-stage-2", + Index: 1, + Name: "plugin-2-stage-2", + }, + }, + }, + }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), @@ -441,48 +466,63 @@ func TestBuildPipelineSyncStages(t *testing.T) { }, { name: "multi plugins multi rollback", - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - pipelineStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - Name: "plugin-1-stage-1", - }, - { - Id: "plugin-1-stage-2", - Name: "plugin-1-stage-2", - }, - { - Id: "plugin-1-stage-3", - Name: "plugin-1-stage-3", - }, - }, - rollbackStages: []*model.PipelineStage{ - { - Id: "plugin-1-rollback", - Index: 0, - Name: "plugin-1-rollback", - Rollback: true, - }, - }, - }, - &fakePlugin{ - pipelineStages: []*model.PipelineStage{ - { - Id: "plugin-2-stage-1", - Name: "plugin-2-stage-1", - }, - }, - rollbackStages: []*model.PipelineStage{ - { - Id: "plugin-2-rollback", - Index: 2, - Name: "plugin-2-rollback", - Rollback: true, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Index: 0, + Name: "plugin-1-stage-1", + }, + { + Id: "plugin-1-stage-2", + Index: 1, + Name: "plugin-1-stage-2", + }, + { + Id: "plugin-1-stage-3", + Index: 2, + Name: "plugin-1-stage-3", + }, + }, + rollbackStages: []*model.PipelineStage{ + { + Id: "plugin-1-rollback", + Index: 0, + Name: "plugin-1-rollback", + Rollback: true, + }, + }, }, }, - }, - }, + { + Name: "plugin-2", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "plugin-2-stage-1", + Index: 0, + Name: "plugin-2-stage-1", + }, + }, + rollbackStages: []*model.PipelineStage{ + { + Id: "plugin-2-rollback", + Index: 2, + Name: "plugin-2-rollback", + Rollback: true, + }, + }, + }, + }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), @@ -551,16 +591,8 @@ func TestBuildPipelineSyncStages(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - stageBasedPluginMap := make(map[string]pluginapi.PluginClient) - for _, p := range tc.plugins { - stages, _ := p.FetchDefinedStages(context.TODO(), &deployment.FetchDefinedStagesRequest{}) - for _, s := range stages.Stages { - stageBasedPluginMap[s] = p - } - } planner := &planner{ - plugins: tc.plugins, - stageBasedPluginsMap: stageBasedPluginMap, + pluginRegistry: tc.pluginRegistry, } stages, err := planner.buildPipelineSyncStages(context.TODO(), tc.cfg) require.Equal(t, tc.wantErr, err != nil) @@ -576,6 +608,7 @@ func TestPlanner_BuildPlan(t *testing.T) { name string isFirstDeploy bool plugins []pluginapi.PluginClient + pluginRegistry registry.PluginRegistry cfg *config.GenericApplicationSpec deployment *model.Deployment wantErr bool @@ -626,17 +659,26 @@ func TestPlanner_BuildPlan(t *testing.T) { { name: "pipeline sync strategy triggered by web console", isFirstDeploy: false, - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - pipelineStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - Name: "plugin-1-stage-1", - Visible: true, - }, - }, - }, - }, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Index: 0, + Name: "plugin-1-stage-1", + Visible: true, + }, + }, + }, + }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), @@ -718,17 +760,26 @@ func TestPlanner_BuildPlan(t *testing.T) { { name: "pipeline sync due to alwaysUsePipeline", isFirstDeploy: false, - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - pipelineStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - Name: "plugin-1-stage-1", - Visible: true, - }, - }, - }, - }, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Index: 0, + Name: "plugin-1-stage-1", + Visible: true, + }, + }, + }, + }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AlwaysUsePipeline: true, @@ -837,6 +888,32 @@ func TestPlanner_BuildPlan(t *testing.T) { }, }, }, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Index: 0, + Name: "plugin-1-stage-1", + Visible: true, + }, + }, + quickStages: []*model.PipelineStage{ + { + Id: "plugin-1-quick-stage-1", + Visible: true, + }, + }, + }, + }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), @@ -877,16 +954,9 @@ func TestPlanner_BuildPlan(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - stageBasedPluginMap := make(map[string]pluginapi.PluginClient) - for _, p := range tc.plugins { - stages, _ := p.FetchDefinedStages(context.TODO(), &deployment.FetchDefinedStagesRequest{}) - for _, s := range stages.Stages { - stageBasedPluginMap[s] = p - } - } planner := &planner{ plugins: tc.plugins, - stageBasedPluginsMap: stageBasedPluginMap, + pluginRegistry: tc.pluginRegistry, deployment: tc.deployment, lastSuccessfulCommitHash: "", lastSuccessfulConfigFilename: "", diff --git a/pkg/plugin/registry/registry.go b/pkg/plugin/registry/registry.go index 9d78ac362e..92c27f72ea 100644 --- a/pkg/plugin/registry/registry.go +++ b/pkg/plugin/registry/registry.go @@ -1,12 +1,19 @@ package registry import ( + "context" "fmt" config "github.com/pipe-cd/pipecd/pkg/configv1" pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" + "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment" ) +type Plugin struct { + Name string + Cli pluginapi.PluginClient +} + // PluginRegistry is the interface that provides methods to get plugin clients. type PluginRegistry interface { GetPluginClientByStageName(name string) (pluginapi.PluginClient, error) @@ -20,6 +27,32 @@ type pluginRegistry struct { // TODO: add more fields if needed (e.g. deploymentBasedPlugins, livestateBasedPlugins) } +// NewPluginRegistry creates a new PluginRegistry based on the given plugins. +func NewPluginRegistry(ctx context.Context, plugins []Plugin) (PluginRegistry, error) { + nameBasedPlugins := make(map[string]pluginapi.PluginClient) + stageBasedPlugins := make(map[string]pluginapi.PluginClient) + + for _, plg := range plugins { + // add the plugin to the name-based plugins + nameBasedPlugins[plg.Name] = plg.Cli + + // add the plugin to the stage-based plugins + res, err := plg.Cli.FetchDefinedStages(ctx, &deployment.FetchDefinedStagesRequest{}) + if err != nil { + return nil, err + } + + for _, stage := range res.Stages { + stageBasedPlugins[stage] = plg.Cli + } + } + + return &pluginRegistry{ + nameBasedPlugins: nameBasedPlugins, + stageBasedPlugins: stageBasedPlugins, + }, nil +} + // GetPluginClientByStageName returns the plugin client based on the given stage name. func (pr *pluginRegistry) GetPluginClientByStageName(name string) (pluginapi.PluginClient, error) { plugin, ok := pr.stageBasedPlugins[name] @@ -36,7 +69,7 @@ func (pr *pluginRegistry) GetPluginClientByStageName(name string) (pluginapi.Plu // 2. If the plugins are specified, it will determine the plugins based on the plugin names. // 3. If neither the pipeline nor the plugins are specified, it will return an error. func (pr *pluginRegistry) GetPluginsByAppConfig(cfg *config.GenericApplicationSpec) ([]pluginapi.PluginClient, error) { - if cfg.Pipeline != nil { + if cfg.Pipeline != nil && len(cfg.Pipeline.Stages) > 0 { return pr.getPluginsByPipeline(cfg.Pipeline) } From eecd6c00be280402391200f4f1bf430382e46757 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 19:00:52 +0900 Subject: [PATCH 04/20] Remove planner.plugins Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/controller/controller.go | 1 - pkg/app/pipedv1/controller/planner.go | 46 ++-- pkg/app/pipedv1/controller/planner_test.go | 272 ++++++++++++++------- 3 files changed, 196 insertions(+), 123 deletions(-) diff --git a/pkg/app/pipedv1/controller/controller.go b/pkg/app/pipedv1/controller/controller.go index 54b7b31556..d7ea1dd21d 100644 --- a/pkg/app/pipedv1/controller/controller.go +++ b/pkg/app/pipedv1/controller/controller.go @@ -454,7 +454,6 @@ func (c *controller) startNewPlanner(ctx context.Context, d *model.Deployment) ( commitHash, configFilename, workingDir, - c.pluginClients, // FIXME: Find a way to ensure the plugins only related to deployment. c.pluginRegistry, c.stageBasedPluginsMap, c.apiClient, diff --git a/pkg/app/pipedv1/controller/planner.go b/pkg/app/pipedv1/controller/planner.go index b90dca6c9e..63f7dac391 100644 --- a/pkg/app/pipedv1/controller/planner.go +++ b/pkg/app/pipedv1/controller/planner.go @@ -58,9 +58,6 @@ type planner struct { lastSuccessfulConfigFilename string workingDir string - // The plugin clients are used to call plugin that actually - // performs planning deployment. - plugins []pluginapi.PluginClient // The map used to know which plugin is incharged for a given stage // of the current deployment. stageBasedPluginsMap map[string]pluginapi.PluginClient @@ -100,7 +97,6 @@ func newPlanner( lastSuccessfulCommitHash string, lastSuccessfulConfigFilename string, workingDir string, - pluginClients []pluginapi.PluginClient, pluginRegistry registry.PluginRegistry, stageBasedPluginsMap map[string]pluginapi.PluginClient, apiClient apiClient, @@ -125,7 +121,6 @@ func newPlanner( lastSuccessfulConfigFilename: lastSuccessfulConfigFilename, workingDir: workingDir, stageBasedPluginsMap: stageBasedPluginsMap, - plugins: pluginClients, pluginRegistry: pluginRegistry, apiClient: apiClient, gitClient: gitClient, @@ -232,20 +227,6 @@ func (p *planner) Run(ctx context.Context) error { runningDS = rds.ToPluginDeploySource() } - // determine plugin based on the application config from targetDSP. - cfg, err := config.DecodeYAML[*config.GenericApplicationSpec](targetDS.GetApplicationConfig()) - if err != nil { - p.logger.Error("unable to parse application config", zap.Error(err)) - return err - } - - plugins, err := p.pluginRegistry.GetPluginsByAppConfig(cfg.Spec) - if err != nil { - p.logger.Error("unable to determine plugin clients", zap.Error(err)) - return err - } - p.plugins = plugins - out, err := p.buildPlan(ctx, runningDS, targetDS) // If the deployment was already cancelled, we ignore the plan result. @@ -286,8 +267,17 @@ func (p *planner) buildPlan(ctx context.Context, runningDS, targetDS *deployment TargetDeploymentSource: targetDS, } + cfg, err := config.DecodeYAML[*config.GenericApplicationSpec](targetDS.GetApplicationConfig()) + if err != nil { + p.logger.Error("unable to parse application config", zap.Error(err)) + return nil, err + } + spec := cfg.Spec + + plugins, err := p.pluginRegistry.GetPluginsByAppConfig(spec) + // Build deployment target versions. - for _, plg := range p.plugins { + for _, plg := range plugins { vRes, err := plg.DetermineVersions(ctx, &deployment.DetermineVersionsRequest{Input: input}) if err != nil { p.logger.Warn("unable to determine versions", zap.Error(err)) @@ -304,13 +294,6 @@ func (p *planner) buildPlan(ctx context.Context, runningDS, targetDS *deployment } } - cfg, err := config.DecodeYAML[*config.GenericApplicationSpec](targetDS.GetApplicationConfig()) - if err != nil { - p.logger.Error("unable to parse application config", zap.Error(err)) - return nil, err - } - spec := cfg.Spec - // In case the strategy has been decided by trigger. // For example: user triggered the deployment via web console. switch p.deployment.Trigger.SyncStrategy { @@ -395,7 +378,7 @@ func (p *planner) buildPlan(ctx context.Context, runningDS, targetDS *deployment summary string ) // Build plan based on plugins determined strategy - for _, plg := range p.plugins { + for _, plg := range plugins { res, err := plg.DetermineStrategy(ctx, &deployment.DetermineStrategyRequest{Input: input}) if err != nil { p.logger.Warn("Unable to determine strategy using current plugin", zap.Error(err)) @@ -441,7 +424,12 @@ func (p *planner) buildQuickSyncStages(ctx context.Context, cfg *config.GenericA rollbackStages = []*model.PipelineStage{} rollback = *cfg.Planner.AutoRollback ) - for _, plg := range p.plugins { + + plugins, err := p.pluginRegistry.GetPluginsByAppConfig(cfg) + if err != nil { + return nil, err + } + for _, plg := range plugins { res, err := plg.BuildQuickSyncStages(ctx, &deployment.BuildQuickSyncStagesRequest{Rollback: rollback}) if err != nil { return nil, fmt.Errorf("failed to build quick sync stage deployment (%w)", err) diff --git a/pkg/app/pipedv1/controller/planner_test.go b/pkg/app/pipedv1/controller/planner_test.go index 9cbc8dbe13..fa01387bd0 100644 --- a/pkg/app/pipedv1/controller/planner_test.go +++ b/pkg/app/pipedv1/controller/planner_test.go @@ -108,139 +108,190 @@ func TestBuildQuickSyncStages(t *testing.T) { testcases := []struct { name string - plugins []pluginapi.PluginClient + pluginRegistry registry.PluginRegistry cfg *config.GenericApplicationSpec wantErr bool expectedStages []*model.PipelineStage }{ { name: "only one plugin", - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - quickStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - }, - }, - rollbackStages: []*model.PipelineStage{ - { - Id: "plugin-1-rollback", - Rollback: true, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + quickStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", + }, + }, + rollbackStages: []*model.PipelineStage{ + { + Id: "plugin-1-rollback", + Name: "plugin-1-rollback", + Rollback: true, + }, + }, }, }, - }, - }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), }, + Plugins: []string{"plugin-1"}, }, wantErr: false, expectedStages: []*model.PipelineStage{ { - Id: "plugin-1-stage-1", + Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", }, { Id: "plugin-1-rollback", + Name: "plugin-1-rollback", Rollback: true, }, }, }, { name: "multi plugins", - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - quickStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - }, - }, - rollbackStages: []*model.PipelineStage{ - { - Id: "plugin-1-rollback", - Rollback: true, - }, - }, - }, - &fakePlugin{ - quickStages: []*model.PipelineStage{ - { - Id: "plugin-2-stage-1", + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + quickStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", + }, + }, + rollbackStages: []*model.PipelineStage{ + { + Id: "plugin-1-rollback", + Name: "plugin-1-rollback", + Rollback: true, + }, + }, }, }, - rollbackStages: []*model.PipelineStage{ - { - Id: "plugin-2-rollback", - Rollback: true, + { + Name: "plugin-2", + Cli: &fakePlugin{ + quickStages: []*model.PipelineStage{ + { + Id: "plugin-2-stage-1", + Name: "plugin-2-stage-1", + }, + }, + rollbackStages: []*model.PipelineStage{ + { + Id: "plugin-2-rollback", + Name: "plugin-2-rollback", + Rollback: true, + }, + }, }, }, - }, - }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), }, + Plugins: []string{"plugin-1", "plugin-2"}, }, wantErr: false, expectedStages: []*model.PipelineStage{ { - Id: "plugin-1-stage-1", + Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", }, { - Id: "plugin-2-stage-1", + Id: "plugin-2-stage-1", + Name: "plugin-2-stage-1", }, { Id: "plugin-1-rollback", + Name: "plugin-1-rollback", Rollback: true, }, { Id: "plugin-2-rollback", + Name: "plugin-2-rollback", Rollback: true, }, }, }, { name: "multi plugins - no rollback", - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - quickStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - }, - }, - rollbackStages: []*model.PipelineStage{ - { - Id: "plugin-1-rollback", - Rollback: true, - }, - }, - }, - &fakePlugin{ - quickStages: []*model.PipelineStage{ - { - Id: "plugin-2-stage-1", + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + quickStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", + }, + }, + rollbackStages: []*model.PipelineStage{ + { + Id: "plugin-1-rollback", + Name: "plugin-1-rollback", + Rollback: true, + }, + }, }, }, - rollbackStages: []*model.PipelineStage{ - { - Id: "plugin-2-rollback", - Rollback: true, + { + Name: "plugin-2", + Cli: &fakePlugin{ + quickStages: []*model.PipelineStage{ + { + Id: "plugin-2-stage-1", + Name: "plugin-2-stage-1", + }, + }, + rollbackStages: []*model.PipelineStage{ + { + Id: "plugin-2-rollback", + Name: "plugin-2-rollback", + Rollback: true, + }, + }, }, }, - }, - }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(false), }, + Plugins: []string{"plugin-1", "plugin-2"}, }, wantErr: false, expectedStages: []*model.PipelineStage{ { - Id: "plugin-1-stage-1", + Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", }, { - Id: "plugin-2-stage-1", + Id: "plugin-2-stage-1", + Name: "plugin-2-stage-1", }, }, }, @@ -249,7 +300,7 @@ func TestBuildQuickSyncStages(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { planner := &planner{ - plugins: tc.plugins, + pluginRegistry: tc.pluginRegistry, } stages, err := planner.buildQuickSyncStages(context.TODO(), tc.cfg) require.Equal(t, tc.wantErr, err != nil) @@ -617,20 +668,30 @@ func TestPlanner_BuildPlan(t *testing.T) { { name: "quick sync strategy triggered by web console", isFirstDeploy: false, - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - quickStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - Visible: true, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + quickStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", + Visible: true, + }, + }, }, }, - }, - }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), }, + Plugins: []string{"plugin-1"}, }, deployment: &model.Deployment{ Trigger: &model.DeploymentTrigger{ @@ -645,6 +706,7 @@ func TestPlanner_BuildPlan(t *testing.T) { Stages: []*model.PipelineStage{ { Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", Visible: true, }, }, @@ -721,20 +783,30 @@ func TestPlanner_BuildPlan(t *testing.T) { { name: "quick sync due to no pipeline configured", isFirstDeploy: false, - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - quickStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - Visible: true, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + quickStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", + Visible: true, + }, + }, }, }, - }, - }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), }, + Plugins: []string{"plugin-1"}, }, deployment: &model.Deployment{ Trigger: &model.DeploymentTrigger{}, @@ -746,6 +818,7 @@ func TestPlanner_BuildPlan(t *testing.T) { Stages: []*model.PipelineStage{ { Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", Visible: true, }, }, @@ -820,16 +893,25 @@ func TestPlanner_BuildPlan(t *testing.T) { { name: "quick sync due to first deployment", isFirstDeploy: true, - plugins: []pluginapi.PluginClient{ - &fakePlugin{ - quickStages: []*model.PipelineStage{ - { - Id: "plugin-1-stage-1", - Visible: true, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "plugin-1", + Cli: &fakePlugin{ + quickStages: []*model.PipelineStage{ + { + Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", + Visible: true, + }, + }, }, }, - }, - }, + }) + require.NoError(t, err) + + return pr + }(), cfg: &config.GenericApplicationSpec{ Planner: config.DeploymentPlanner{ AutoRollback: pointerBool(true), @@ -853,6 +935,7 @@ func TestPlanner_BuildPlan(t *testing.T) { Stages: []*model.PipelineStage{ { Id: "plugin-1-stage-1", + Name: "plugin-1-stage-1", Visible: true, }, }, @@ -893,6 +976,10 @@ func TestPlanner_BuildPlan(t *testing.T) { { Name: "plugin-1", Cli: &fakePlugin{ + syncStrategy: &deployment.DetermineStrategyResponse{ + SyncStrategy: model.SyncStrategy_PIPELINE, + Summary: "determined by plugin", + }, pipelineStages: []*model.PipelineStage{ { Id: "plugin-1-stage-1", @@ -955,7 +1042,6 @@ func TestPlanner_BuildPlan(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { planner := &planner{ - plugins: tc.plugins, pluginRegistry: tc.pluginRegistry, deployment: tc.deployment, lastSuccessfulCommitHash: "", From c00aa307a4dc96dd34ee82fffa84ef530f56c1e1 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 19:05:20 +0900 Subject: [PATCH 05/20] Remove planner.stageBasedPluginsMap Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/controller/controller.go | 1 - pkg/app/pipedv1/controller/planner.go | 2 -- 2 files changed, 3 deletions(-) diff --git a/pkg/app/pipedv1/controller/controller.go b/pkg/app/pipedv1/controller/controller.go index d7ea1dd21d..91056bbb6a 100644 --- a/pkg/app/pipedv1/controller/controller.go +++ b/pkg/app/pipedv1/controller/controller.go @@ -455,7 +455,6 @@ func (c *controller) startNewPlanner(ctx context.Context, d *model.Deployment) ( configFilename, workingDir, c.pluginRegistry, - c.stageBasedPluginsMap, c.apiClient, c.gitClient, c.notifier, diff --git a/pkg/app/pipedv1/controller/planner.go b/pkg/app/pipedv1/controller/planner.go index 63f7dac391..e256e6f550 100644 --- a/pkg/app/pipedv1/controller/planner.go +++ b/pkg/app/pipedv1/controller/planner.go @@ -98,7 +98,6 @@ func newPlanner( lastSuccessfulConfigFilename string, workingDir string, pluginRegistry registry.PluginRegistry, - stageBasedPluginsMap map[string]pluginapi.PluginClient, apiClient apiClient, gitClient gitClient, notifier notifier, @@ -120,7 +119,6 @@ func newPlanner( lastSuccessfulCommitHash: lastSuccessfulCommitHash, lastSuccessfulConfigFilename: lastSuccessfulConfigFilename, workingDir: workingDir, - stageBasedPluginsMap: stageBasedPluginsMap, pluginRegistry: pluginRegistry, apiClient: apiClient, gitClient: gitClient, From 5589e8e9c181eeceedb44df7e33e523c772da2e7 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 20:54:29 +0900 Subject: [PATCH 06/20] Remove scheduler.stageBasedPluginsMap Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/controller/controller.go | 1 - pkg/app/pipedv1/controller/planner_test.go | 12 ++++ pkg/app/pipedv1/controller/scheduler.go | 12 ++-- pkg/app/pipedv1/controller/scheduler_test.go | 74 +++++++++++++++++--- 4 files changed, 82 insertions(+), 17 deletions(-) diff --git a/pkg/app/pipedv1/controller/controller.go b/pkg/app/pipedv1/controller/controller.go index 91056bbb6a..57dca367d2 100644 --- a/pkg/app/pipedv1/controller/controller.go +++ b/pkg/app/pipedv1/controller/controller.go @@ -595,7 +595,6 @@ func (c *controller) startNewScheduler(ctx context.Context, d *model.Deployment) workingDir, c.apiClient, c.gitClient, - c.stageBasedPluginsMap, c.notifier, c.secretDecrypter, c.logger, diff --git a/pkg/app/pipedv1/controller/planner_test.go b/pkg/app/pipedv1/controller/planner_test.go index fa01387bd0..497b340cb9 100644 --- a/pkg/app/pipedv1/controller/planner_test.go +++ b/pkg/app/pipedv1/controller/planner_test.go @@ -38,6 +38,7 @@ type fakePlugin struct { quickStages []*model.PipelineStage pipelineStages []*model.PipelineStage rollbackStages []*model.PipelineStage + stageStatusMap map[string]model.StageStatus } func (p *fakePlugin) Close() error { return nil } @@ -98,7 +99,18 @@ func (p *fakePlugin) FetchDefinedStages(ctx context.Context, req *deployment.Fet Stages: stages, }, nil } +func (p *fakePlugin) ExecuteStage(ctx context.Context, req *deployment.ExecuteStageRequest, opts ...grpc.CallOption) (*deployment.ExecuteStageResponse, error) { + status, ok := p.stageStatusMap[req.Input.Stage.Id] + if !ok { + return &deployment.ExecuteStageResponse{ + Status: model.StageStatus_STAGE_NOT_STARTED_YET, + }, nil + } + return &deployment.ExecuteStageResponse{ + Status: status, + }, nil +} func pointerBool(b bool) *bool { return &b } diff --git a/pkg/app/pipedv1/controller/scheduler.go b/pkg/app/pipedv1/controller/scheduler.go index 5b3bf4e636..851eeb71ad 100644 --- a/pkg/app/pipedv1/controller/scheduler.go +++ b/pkg/app/pipedv1/controller/scheduler.go @@ -34,8 +34,8 @@ import ( "github.com/pipe-cd/pipecd/pkg/app/server/service/pipedservice" config "github.com/pipe-cd/pipecd/pkg/configv1" "github.com/pipe-cd/pipecd/pkg/model" - pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment" + "github.com/pipe-cd/pipecd/pkg/plugin/registry" ) // scheduler is a dedicated object for a specific deployment of a single application. @@ -43,7 +43,7 @@ type scheduler struct { deployment *model.Deployment workingDir string - stageBasedPluginsMap map[string]pluginapi.PluginClient + pluginRegistry registry.PluginRegistry apiClient apiClient gitClient gitClient @@ -79,7 +79,6 @@ func newScheduler( workingDir string, apiClient apiClient, gitClient gitClient, - stageBasedPluginsMap map[string]pluginapi.PluginClient, notifier notifier, secretsDecrypter secretDecrypter, logger *zap.Logger, @@ -96,7 +95,6 @@ func newScheduler( s := &scheduler{ deployment: d, workingDir: workingDir, - stageBasedPluginsMap: stageBasedPluginsMap, apiClient: apiClient, gitClient: gitClient, metadataStore: metadatastore.NewMetadataStore(apiClient, d), @@ -513,9 +511,9 @@ func (s *scheduler) executeStage(sig StopSignal, ps *model.PipelineStage) (final } // Find the executor plugin for this stage. - plugin, ok := s.stageBasedPluginsMap[ps.Name] - if !ok { - s.logger.Error("failed to find the plugin for the stage", zap.String("stage-name", ps.Name)) + plugin, err := s.pluginRegistry.GetPluginClientByStageName(ps.Name) + if err != nil { + s.logger.Error("failed to find the plugin for the stage", zap.String("stage-name", ps.Name), zap.Error(err)) s.reportStageStatus(ctx, ps.Id, model.StageStatus_STAGE_FAILURE, ps.Requires) return model.StageStatus_STAGE_FAILURE } diff --git a/pkg/app/pipedv1/controller/scheduler_test.go b/pkg/app/pipedv1/controller/scheduler_test.go index b73bd92f7e..518b28863f 100644 --- a/pkg/app/pipedv1/controller/scheduler_test.go +++ b/pkg/app/pipedv1/controller/scheduler_test.go @@ -21,6 +21,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/zap/zaptest" "google.golang.org/grpc" @@ -30,6 +31,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/model" pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment" + "github.com/pipe-cd/pipecd/pkg/plugin/registry" ) func TestDetermineStageStatus(t *testing.T) { @@ -215,9 +217,27 @@ func TestExecuteStage(t *testing.T) { apiClient: &fakeAPIClient{}, targetDSP: &fakeDeploySourceProvider{}, runningDSP: &fakeDeploySourceProvider{}, - stageBasedPluginsMap: map[string]pluginapi.PluginClient{ - "stage-name": &fakeExecutorPluginClient{}, - }, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "stage-name", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "stage-id", + Name: "stage-name", + }, + }, + stageStatusMap: map[string]model.StageStatus{ + "stage-id": model.StageStatus_STAGE_SUCCESS, + }, + }, + }, + }) + require.NoError(t, err) + + return pr + }(), genericApplicationConfig: &config.GenericApplicationSpec{ Pipeline: &config.DeploymentPipeline{ Stages: []config.PipelineStage{ @@ -257,9 +277,27 @@ func TestExecuteStage_SignalTerminated(t *testing.T) { apiClient: &fakeAPIClient{}, targetDSP: &fakeDeploySourceProvider{}, runningDSP: &fakeDeploySourceProvider{}, - stageBasedPluginsMap: map[string]pluginapi.PluginClient{ - "stage-name": &fakeExecutorPluginClient{}, - }, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "stage-name", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "stage-id", + Name: "stage-name", + }, + }, + stageStatusMap: map[string]model.StageStatus{ + "stage-id": model.StageStatus_STAGE_SUCCESS, + }, + }, + }, + }) + require.NoError(t, err) + + return pr + }(), genericApplicationConfig: &config.GenericApplicationSpec{ Pipeline: &config.DeploymentPipeline{ Stages: []config.PipelineStage{ @@ -295,9 +333,27 @@ func TestExecuteStage_SignalCancelled(t *testing.T) { apiClient: &fakeAPIClient{}, targetDSP: &fakeDeploySourceProvider{}, runningDSP: &fakeDeploySourceProvider{}, - stageBasedPluginsMap: map[string]pluginapi.PluginClient{ - "stage-name": &fakeExecutorPluginClient{}, - }, + pluginRegistry: func() registry.PluginRegistry { + pr, err := registry.NewPluginRegistry(context.TODO(), []registry.Plugin{ + { + Name: "stage-name", + Cli: &fakePlugin{ + pipelineStages: []*model.PipelineStage{ + { + Id: "stage-id", + Name: "stage-name", + }, + }, + stageStatusMap: map[string]model.StageStatus{ + "stage-id": model.StageStatus_STAGE_SUCCESS, + }, + }, + }, + }) + require.NoError(t, err) + + return pr + }(), genericApplicationConfig: &config.GenericApplicationSpec{ Pipeline: &config.DeploymentPipeline{ Stages: []config.PipelineStage{ From a8edb8be18c28cfb367361acc6f613ada3bd9848 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 20:56:25 +0900 Subject: [PATCH 07/20] Remove controller.stageBasedPluginsMap Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/controller/controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/app/pipedv1/controller/controller.go b/pkg/app/pipedv1/controller/controller.go index 57dca367d2..4d4daae4dc 100644 --- a/pkg/app/pipedv1/controller/controller.go +++ b/pkg/app/pipedv1/controller/controller.go @@ -200,7 +200,6 @@ func (c *controller) Run(ctx context.Context) error { stagesBasedPluginsMap[stage] = plugin } } - c.stageBasedPluginsMap = stagesBasedPluginsMap ticker := time.NewTicker(c.syncInternal) defer ticker.Stop() From d6626dee8d4a44202f719f67baab026fad5b271a Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 21:20:27 +0900 Subject: [PATCH 08/20] Remove controller.pluginClients Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/cmd/piped/piped.go | 3 --- pkg/app/pipedv1/controller/controller.go | 22 +--------------------- pkg/app/pipedv1/controller/scheduler.go | 2 ++ 3 files changed, 3 insertions(+), 24 deletions(-) diff --git a/pkg/app/pipedv1/cmd/piped/piped.go b/pkg/app/pipedv1/cmd/piped/piped.go index be98e1b637..aa4f2e87e9 100644 --- a/pkg/app/pipedv1/cmd/piped/piped.go +++ b/pkg/app/pipedv1/cmd/piped/piped.go @@ -349,8 +349,6 @@ func (p *piped) run(ctx context.Context, input cli.Input) (runErr error) { } // Make grpc clients to connect to plugins. - pluginClis := make([]pluginapi.PluginClient, 0, len(cfg.Plugins)) - plugins := make([]pluginRegistry.Plugin, 0, len(cfg.Plugins)) options := []rpcclient.DialOption{ rpcclient.WithBlock(), @@ -396,7 +394,6 @@ func (p *piped) run(ctx context.Context, input cli.Input) (runErr error) { c := controller.NewController( apiClient, gitClient, - pluginClis, pluginRegistry, deploymentLister, commandLister, diff --git a/pkg/app/pipedv1/controller/controller.go b/pkg/app/pipedv1/controller/controller.go index 4d4daae4dc..bfcaa836b3 100644 --- a/pkg/app/pipedv1/controller/controller.go +++ b/pkg/app/pipedv1/controller/controller.go @@ -38,7 +38,6 @@ import ( "github.com/pipe-cd/pipecd/pkg/git" "github.com/pipe-cd/pipecd/pkg/model" pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" - "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment" "github.com/pipe-cd/pipecd/pkg/plugin/registry" ) @@ -97,8 +96,6 @@ type controller struct { notifier notifier secretDecrypter secretDecrypter - // gRPC clients to communicate with plugins. - pluginClients []pluginapi.PluginClient // The registry of all plugins. pluginRegistry registry.PluginRegistry @@ -135,7 +132,6 @@ type controller struct { func NewController( apiClient apiClient, gitClient gitClient, - pluginClients []pluginapi.PluginClient, pluginRegistry registry.PluginRegistry, deploymentLister deploymentLister, commandLister commandLister, @@ -149,7 +145,6 @@ func NewController( return &controller{ apiClient: apiClient, gitClient: gitClient, - pluginClients: pluginClients, pluginRegistry: pluginRegistry, deploymentLister: deploymentLister, commandLister: commandLister, @@ -185,22 +180,6 @@ func (c *controller) Run(ctx context.Context) error { c.workspaceDir = dir c.logger.Info(fmt.Sprintf("workspace directory was configured to %s", c.workspaceDir)) - // Build the list of stages that can be handled by piped's plugins. - stagesBasedPluginsMap := make(map[string]pluginapi.PluginClient) - for _, plugin := range c.pluginClients { - resp, err := plugin.FetchDefinedStages(ctx, &deployment.FetchDefinedStagesRequest{}) - if err != nil { - return err - } - for _, stage := range resp.GetStages() { - if _, ok := stagesBasedPluginsMap[stage]; ok { - c.logger.Error("duplicated stage name", zap.String("stage", stage)) - return fmt.Errorf("duplicated stage name %s", stage) - } - stagesBasedPluginsMap[stage] = plugin - } - } - ticker := time.NewTicker(c.syncInternal) defer ticker.Stop() c.logger.Info("start syncing planners and schedulers") @@ -594,6 +573,7 @@ func (c *controller) startNewScheduler(ctx context.Context, d *model.Deployment) workingDir, c.apiClient, c.gitClient, + c.pluginRegistry, c.notifier, c.secretDecrypter, c.logger, diff --git a/pkg/app/pipedv1/controller/scheduler.go b/pkg/app/pipedv1/controller/scheduler.go index 851eeb71ad..58146848f0 100644 --- a/pkg/app/pipedv1/controller/scheduler.go +++ b/pkg/app/pipedv1/controller/scheduler.go @@ -79,6 +79,7 @@ func newScheduler( workingDir string, apiClient apiClient, gitClient gitClient, + pluginRegistry registry.PluginRegistry, notifier notifier, secretsDecrypter secretDecrypter, logger *zap.Logger, @@ -97,6 +98,7 @@ func newScheduler( workingDir: workingDir, apiClient: apiClient, gitClient: gitClient, + pluginRegistry: pluginRegistry, metadataStore: metadatastore.NewMetadataStore(apiClient, d), notifier: notifier, secretDecrypter: secretsDecrypter, From 3c97335a835c31218825f3280f11212b8999261b Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:18:40 +0900 Subject: [PATCH 09/20] Refactor: remove planner.stageBasedPluginsMap Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/controller/planner.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/app/pipedv1/controller/planner.go b/pkg/app/pipedv1/controller/planner.go index e256e6f550..5453e5b9aa 100644 --- a/pkg/app/pipedv1/controller/planner.go +++ b/pkg/app/pipedv1/controller/planner.go @@ -58,10 +58,6 @@ type planner struct { lastSuccessfulConfigFilename string workingDir string - // The map used to know which plugin is incharged for a given stage - // of the current deployment. - stageBasedPluginsMap map[string]pluginapi.PluginClient - // The apiClient is used to report the deployment status. apiClient apiClient From 39abef8c41f3ecb59cade852b025d36ae2138d45 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:21:01 +0900 Subject: [PATCH 10/20] Refactor: add log Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/cmd/piped/piped.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/app/pipedv1/cmd/piped/piped.go b/pkg/app/pipedv1/cmd/piped/piped.go index aa4f2e87e9..b9b1d66470 100644 --- a/pkg/app/pipedv1/cmd/piped/piped.go +++ b/pkg/app/pipedv1/cmd/piped/piped.go @@ -357,6 +357,7 @@ func (p *piped) run(ctx context.Context, input cli.Input) (runErr error) { for _, plg := range cfg.Plugins { cli, err := pluginapi.NewClient(ctx, net.JoinHostPort("localhost", strconv.Itoa(plg.Port)), options...) if err != nil { + input.Logger.Error("failed to create client to connect plugin", zap.String("plugin", plg.Name), zap.Error(err)) return err } From f4427cebe855e421149e5de46b88f2c565917a42 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:24:29 +0900 Subject: [PATCH 11/20] Refactor: add comment for register.Plugin and register.pluginRegistry Signed-off-by: Yoshiki Fujikane --- pkg/plugin/registry/registry.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/plugin/registry/registry.go b/pkg/plugin/registry/registry.go index 92c27f72ea..44de1096f5 100644 --- a/pkg/plugin/registry/registry.go +++ b/pkg/plugin/registry/registry.go @@ -9,6 +9,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment" ) +// Plugin represents a plugin with its name and client. type Plugin struct { Name string Cli pluginapi.PluginClient @@ -21,8 +22,8 @@ type PluginRegistry interface { } type pluginRegistry struct { - nameBasedPlugins map[string]pluginapi.PluginClient - stageBasedPlugins map[string]pluginapi.PluginClient + nameBasedPlugins map[string]pluginapi.PluginClient // key: plugin name + stageBasedPlugins map[string]pluginapi.PluginClient // key: stage name // TODO: add more fields if needed (e.g. deploymentBasedPlugins, livestateBasedPlugins) } From e9291b83de32a0eae0e21a6c8d43d8620e879484 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:28:40 +0900 Subject: [PATCH 12/20] Refactor: rename GetPluginsByAppConfig to GetPluginClientsByAppConfig Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/controller/planner.go | 4 ++-- pkg/plugin/registry/registry.go | 6 +++--- pkg/plugin/registry/registry_test.go | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/app/pipedv1/controller/planner.go b/pkg/app/pipedv1/controller/planner.go index 5453e5b9aa..b406b68332 100644 --- a/pkg/app/pipedv1/controller/planner.go +++ b/pkg/app/pipedv1/controller/planner.go @@ -268,7 +268,7 @@ func (p *planner) buildPlan(ctx context.Context, runningDS, targetDS *deployment } spec := cfg.Spec - plugins, err := p.pluginRegistry.GetPluginsByAppConfig(spec) + plugins, err := p.pluginRegistry.GetPluginClientsByAppConfig(spec) // Build deployment target versions. for _, plg := range plugins { @@ -419,7 +419,7 @@ func (p *planner) buildQuickSyncStages(ctx context.Context, cfg *config.GenericA rollback = *cfg.Planner.AutoRollback ) - plugins, err := p.pluginRegistry.GetPluginsByAppConfig(cfg) + plugins, err := p.pluginRegistry.GetPluginClientsByAppConfig(cfg) if err != nil { return nil, err } diff --git a/pkg/plugin/registry/registry.go b/pkg/plugin/registry/registry.go index 44de1096f5..649d5f9a66 100644 --- a/pkg/plugin/registry/registry.go +++ b/pkg/plugin/registry/registry.go @@ -18,7 +18,7 @@ type Plugin struct { // PluginRegistry is the interface that provides methods to get plugin clients. type PluginRegistry interface { GetPluginClientByStageName(name string) (pluginapi.PluginClient, error) - GetPluginsByAppConfig(cfg *config.GenericApplicationSpec) ([]pluginapi.PluginClient, error) + GetPluginClientsByAppConfig(cfg *config.GenericApplicationSpec) ([]pluginapi.PluginClient, error) } type pluginRegistry struct { @@ -64,12 +64,12 @@ func (pr *pluginRegistry) GetPluginClientByStageName(name string) (pluginapi.Plu return plugin, nil } -// GetPluginsByAppConfig returns the plugin clients based on the given configuration. +// GetPluginClientsByAppConfig returns the plugin clients based on the given configuration. // The priority of determining plugins is as follows: // 1. If the pipeline is specified, it will determine the plugins based on the pipeline stages. // 2. If the plugins are specified, it will determine the plugins based on the plugin names. // 3. If neither the pipeline nor the plugins are specified, it will return an error. -func (pr *pluginRegistry) GetPluginsByAppConfig(cfg *config.GenericApplicationSpec) ([]pluginapi.PluginClient, error) { +func (pr *pluginRegistry) GetPluginClientsByAppConfig(cfg *config.GenericApplicationSpec) ([]pluginapi.PluginClient, error) { if cfg.Pipeline != nil && len(cfg.Pipeline.Stages) > 0 { return pr.getPluginsByPipeline(cfg.Pipeline) } diff --git a/pkg/plugin/registry/registry_test.go b/pkg/plugin/registry/registry_test.go index db51088472..62558f78d6 100644 --- a/pkg/plugin/registry/registry_test.go +++ b/pkg/plugin/registry/registry_test.go @@ -18,7 +18,7 @@ func createMockPluginClient(name string) pluginapi.PluginClient { name: name, } } -func TestPluginDeterminer_GetPluginsByAppConfig(t *testing.T) { +func TestPluginDeterminer_GetPluginClientsByAppConfig(t *testing.T) { tests := []struct { name string cfg *config.GenericApplicationSpec @@ -115,7 +115,7 @@ func TestPluginDeterminer_GetPluginsByAppConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { pr := tt.setup() - plugins, err := pr.GetPluginsByAppConfig(tt.cfg) + plugins, err := pr.GetPluginClientsByAppConfig(tt.cfg) assert.Equal(t, tt.expected, plugins) assert.Equal(t, tt.wantErr, err != nil) }) From 04607bc9eb83c53126c310e8643962493af149f8 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:30:41 +0900 Subject: [PATCH 13/20] Refactor: rename to getPluginClientsByPipeline Signed-off-by: Yoshiki Fujikane --- pkg/plugin/registry/registry.go | 4 ++-- pkg/plugin/registry/registry_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/plugin/registry/registry.go b/pkg/plugin/registry/registry.go index 649d5f9a66..cb3fea5ef7 100644 --- a/pkg/plugin/registry/registry.go +++ b/pkg/plugin/registry/registry.go @@ -71,7 +71,7 @@ func (pr *pluginRegistry) GetPluginClientByStageName(name string) (pluginapi.Plu // 3. If neither the pipeline nor the plugins are specified, it will return an error. func (pr *pluginRegistry) GetPluginClientsByAppConfig(cfg *config.GenericApplicationSpec) ([]pluginapi.PluginClient, error) { if cfg.Pipeline != nil && len(cfg.Pipeline.Stages) > 0 { - return pr.getPluginsByPipeline(cfg.Pipeline) + return pr.getPluginClientsByPipeline(cfg.Pipeline) } if cfg.Plugins != nil { @@ -81,7 +81,7 @@ func (pr *pluginRegistry) GetPluginClientsByAppConfig(cfg *config.GenericApplica return nil, fmt.Errorf("no plugin specified") } -func (pr *pluginRegistry) getPluginsByPipeline(pipeline *config.DeploymentPipeline) ([]pluginapi.PluginClient, error) { +func (pr *pluginRegistry) getPluginClientsByPipeline(pipeline *config.DeploymentPipeline) ([]pluginapi.PluginClient, error) { plugins := make([]pluginapi.PluginClient, 0, len(pipeline.Stages)) if len(pipeline.Stages) == 0 { diff --git a/pkg/plugin/registry/registry_test.go b/pkg/plugin/registry/registry_test.go index 62558f78d6..b7287ce34f 100644 --- a/pkg/plugin/registry/registry_test.go +++ b/pkg/plugin/registry/registry_test.go @@ -121,7 +121,7 @@ func TestPluginDeterminer_GetPluginClientsByAppConfig(t *testing.T) { }) } } -func TestPluginRegistry_getPluginsByPipeline(t *testing.T) { +func TestPluginRegistry_getPluginClientsByPipeline(t *testing.T) { tests := []struct { name string pipeline *config.DeploymentPipeline @@ -185,7 +185,7 @@ func TestPluginRegistry_getPluginsByPipeline(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { pr := tt.setup() - plugins, err := pr.getPluginsByPipeline(tt.pipeline) + plugins, err := pr.getPluginClientsByPipeline(tt.pipeline) assert.Equal(t, tt.expected, plugins) assert.Equal(t, tt.wantErr, err != nil) }) From 6450c0e918096b2493b6c2af9b2d591c649a3116 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:32:28 +0900 Subject: [PATCH 14/20] Refactor: rename to getPluginClientsByNames Signed-off-by: Yoshiki Fujikane --- pkg/plugin/registry/registry.go | 4 ++-- pkg/plugin/registry/registry_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/plugin/registry/registry.go b/pkg/plugin/registry/registry.go index cb3fea5ef7..4443f9958d 100644 --- a/pkg/plugin/registry/registry.go +++ b/pkg/plugin/registry/registry.go @@ -75,7 +75,7 @@ func (pr *pluginRegistry) GetPluginClientsByAppConfig(cfg *config.GenericApplica } if cfg.Plugins != nil { - return pr.getPluginsByPluginNames(cfg.Plugins) + return pr.getPluginClientsByNames(cfg.Plugins) } return nil, fmt.Errorf("no plugin specified") @@ -102,7 +102,7 @@ func (pr *pluginRegistry) getPluginClientsByPipeline(pipeline *config.Deployment return plugins, nil } -func (pr *pluginRegistry) getPluginsByPluginNames(names []string) ([]pluginapi.PluginClient, error) { +func (pr *pluginRegistry) getPluginClientsByNames(names []string) ([]pluginapi.PluginClient, error) { if len(names) == 0 { return nil, fmt.Errorf("no plugin specified") } diff --git a/pkg/plugin/registry/registry_test.go b/pkg/plugin/registry/registry_test.go index b7287ce34f..66d4aa5bbd 100644 --- a/pkg/plugin/registry/registry_test.go +++ b/pkg/plugin/registry/registry_test.go @@ -191,7 +191,7 @@ func TestPluginRegistry_getPluginClientsByPipeline(t *testing.T) { }) } } -func TestPluginRegistry_getPluginsByPluginNames(t *testing.T) { +func TestPluginRegistry_getPluginClientsByNames(t *testing.T) { tests := []struct { name string pluginNames []string @@ -247,7 +247,7 @@ func TestPluginRegistry_getPluginsByPluginNames(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { pr := tt.setup() - plugins, err := pr.getPluginsByPluginNames(tt.pluginNames) + plugins, err := pr.getPluginClientsByNames(tt.pluginNames) assert.Equal(t, tt.expected, plugins) assert.Equal(t, tt.wantErr, err != nil) }) From 3fa48f87b7f70c7e7810fcec9fb185177e1c5207 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:36:22 +0900 Subject: [PATCH 15/20] Refactor: change error message Signed-off-by: Yoshiki Fujikane --- pkg/plugin/registry/registry.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/plugin/registry/registry.go b/pkg/plugin/registry/registry.go index 4443f9958d..ff62d6d752 100644 --- a/pkg/plugin/registry/registry.go +++ b/pkg/plugin/registry/registry.go @@ -82,12 +82,11 @@ func (pr *pluginRegistry) GetPluginClientsByAppConfig(cfg *config.GenericApplica } func (pr *pluginRegistry) getPluginClientsByPipeline(pipeline *config.DeploymentPipeline) ([]pluginapi.PluginClient, error) { - plugins := make([]pluginapi.PluginClient, 0, len(pipeline.Stages)) - if len(pipeline.Stages) == 0 { - return nil, fmt.Errorf("no plugin found for the specified stages") + return nil, fmt.Errorf("no stages are set in the pipeline") } + plugins := make([]pluginapi.PluginClient, 0, len(pipeline.Stages)) for _, stage := range pipeline.Stages { plugin, ok := pr.stageBasedPlugins[stage.Name.String()] if ok { @@ -96,7 +95,7 @@ func (pr *pluginRegistry) getPluginClientsByPipeline(pipeline *config.Deployment } if len(plugins) == 0 { - return nil, fmt.Errorf("no plugin found for the specified stages") + return nil, fmt.Errorf("no plugin found for each stages") } return plugins, nil @@ -104,7 +103,7 @@ func (pr *pluginRegistry) getPluginClientsByPipeline(pipeline *config.Deployment func (pr *pluginRegistry) getPluginClientsByNames(names []string) ([]pluginapi.PluginClient, error) { if len(names) == 0 { - return nil, fmt.Errorf("no plugin specified") + return nil, fmt.Errorf("no plugin names are set") } plugins := make([]pluginapi.PluginClient, 0, len(names)) @@ -116,7 +115,7 @@ func (pr *pluginRegistry) getPluginClientsByNames(names []string) ([]pluginapi.P } if len(plugins) == 0 { - return nil, fmt.Errorf("no plugin found for the specified stages") + return nil, fmt.Errorf("no plugin found for the given plugin names") } return plugins, nil From 66030f0fdef57038405c539f05400c059da631ff Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:45:24 +0900 Subject: [PATCH 16/20] Refactor: remove createMockPluginClient Signed-off-by: Yoshiki Fujikane --- pkg/plugin/registry/registry_test.go | 65 +++++++++++++--------------- 1 file changed, 30 insertions(+), 35 deletions(-) diff --git a/pkg/plugin/registry/registry_test.go b/pkg/plugin/registry/registry_test.go index 66d4aa5bbd..12716e3a3a 100644 --- a/pkg/plugin/registry/registry_test.go +++ b/pkg/plugin/registry/registry_test.go @@ -8,16 +8,11 @@ import ( "github.com/stretchr/testify/assert" ) -type mockPluginClient struct { +type fakePluginClient struct { pluginapi.PluginClient name string } -func createMockPluginClient(name string) pluginapi.PluginClient { - return &mockPluginClient{ - name: name, - } -} func TestPluginDeterminer_GetPluginClientsByAppConfig(t *testing.T) { tests := []struct { name string @@ -40,14 +35,14 @@ func TestPluginDeterminer_GetPluginClientsByAppConfig(t *testing.T) { setup: func() *pluginRegistry { return &pluginRegistry{ stageBasedPlugins: map[string]pluginapi.PluginClient{ - "stage1": createMockPluginClient("stage1"), - "stage2": createMockPluginClient("stage2"), + "stage1": fakePluginClient{name: "stage1"}, + "stage2": fakePluginClient{name: "stage2"}, }, } }, expected: []pluginapi.PluginClient{ - createMockPluginClient("stage1"), - createMockPluginClient("stage2"), + fakePluginClient{name: "stage1"}, + fakePluginClient{name: "stage2"}, }, wantErr: false, }, @@ -60,14 +55,14 @@ func TestPluginDeterminer_GetPluginClientsByAppConfig(t *testing.T) { setup: func() *pluginRegistry { return &pluginRegistry{ nameBasedPlugins: map[string]pluginapi.PluginClient{ - "plugin1": createMockPluginClient("plugin1"), - "plugin2": createMockPluginClient("plugin2"), + "plugin1": fakePluginClient{name: "plugin1"}, + "plugin2": fakePluginClient{name: "plugin2"}, }, } }, expected: []pluginapi.PluginClient{ - createMockPluginClient("plugin1"), - createMockPluginClient("plugin2"), + fakePluginClient{name: "plugin1"}, + fakePluginClient{name: "plugin2"}, }, wantErr: false, }, @@ -85,18 +80,18 @@ func TestPluginDeterminer_GetPluginClientsByAppConfig(t *testing.T) { setup: func() *pluginRegistry { return &pluginRegistry{ stageBasedPlugins: map[string]pluginapi.PluginClient{ - "stage1": createMockPluginClient("stage1"), - "stage2": createMockPluginClient("stage2"), + "stage1": fakePluginClient{name: "stage1"}, + "stage2": fakePluginClient{name: "stage2"}, }, nameBasedPlugins: map[string]pluginapi.PluginClient{ - "plugin1": createMockPluginClient("plugin1"), - "plugin2": createMockPluginClient("plugin2"), + "plugin1": fakePluginClient{name: "plugin1"}, + "plugin2": fakePluginClient{name: "plugin2"}, }, } }, expected: []pluginapi.PluginClient{ - createMockPluginClient("stage1"), - createMockPluginClient("stage2"), + fakePluginClient{name: "stage1"}, + fakePluginClient{name: "stage2"}, }, wantErr: false, }, @@ -140,14 +135,14 @@ func TestPluginRegistry_getPluginClientsByPipeline(t *testing.T) { setup: func() *pluginRegistry { return &pluginRegistry{ stageBasedPlugins: map[string]pluginapi.PluginClient{ - "stage1": createMockPluginClient("stage1"), - "stage2": createMockPluginClient("stage2"), + "stage1": fakePluginClient{name: "stage1"}, + "stage2": fakePluginClient{name: "stage2"}, }, } }, expected: []pluginapi.PluginClient{ - createMockPluginClient("stage1"), - createMockPluginClient("stage2"), + fakePluginClient{name: "stage1"}, + fakePluginClient{name: "stage2"}, }, wantErr: false, }, @@ -205,14 +200,14 @@ func TestPluginRegistry_getPluginClientsByNames(t *testing.T) { setup: func() *pluginRegistry { return &pluginRegistry{ nameBasedPlugins: map[string]pluginapi.PluginClient{ - "plugin1": createMockPluginClient("plugin1"), - "plugin2": createMockPluginClient("plugin2"), + "plugin1": fakePluginClient{name: "plugin1"}, + "plugin2": fakePluginClient{name: "plugin2"}, }, } }, expected: []pluginapi.PluginClient{ - createMockPluginClient("plugin1"), - createMockPluginClient("plugin2"), + fakePluginClient{name: "plugin1"}, + fakePluginClient{name: "plugin2"}, }, wantErr: false, }, @@ -222,8 +217,8 @@ func TestPluginRegistry_getPluginClientsByNames(t *testing.T) { setup: func() *pluginRegistry { return &pluginRegistry{ nameBasedPlugins: map[string]pluginapi.PluginClient{ - "plugin1": createMockPluginClient("plugin1"), - "plugin2": createMockPluginClient("plugin2"), + "plugin1": fakePluginClient{name: "plugin1"}, + "plugin2": fakePluginClient{name: "plugin2"}, }, } }, @@ -235,8 +230,8 @@ func TestPluginRegistry_getPluginClientsByNames(t *testing.T) { setup: func() *pluginRegistry { return &pluginRegistry{ nameBasedPlugins: map[string]pluginapi.PluginClient{ - "plugin3": createMockPluginClient("plugin3"), - "plugin4": createMockPluginClient("plugin4"), + "plugin3": fakePluginClient{name: "plugin3"}, + "plugin4": fakePluginClient{name: "plugin4"}, }, } }, @@ -267,11 +262,11 @@ func TestPluginRegistry_GetPluginClientByStageName(t *testing.T) { setup: func() *pluginRegistry { return &pluginRegistry{ stageBasedPlugins: map[string]pluginapi.PluginClient{ - "stage1": createMockPluginClient("stage1"), + "stage1": fakePluginClient{name: "stage1"}, }, } }, - expected: createMockPluginClient("stage1"), + expected: fakePluginClient{name: "stage1"}, wantErr: false, }, { @@ -280,7 +275,7 @@ func TestPluginRegistry_GetPluginClientByStageName(t *testing.T) { setup: func() *pluginRegistry { return &pluginRegistry{ stageBasedPlugins: map[string]pluginapi.PluginClient{ - "stage1": createMockPluginClient("stage1"), + "stage1": fakePluginClient{name: "stage1"}, }, } }, From 9f428776db60ae94f8a2e0c4e875b68027cfb480 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:54:26 +0900 Subject: [PATCH 17/20] Refactor: add header Signed-off-by: Yoshiki Fujikane --- pkg/plugin/registry/registry.go | 19 +++++++++++++++++++ pkg/plugin/registry/registry_test.go | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/pkg/plugin/registry/registry.go b/pkg/plugin/registry/registry.go index ff62d6d752..ef44396e1f 100644 --- a/pkg/plugin/registry/registry.go +++ b/pkg/plugin/registry/registry.go @@ -1,3 +1,22 @@ +// Copyright 2024 The PipeCD Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package planner provides a piped component +// that decides the deployment pipeline of a deployment. +// The planner bases on the changes from git commits +// then builds the deployment manifests to know the behavior of the deployment. +// From that behavior the planner can decides which pipeline should be applied. package registry import ( diff --git a/pkg/plugin/registry/registry_test.go b/pkg/plugin/registry/registry_test.go index 12716e3a3a..57e4e281fa 100644 --- a/pkg/plugin/registry/registry_test.go +++ b/pkg/plugin/registry/registry_test.go @@ -1,3 +1,22 @@ +// Copyright 2024 The PipeCD Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package planner provides a piped component +// that decides the deployment pipeline of a deployment. +// The planner bases on the changes from git commits +// then builds the deployment manifests to know the behavior of the deployment. +// From that behavior the planner can decides which pipeline should be applied. package registry import ( From 7b756203b5ffe4b386c591b14924ee8eba9c6d8b Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 22:55:02 +0900 Subject: [PATCH 18/20] Refactor: add error handling Signed-off-by: Yoshiki Fujikane --- pkg/app/pipedv1/controller/planner.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/app/pipedv1/controller/planner.go b/pkg/app/pipedv1/controller/planner.go index b406b68332..c77ed8739a 100644 --- a/pkg/app/pipedv1/controller/planner.go +++ b/pkg/app/pipedv1/controller/planner.go @@ -269,6 +269,10 @@ func (p *planner) buildPlan(ctx context.Context, runningDS, targetDS *deployment spec := cfg.Spec plugins, err := p.pluginRegistry.GetPluginClientsByAppConfig(spec) + if err != nil { + p.logger.Error("unable to get plugin clients by app config", zap.Error(err)) + return nil, err + } // Build deployment target versions. for _, plg := range plugins { From 3b9679f225620553b3107aed84b4bb139ef44243 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 23:05:35 +0900 Subject: [PATCH 19/20] Refactor: fix to TestPluginRegistry_ Signed-off-by: Yoshiki Fujikane --- pkg/plugin/registry/registry_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/plugin/registry/registry_test.go b/pkg/plugin/registry/registry_test.go index 57e4e281fa..ab0edb2bc8 100644 --- a/pkg/plugin/registry/registry_test.go +++ b/pkg/plugin/registry/registry_test.go @@ -32,7 +32,7 @@ type fakePluginClient struct { name string } -func TestPluginDeterminer_GetPluginClientsByAppConfig(t *testing.T) { +func TestPluginRegistry_GetPluginClientsByAppConfig(t *testing.T) { tests := []struct { name string cfg *config.GenericApplicationSpec From 0681f931de00288a0d9081c9648bd6a643de5be6 Mon Sep 17 00:00:00 2001 From: Yoshiki Fujikane Date: Mon, 6 Jan 2025 23:06:44 +0900 Subject: [PATCH 20/20] Refactor: fix for lint Signed-off-by: Yoshiki Fujikane --- pkg/plugin/registry/registry_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/plugin/registry/registry_test.go b/pkg/plugin/registry/registry_test.go index ab0edb2bc8..98afd5271d 100644 --- a/pkg/plugin/registry/registry_test.go +++ b/pkg/plugin/registry/registry_test.go @@ -22,9 +22,10 @@ package registry import ( "testing" + "github.com/stretchr/testify/assert" + config "github.com/pipe-cd/pipecd/pkg/configv1" pluginapi "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1" - "github.com/stretchr/testify/assert" ) type fakePluginClient struct {