Add test for executing K8S_SYNC stage at the kubernetes plugin #5431

Warashi · 2024-12-17T08:33:54Z

What this PR does:

Implement the test in the same way as #5399 at the Kubernetes plugin, then fix the failure about the resource key.

Why we need it:

To ensure compatibility between the current Kubernetes platform provider and the Kubernetes plugin.

Which issue(s) this PR fixes:

Part of #4980

Does this PR introduce a user-facing change?: No

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

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

Warashi · 2024-12-17T08:35:55Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go

+	assert.Equal(t, "simple", deployment.GetName())
+	assert.Equal(t, "simple", deployment.GetLabels()["app"])
+	assert.Equal(t, "piped", deployment.GetAnnotations()["pipecd.dev/managed-by"])
+	assert.Equal(t, "piped-id", deployment.GetAnnotations()["pipecd.dev/piped"])
+	assert.Equal(t, "app-id", deployment.GetAnnotations()["pipecd.dev/application"])
+	assert.Equal(t, "apps/v1", deployment.GetAnnotations()["pipecd.dev/original-api-version"])
+	assert.Equal(t, "apps/v1:Deployment:default:simple", deployment.GetAnnotations()["pipecd.dev/resource-key"])
+	assert.Equal(t, "0123456789", deployment.GetAnnotations()["pipecd.dev/commit-hash"])


these assertions are copied from #5399

codecov · 2024-12-17T08:39:10Z

Codecov Report

Attention: Patch coverage is 50.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 26.24%. Comparing base (7cf95df) to head (8fe9dad).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...ugin/logpersister/logpersistertest/logpersister.go	47.82%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5431      +/-   ##
==========================================
+ Coverage   26.11%   26.24%   +0.13%     
==========================================
  Files         451      452       +1     
  Lines       48823    48860      +37     
==========================================
+ Hits        12749    12825      +76     
+ Misses      35057    35011      -46     
- Partials     1017     1024       +7

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

Warashi · 2024-12-17T08:48:14Z

pkg/app/pipedv1/plugin/kubernetes/provider/resource.go

+	if k.Namespace == "" {
+		k.Namespace = DefaultNamespace
+	}


There was an incompatibility between the current Kubernetes platform provider and the Kubernetes plugin, so I fixed this.

This quite tricky and could lead to some issues because logic which depends on resource key need the ns be set carefully. Say, if the resource is cluster resource, we should not force setting ns value to it cc @ffjlabo

IMO: it is quite tricky as @khanhtc1202 said. So at first, we might be good to implement the same as pipedv0. And later fix the resource key problem.

Thank you for commenting. I reverted this change and fixed the assertion.
We can write the e2e test now, so I want to write tests to prevent bugs.

057b9f5
e030be1

khanhtc1202 · 2024-12-17T11:02:58Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go

+
+// newTestLogPersister creates a new testLogPersister for testing.
+//
+// TODO: move to a common package


it's good to move this to logpersister package itself. Just like git package provides gittest for testing

Thank you. I moved these to pkg/plugin/logpersister/logpersistertest.
3c0a57d

khanhtc1202 · 2024-12-17T11:04:05Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go

+	}
+}
+
+// TODO: move to a common package


this is okay to be here since it's kubernetes specific

Thank you. I removed the TODO comment.
8fe9dad

khanhtc1202 · 2024-12-17T11:06:30Z

pkg/app/pipedv1/plugin/kubernetes/provider/resource.go

+	if k.Namespace == "" {
+		k.Namespace = DefaultNamespace
+	}


This quite tricky and could lead to some issues because logic which depends on resource key need the ns be set carefully. Say, if the resource is cluster resource, we should not force setting ns value to it cc @ffjlabo

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev> This reverts commit 098d1da.

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

khanhtc1202 · 2024-12-19T02:50:27Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server.go

@@ -189,7 +189,7 @@ func (a *DeploymentService) loadManifests(ctx context.Context, deploy *model.Dep
 	manifests, err := a.loader.LoadManifests(ctx, provider.LoaderInput{
 		PipedID:          deploy.GetPipedId(),
 		AppID:            deploy.GetApplicationId(),
-		CommitHash:       deploy.GetTrigger().GetCommit().GetHash(),
+		CommitHash:       deploymentSource.GetRevision(),


This could be the same but I like the previous version more. How about to rename GetRevision to GetCommitHash 🤔

First, I also like commit hash more than revision.

However, this is not the same because the deploymentSource can be the running deploy source, not the target deploy source.

The naming GetRevision comes from proto definition, so how about renaming this field as commit_hash?

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

Lines 142 to 143 in 17a5007

// The git commit revision of the source code.

string revision = 2;

I got your point. Also, it's a commit hash, so it should be named directly as a commit hash to reduce confusion. Let's address it by another PR (change proto and update this)

khanhtc1202

LGTM 👍

ffjlabo

LGTM 🚀

Warashi added 3 commits December 17, 2024 17:25

Fix commit hash in loadManifests function

ccebc52

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

Fix resource key to keep compatibility

098d1da

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

Add tests for executing K8S_SYNC

6460f32

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

github-actions bot added area/go area/pipedv1 labels Dec 17, 2024

Warashi commented Dec 17, 2024

View reviewed changes

Warashi marked this pull request as ready for review December 17, 2024 08:38

Warashi requested review from nghialv, khanhtc1202, kentakozuka, ffjlabo and t-kikuc as code owners December 17, 2024 08:38

Warashi commented Dec 17, 2024

View reviewed changes

khanhtc1202 reviewed Dec 17, 2024

View reviewed changes

Warashi added 5 commits December 18, 2024 13:56

Move log persister for testing to separate package

3c0a57d

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

Merge branch 'master' into k8s-plugin-sync-test

63c8d77

Revert "Fix resource key to keep compatibility"

057b9f5

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev> This reverts commit 098d1da.

Change the assertion and add comment

e030be1

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

Remove TODO comment

8fe9dad

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

Warashi requested a review from khanhtc1202 December 18, 2024 08:52

khanhtc1202 reviewed Dec 19, 2024

View reviewed changes

khanhtc1202 approved these changes Dec 19, 2024

View reviewed changes

ffjlabo approved these changes Dec 20, 2024

View reviewed changes

Warashi merged commit b98a963 into master Dec 20, 2024
18 checks passed

Warashi deleted the k8s-plugin-sync-test branch December 20, 2024 04:27

github-actions bot mentioned this pull request Jan 21, 2025

Release v0.50.1 #5498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for executing K8S_SYNC stage at the kubernetes plugin #5431

Add test for executing K8S_SYNC stage at the kubernetes plugin #5431

Warashi commented Dec 17, 2024

Warashi Dec 17, 2024

codecov bot commented Dec 17, 2024 •

edited

Loading

Warashi Dec 17, 2024

khanhtc1202 Dec 17, 2024

ffjlabo Dec 18, 2024

Warashi Dec 18, 2024

khanhtc1202 Dec 17, 2024

Warashi Dec 18, 2024

khanhtc1202 Dec 17, 2024

Warashi Dec 18, 2024 •

edited

Loading

khanhtc1202 Dec 17, 2024

khanhtc1202 Dec 19, 2024

Warashi Dec 19, 2024

khanhtc1202 Dec 19, 2024

khanhtc1202 left a comment

ffjlabo left a comment

	// The git commit revision of the source code.
	string revision = 2;

Add test for executing K8S_SYNC stage at the kubernetes plugin #5431

Add test for executing K8S_SYNC stage at the kubernetes plugin #5431

Conversation

Warashi commented Dec 17, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Warashi Dec 18, 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

khanhtc1202 left a comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2024 •

edited

Loading

Warashi Dec 18, 2024 •

edited

Loading