-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement QuickSyncPlan for k8s plugin #5020
Changes from 1 commit
4b4ee33
2d03122
2203a1e
6d3a18a
0a9a0e9
355ce05
0b562e4
d845f7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,17 +17,27 @@ package planner | |
import ( | ||
"context" | ||
"fmt" | ||
"io" | ||
"os" | ||
"time" | ||
|
||
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/deploysource" | ||
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/sourcecloner" | ||
"github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/platform" | ||
"github.com/pipe-cd/pipecd/pkg/regexpool" | ||
|
||
"go.uber.org/zap" | ||
"google.golang.org/grpc" | ||
) | ||
|
||
type secretDecrypter interface { | ||
Decrypt(string) (string, error) | ||
} | ||
|
||
type PlannerService struct { | ||
platform.UnimplementedPlannerServiceServer | ||
|
||
Decrypter secretDecrypter | ||
RegexPool *regexpool.Pool | ||
Logger *zap.Logger | ||
} | ||
|
@@ -38,8 +48,12 @@ func (a *PlannerService) Register(server *grpc.Server) { | |
} | ||
|
||
// NewPlannerService creates a new planService. | ||
func NewPlannerService(logger *zap.Logger) *PlannerService { | ||
func NewPlannerService( | ||
decrypter secretDecrypter, | ||
logger *zap.Logger, | ||
) *PlannerService { | ||
return &PlannerService{ | ||
Decrypter: decrypter, | ||
RegexPool: regexpool.DefaultPool(), | ||
Logger: logger.Named("planner"), | ||
} | ||
|
@@ -63,7 +77,43 @@ func (ps *PlannerService) DetermineStrategy(ctx context.Context, in *platform.De | |
} | ||
|
||
func (ps *PlannerService) QuickSyncPlan(ctx context.Context, in *platform.QuickSyncPlanRequest) (*platform.QuickSyncPlanResponse, error) { | ||
return nil, fmt.Errorf("not implemented yet") | ||
now := time.Now() | ||
|
||
cloner, err := sourcecloner.NewCloner( | ||
in.GetInput().GetSourceRemoteUrl(), | ||
in.GetInput().GetDeployment().GetGitPath().GetRepo().GetBranch(), | ||
"target", | ||
) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to create git client (%w)", err) | ||
} | ||
|
||
d, err := os.MkdirTemp("", "") // TODO | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to prepare temporary directory (%w)", err) | ||
} | ||
defer os.RemoveAll(d) | ||
|
||
p := deploysource.NewProvider( | ||
d, | ||
cloner, | ||
*in.GetInput().GetDeployment().GetGitPath(), | ||
ps.Decrypter, | ||
) | ||
|
||
ds, err := p.GetReadOnly(ctx, io.Discard /* TODO */) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nits cfg := ds.ApplicationConfig.KubernetesApplicationSpec
if cfg == nil {
return nil, fmt.Errorf("missing KubernetesApplicationSpec in application configuration")
} Should check spec available to avoid nil pointer 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fixed it on this commit. |
||
autoRollback := false | ||
if a := ds.ApplicationConfig.KubernetesApplicationSpec.Input.AutoRollback; a != nil { | ||
autoRollback = *a | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nits, we have a default value set as true, so no need to initialize autoRollback var as false here, since the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fixed it on this commit. |
||
|
||
return &platform.QuickSyncPlanResponse{ | ||
Stages: buildQuickSyncPipeline(autoRollback, now), | ||
}, nil | ||
} | ||
|
||
func (ps *PlannerService) PipelineSyncPlan(ctx context.Context, in *platform.PipelineSyncPlanRequest) (*platform.PipelineSyncPlanResponse, error) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
// 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 secrets | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
|
||
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/cmd/piped/service" | ||
) | ||
|
||
type Decrypter struct { | ||
client service.PluginServiceClient | ||
} | ||
|
||
func (d *Decrypter) Decrypt(src string) (string, error) { | ||
r, err := d.client.DecryptSecret(context.TODO(), &service.DecryptSecretRequest{Secret: src}) | ||
if err != nil { | ||
return "", fmt.Errorf("failed to decrypt secret: %w", err) | ||
} | ||
return r.GetDecryptedSecret(), nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
// 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 sourcecloner | ||
|
||
import ( | ||
"context" | ||
"os" | ||
"os/exec" | ||
"sync" | ||
|
||
"github.com/pipe-cd/pipecd/pkg/app/piped/deploysource" | ||
"github.com/pipe-cd/pipecd/pkg/git" | ||
) | ||
|
||
var _ deploysource.SourceCloner = (*Cloner)(nil) | ||
|
||
var gitPath = sync.OnceValues(func() (string, error) { | ||
path, err := exec.LookPath("git") | ||
if err != nil { | ||
return "", err | ||
} | ||
return path, nil | ||
}) | ||
|
||
// Cloner is a source cloner for git repositories. | ||
type Cloner struct { | ||
gitPath string | ||
remotePath string | ||
revision string | ||
revisionName string | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Warashi I have an idea with this sourcecloner package 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, I missed the localSourceCloner. Thank you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Warashi That's a good catch 👍 I think up to now, the localSourceCloner not yet raised any issues because its caller doesn't require any other revision than the HEAD 🤔 So, basically I think it's a bug potential logic and should be fixed, but as far as I investigated, the only place which we use this localSourceCloner is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My point is: we should not create too many interfaces/packages that do similar tasks (in this case: clone source from git), or it will lead to confusion for the caller side. In this case, it would be better to have only one SourceCloner interface, one pull/fetch to the outer git, one pull/fetch to the local repo that was pulled by the previous implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with you. |
||
|
||
// NewCloner creates a new Cloner instance. | ||
// if revision is empty, it will checkout defualt branch. | ||
func NewCloner(remotePath, revision, revisionName string) (*Cloner, error) { | ||
gitPath, err := gitPath() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return &Cloner{ | ||
gitPath: gitPath, | ||
remotePath: remotePath, | ||
revision: revision, | ||
revisionName: revisionName, | ||
}, nil | ||
} | ||
|
||
// Clone implements deploysource.SourceCloner. | ||
func (c *Cloner) Clone(ctx context.Context, dest string) error { | ||
if _, err := git.RunGitCommand(ctx, c.gitPath, "", os.Environ(), "clone", c.remotePath, dest); err != nil { | ||
return err | ||
} | ||
if c.revision == "" { | ||
if _, err := git.RunGitCommand(ctx, c.gitPath, "", os.Environ(), "-C", dest, "checkout", c.revision); err != nil { | ||
return err | ||
} | ||
} | ||
return nil | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to write tests for this method with git.faker. |
||
|
||
// Revision implements deploysource.SourceCloner. | ||
func (c *Cloner) Revision() string { | ||
return c.revision | ||
} | ||
|
||
// RevisionName implements deploysource.SourceCloner. | ||
func (c *Cloner) RevisionName() string { | ||
return c.revisionName | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -252,6 +252,11 @@ func (c *client) envsForRepo(remote string) []string { | |
return append(envs, c.gitEnvs...) | ||
} | ||
|
||
// RunGitCommand runs a git command with the given arguments. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't use this anymore 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. I removed it on this commit |
||
func RunGitCommand(ctx context.Context, execPath, dir string, envs []string, args ...string) ([]byte, error) { | ||
return runGitCommand(ctx, execPath, dir, envs, args...) | ||
} | ||
|
||
func runGitCommand(ctx context.Context, execPath, dir string, envs []string, args ...string) ([]byte, error) { | ||
cmd := exec.CommandContext(ctx, execPath, args...) | ||
cmd.Dir = dir | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Warashi Question:
Should we add workingDir as PlanPluginInput? 🤔 Currently, the planner in the controller uses workingDir passed at the time controller.planner object is created, which mean we have 1:1 for relationship between workingDir & planner which is used to build plan for a deployment
ref:
pipecd/pkg/app/piped/controller/controller.go
Lines 438 to 443 in 6400c89
pipecd/pkg/plugin/api/v1alpha1/platform/api.proto
Line 63 in 6400c89
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make it that way, then the piped.planner object will create the workingDir then clone/pull the deploy source, then we pass the workingDir (which contains cloned source) to the planner plugin via PlanPluginInput.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds nice.
In this PR, we clone the deploy source manually.
However, it's needed for each plugin implementation, so it's simpler if we handle it at piped and only use the prepared source in the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let address it by another PR 👍