Skip to content
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 sync-by-replace option #3237

Merged
merged 9 commits into from Feb 10, 2022
Merged

Implement sync-by-replace option #3237

merged 9 commits into from Feb 10, 2022

Conversation

Hosshii
Copy link
Member

@Hosshii Hosshii commented Feb 10, 2022

What this PR does / why we need it:
Implement pipecd.dev/sync-by-replace: "true" option. This enable use of kubectl replace/create instead of kubectl apply.
With this change, CRD is not supported for simplicity.

Which issue(s) this PR fixes:

Fixes #3173

Does this PR introduce a user-facing change?:

Enable using `pipecd.dev/sync-by-replace: "enabled"` annotation in Kubernetes manifests to use `kubectl replace/create` instead of `kubectl apply` while applying it 

Copy link
Collaborator

@pipecd-bot pipecd-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GO_LINTER

Some issues were detected while linting go source files in your changes.

KindClusterRole = "ClusterRole"
KindClusterRoleBinding = "ClusterRoleBinding"
KindNameSpace = "NameSpace"
KindDeployment = "Deployment"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exported const KindDeployment should have comment (or a comment on this block) or be unexported

https://golang.org/wiki/CodeReviewComments#doc-comments

@Hosshii Hosshii marked this pull request as draft February 10, 2022 02:04
@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 31.91%. This pull request decreases coverage by -0.01%.

File Function Base Head Diff
pkg/app/piped/cloudprovider/kubernetes/resourcekey.go ResourceKey.IsCRD -- 0.00% +0.00%
pkg/app/piped/cloudprovider/kubernetes/manifest.go Manifest.GetAnnotations 0.00% 100.00% +100.00%
pkg/app/piped/executor/kubernetes/kubernetes.go applyManifests 90.00% 55.00% -35.00%

@pipecd-bot pipecd-bot added size/L and removed size/M labels Feb 10, 2022
@Hosshii Hosshii marked this pull request as ready for review February 10, 2022 02:48
@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 31.99%. This pull request increases coverage by 0.07%.

File Function Base Head Diff
pkg/app/piped/cloudprovider/kubernetes/resourcekey.go ResourceKey.IsCRD -- 80.00% +80.00%
pkg/app/piped/cloudprovider/kubernetes/manifest.go Manifest.GetAnnotations 0.00% 100.00% +100.00%
pkg/app/piped/executor/kubernetes/kubernetes.go applyManifests 90.00% 95.00% +5.00%

Co-authored-by: knanao <50069775+knanao@users.noreply.github.com>
@pipecd-bot
Copy link
Collaborator

GO_LINTER

The following files are not gofmt-ed. By commenting /golinter fmt, the formatted one will be appended to this pull request automatically.

pkg/app/piped/executor/kubernetes/kubernetes.go
--- pkg/app/piped/executor/kubernetes/kubernetes.go.orig
+++ pkg/app/piped/executor/kubernetes/kubernetes.go
@@ -214,26 +214,26 @@
 	for _, m := range manifests {
 		annotation := m.GetAnnotations()[provider.LabelSyncReplace]
 		// Do not replace CRD because replace might delete CRD instance
-                if annotation != provider.UserReplaceTrue || m.Key.IsCRD() {
-	                if err := applier.ApplyManifest(ctx, m); err != nil {
-		                lp.Errorf("Failed to apply manifest: %s (%w)", m.Key.ReadableLogString(), err)
-		                return err
-	                }
-	                lp.Successf("- applied manifest: %s", m.Key.ReadableLogString())
-	                continue
-                }
-                // Always try to replace first and create if it fails due to resource not found error.
-                // This is because we cannot know whether resource already exists before executing command.
-                err := applier.ReplaceManifest(ctx, m)
-                if errors.Is(err, provider.ErrNotFound) {
-	                lp.Infof("Specified resource does not exist, so create the resource: %s (%w)", m.Key.ReadableLogString(), err)
-	                err = applier.CreateManifest(ctx, m)
-                }
-                if err != nil {
-	                lp.Errorf("Failed to replace or create manifest: %s (%w)", m.Key.ReadableLogString(), err)
-	                return err
-                }
-                lp.Successf("- replaced or created manifest: %s", m.Key.ReadableLogString())
+		if annotation != provider.UserReplaceTrue || m.Key.IsCRD() {
+			if err := applier.ApplyManifest(ctx, m); err != nil {
+				lp.Errorf("Failed to apply manifest: %s (%w)", m.Key.ReadableLogString(), err)
+				return err
+			}
+			lp.Successf("- applied manifest: %s", m.Key.ReadableLogString())
+			continue
+		}
+		// Always try to replace first and create if it fails due to resource not found error.
+		// This is because we cannot know whether resource already exists before executing command.
+		err := applier.ReplaceManifest(ctx, m)
+		if errors.Is(err, provider.ErrNotFound) {
+			lp.Infof("Specified resource does not exist, so create the resource: %s (%w)", m.Key.ReadableLogString(), err)
+			err = applier.CreateManifest(ctx, m)
+		}
+		if err != nil {
+			lp.Errorf("Failed to replace or create manifest: %s (%w)", m.Key.ReadableLogString(), err)
+			return err
+		}
+		lp.Successf("- replaced or created manifest: %s", m.Key.ReadableLogString())
 
 	}
 	lp.Successf("Successfully applied %d manifests", len(manifests))

@knanao
Copy link
Member

knanao commented Feb 10, 2022

Pretty good!
/lgtm

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 32.00%. This pull request increases coverage by 0.07%.

File Function Base Head Diff
pkg/app/piped/cloudprovider/kubernetes/resourcekey.go ResourceKey.IsCRD -- 80.00% +80.00%
pkg/app/piped/cloudprovider/kubernetes/manifest.go Manifest.GetAnnotations 0.00% 100.00% +100.00%
pkg/app/piped/executor/kubernetes/kubernetes.go applyManifests 90.00% 95.24% +5.24%

@pipecd-bot pipecd-bot removed the lgtm label Feb 10, 2022
@Hosshii
Copy link
Member Author

Hosshii commented Feb 10, 2022

Change option name from true to enabled for usability and remove CRD check because ignore replace option when CRD is confusing.

@nghialv
Copy link
Member

nghialv commented Feb 10, 2022

Change option name from true to enabled for usability and remove CRD check because ignore replace option when CRD is confusing.

Got it. Look nice.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 31.98%. This pull request increases coverage by 0.06%.

File Function Base Head Diff
pkg/app/piped/cloudprovider/kubernetes/manifest.go Manifest.GetAnnotations 0.00% 100.00% +100.00%
pkg/app/piped/executor/kubernetes/kubernetes.go applyManifests 90.00% 95.24% +5.24%

@nghialv
Copy link
Member

nghialv commented Feb 10, 2022

Great work! Thank you.
/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace instead of kubectl apply to avoid annotation size limit being exceeded
4 participants