Implement QuickSyncPlan for k8s plugin #5020

Warashi · 2024-07-03T00:41:03Z

What this PR does / why we need it:

This PR implements QuickSyncPlan RPC for the Kubernetes plugin.

Which issue(s) this PR fixes:

Part of #4980

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

Signed-off-by: Shinnosuke Sawada <[email protected]>

Warashi · 2024-07-03T00:48:23Z

pkg/app/pipedv1/plugin/sourcecloner/cloner.go

+// 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
+}


I want to write tests for this method with git.faker.
However, the PR will become larger if I make git.faker public.
So, I decide not to write the test in this PR.
I'll make git.faker public later, and then write the test.

codecov · 2024-07-03T00:56:59Z

Codecov Report

Attention: Patch coverage is 0% with 71 lines in your changes missing coverage. Please review.

Project coverage is 22.41%. Comparing base (fe8f0d3) to head (0b562e4).
Report is 8 commits behind head on master.

Files	Patch %	Lines
...pedv1/plugin/platform/kubernetes/planner/server.go	0.00%	32 Missing ⚠️
pkg/app/pipedv1/plugin/inputs.go	0.00%	12 Missing ⚠️
pkg/app/piped/deploysource/sourcecloner.go	0.00%	8 Missing ⚠️
pkg/app/pipedv1/deploysource/sourcecloner.go	0.00%	8 Missing ⚠️
pkg/app/pipedv1/plugin/secrets/decrypter.go	0.00%	6 Missing ⚠️
...g/app/pipedv1/plugin/platform/kubernetes/server.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5020      +/-   ##
==========================================
- Coverage   22.45%   22.41%   -0.04%     
==========================================
  Files         520      522       +2     
  Lines       56852    56915      +63     
==========================================
- Hits        12766    12760       -6     
- Misses      43060    43129      +69     
  Partials     1026     1026

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Shinnosuke Sawada <[email protected]>

khanhtc1202 · 2024-07-08T08:27:46Z

pkg/app/pipedv1/plugin/sourcecloner/cloner.go

+// Cloner is a source cloner for git repositories.
+type Cloner struct {
+	gitPath      string
+	remotePath   string
+	revision     string
+	revisionName string
+}


@Warashi I have an idea with this sourcecloner package 🤔
Basically, our plan is to use the piped/deploysource as the portal for all git resource operations (indeed, the name is deploysource and it emits the internal using git stub).
I think this newly created package sourcecloner should follow that approach and more. What package dose is to fetch the already cloned source (prepared by deploysource), then I think we should emits the internal using git as same as we do in deploysource.

Instead of a Clone interface, which could lead to confusion (if the reviewer doesn't know we don't clone or any, just fetch the cloned local source prepared by deploysource), how about to reuse the SourceCloner interface. aka this: https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/deploysource/sourcecloner.go#L24-L28
I can see we have some implementation of this interface looks pretty fits our use case here:
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/deploysource/sourcecloner.go#L69-L94

Oops, I missed the localSourceCloner. Thank you!
I think we can use this, but I wonder why this Clone implementation only invokes the Copy method of the internal git repo.
If the internal git repo's current HEAD doesn't match localSourceCloner's revision, this Clone method will give the wrong results.
Or have I misunderstood the Revision() of SourceCloner?

@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 planpreview/builder (ref: this code: https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/planpreview/builder.go#L228), which basically only request for the HEAD of the current branch - due to the feature plan preview logic.
So, my point is to fix this localSourceCloner and use it for whatever "NOT git action pull directly but pull/fetch from a local source". WDYT? 🤔

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.

I agree with you.
I'll fix the localSourceCloner and use it.

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi · 2024-07-10T00:52:44Z

pkg/app/pipedv1/plugin/inputs.go

+	"github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/platform"
+)
+
+func GetPlanSourceCloner(input *platform.PlanPluginInput) (deploysource.SourceCloner, error) {


🤔 I wonder where we should place utilities like this function.
I place it here because all plugins may need this utility, but should I create another package under the plugin package?

I got your point, let's find out later. In this PR, place it here is LGTM 👍

khanhtc1202 · 2024-07-11T08:41:47Z

pkg/git/client.go

@@ -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.


we don't use this anymore 👀

Thank you. I removed it on this commit
355ce05

pkg/app/pipedv1/plugin/inputs.go

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

khanhtc1202

LGTM with some nits, thank you 🚀
(we can fix nits with other PRs)

khanhtc1202 · 2024-07-11T09:25:55Z

pkg/app/pipedv1/plugin/platform/kubernetes/planner/server.go

+	d, err := os.MkdirTemp("", "") // TODO
+	if err != nil {
+		return nil, fmt.Errorf("failed to prepare temporary directory (%w)", err)
+	}
+	defer os.RemoveAll(d)


@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

// Ensure the existence of the working directory for the deployment.

workingDir, err := os.MkdirTemp(c.workspaceDir, d.Id+"-planner-*")

if err != nil {

logger.Error("failed to create working directory for planner", zap.Error(err))

return nil, err

}

pipecd/pkg/plugin/api/v1alpha1/platform/api.proto

Line 63 in 6400c89

message PlanPluginInput {

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.

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.

Ok, let address it by another PR 👍

khanhtc1202 · 2024-07-11T11:36:12Z

pkg/app/pipedv1/plugin/platform/kubernetes/planner/server.go

+	if err != nil {
+		return nil, err
+	}
+


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 👀

I fixed it on this commit.
d845f7a

khanhtc1202 · 2024-07-12T05:01:57Z

pkg/app/pipedv1/plugin/platform/kubernetes/planner/server.go

+	autoRollback := false
+	if a := ds.ApplicationConfig.KubernetesApplicationSpec.Input.AutoRollback; a != nil {
+		autoRollback = *a
+	}


nits, we have a default value set as true, so no need to initialize autoRollback var as false here, since the ds.ApplicationConfig.KubernetesApplicationSpec.Input.AutoRollback is true as init
ref: ds.ApplicationConfig.KubernetesApplicationSpec.Input.AutoRollback

Thank you.
I noticed that I can view the document here.
https://pkg.go.dev/github.com/pipe-cd/pipecd/pkg/config#KubernetesDeploymentInput

I fixed it on this commit.
d845f7a

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

ffjlabo

🚀

khanhtc1202

👍👍

* Implement QuickSyncPlan for k8s plugin Signed-off-by: Shinnosuke Sawada <[email protected]> * Fix build and add TODO comment Signed-off-by: Shinnosuke Sawada <[email protected]> * Use deploysource.NewLocalSourceCloner to clone source code Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Fix localSourceCloner.Clone to checkout target revision Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Format sources Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Remove unused function Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Add licence Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Fix kubernetes application spec nil checks Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> --------- Signed-off-by: Shinnosuke Sawada <[email protected]> Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> Signed-off-by: khanhtc1202 <[email protected]>

Implement QuickSyncPlan for k8s plugin

4b4ee33

Signed-off-by: Shinnosuke Sawada <[email protected]>

github-actions bot added the area/go label Jul 3, 2024

Warashi commented Jul 3, 2024

View reviewed changes

Fix build and add TODO comment

2d03122

Signed-off-by: Shinnosuke Sawada <[email protected]>

Warashi force-pushed the kubernetes-platfrom-provider-plugin branch from e18305f to 2d03122 Compare July 3, 2024 01:00

Warashi marked this pull request as ready for review July 3, 2024 01:04

Warashi requested review from nghialv, khanhtc1202, kentakozuka, ffjlabo and t-kikuc as code owners July 3, 2024 01:04

khanhtc1202 reviewed Jul 8, 2024

View reviewed changes

Warashi added 3 commits July 10, 2024 09:43

Use deploysource.NewLocalSourceCloner to clone source code

2203a1e

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix localSourceCloner.Clone to checkout target revision

6d3a18a

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Format sources

0a9a0e9

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi commented Jul 10, 2024

View reviewed changes

Warashi requested a review from khanhtc1202 July 10, 2024 00:52

khanhtc1202 reviewed Jul 11, 2024

View reviewed changes

pkg/app/pipedv1/plugin/inputs.go Show resolved Hide resolved

Warashi added 2 commits July 11, 2024 17:45

Remove unused function

355ce05

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add licence

0b562e4

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi requested a review from khanhtc1202 July 11, 2024 08:49

khanhtc1202 previously approved these changes Jul 11, 2024

View reviewed changes

khanhtc1202 reviewed Jul 11, 2024

View reviewed changes

khanhtc1202 reviewed Jul 12, 2024

View reviewed changes

Fix kubernetes application spec nil checks

d845f7a

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi dismissed khanhtc1202’s stale review via d845f7a July 12, 2024 05:08

Warashi requested a review from khanhtc1202 July 12, 2024 05:10

ffjlabo approved these changes Jul 12, 2024

View reviewed changes

Warashi enabled auto-merge (squash) July 12, 2024 05:12

khanhtc1202 approved these changes Jul 12, 2024

View reviewed changes

Warashi merged commit aff6609 into master Jul 12, 2024
14 of 16 checks passed

Warashi deleted the kubernetes-platfrom-provider-plugin branch July 12, 2024 05:12

Warashi mentioned this pull request Jul 16, 2024

Add working_dir to PlanPluginInput #5043

Closed

This was referenced Jul 18, 2024

Release v0.48.1 #5054

Merged

Release v0.48.2 #5058

Merged

This was referenced Jul 29, 2024

Release/init v0.48.3 #5090

Merged

Update RELEASE to v0.48.4 #5104

Merged

This was referenced Aug 13, 2024

Update RELEASE to v0.48.5 #5130

Merged

Update to v0.48.6 #5137

Merged

github-actions bot mentioned this pull request Aug 26, 2024

Release v0.48.7 #5157

Merged

This was referenced Sep 3, 2024

Update release to v0.48.8 #5176

Merged

Release v0.48.9 #5205

Merged

github-actions bot mentioned this pull request Sep 24, 2024

Update RELEASE to v0.49.0 #5221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement QuickSyncPlan for k8s plugin #5020

Implement QuickSyncPlan for k8s plugin #5020

Warashi commented Jul 3, 2024

Warashi Jul 3, 2024

codecov bot commented Jul 3, 2024 •

edited

Loading

khanhtc1202 Jul 8, 2024

khanhtc1202 Jul 8, 2024

Warashi Jul 8, 2024

khanhtc1202 Jul 8, 2024

khanhtc1202 Jul 8, 2024 •

edited

Loading

Warashi Jul 10, 2024

Warashi Jul 10, 2024

khanhtc1202 Jul 11, 2024

khanhtc1202 Jul 11, 2024

Warashi Jul 11, 2024

khanhtc1202 left a comment •

edited

Loading

khanhtc1202 Jul 11, 2024

khanhtc1202 Jul 11, 2024

Warashi Jul 12, 2024

khanhtc1202 Jul 12, 2024

khanhtc1202 Jul 11, 2024 •

edited

Loading

Warashi Jul 12, 2024

khanhtc1202 Jul 12, 2024

Warashi Jul 12, 2024

Warashi Jul 12, 2024

ffjlabo left a comment

khanhtc1202 left a comment

	// Ensure the existence of the working directory for the deployment.
	workingDir, err := os.MkdirTemp(c.workspaceDir, d.Id+"-planner-*")
	if err != nil {
	logger.Error("failed to create working directory for planner", zap.Error(err))
	return nil, err
	}

Implement QuickSyncPlan for k8s plugin #5020

Implement QuickSyncPlan for k8s plugin #5020

Conversation

Warashi commented Jul 3, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanhtc1202 Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanhtc1202 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanhtc1202 Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 3, 2024 •

edited

Loading

khanhtc1202 Jul 8, 2024 •

edited

Loading

khanhtc1202 left a comment •

edited

Loading

khanhtc1202 Jul 11, 2024 •

edited

Loading