Skip to content

Implement BuildPipelineSyncStages for k8s multi sync #5596

Merged
ffjlabo merged 9 commits intomasterfrom
implement-k8s-sync-for-multi-cluster-plugin
Feb 25, 2025
Merged

Implement BuildPipelineSyncStages for k8s multi sync #5596
ffjlabo merged 9 commits intomasterfrom
implement-k8s-sync-for-multi-cluster-plugin

Conversation

@ffjlabo
Copy link
Member

@ffjlabo ffjlabo commented Feb 21, 2025

What this PR does:

as title.

I checked the behavior with planning pipeline sync. It is ok to fail the execution because I don't implement its logic.

apiVersion: pipecd.dev/v1beta1
kind: Application
spec:
  name: multicluster-plugin
  labels:
    env: example
    team: product
  planner:
    alwaysUsePipeline: true
  quickSync:
    prune: true
  pipeline:
    stages:
        - name: K8S_SYNC
  input:
    kubectlVersion: 1.31.0
  description: |
    try multi cluster plugin
  plugins:
    - kubernetes_multicluster
PipeCD

Why we need it:

Which issue(s) this PR fixes:

Part of #5006

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):

@@ -0,0 +1,125 @@
// Copyright 202 The PipeCD Authors.
Copy link
Member Author

Choose a reason for hiding this comment

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

borrowed these code from k8s plugin

@codecov
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 81.25000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 26.26%. Comparing base (a20eaef) to head (8c98e8a).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...p/pipedv1/plugin/kubernetes_multicluster/plugin.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5596      +/-   ##
==========================================
+ Coverage   26.24%   26.26%   +0.02%     
==========================================
  Files         472      474       +2     
  Lines       50428    50475      +47     
==========================================
+ Hits        13234    13259      +25     
- Misses      36134    36153      +19     
- Partials     1060     1063       +3     

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

@ffjlabo ffjlabo marked this pull request as draft February 21, 2025 05:19
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
@ffjlabo ffjlabo force-pushed the implement-k8s-sync-for-multi-cluster-plugin branch from 7337551 to 4e93517 Compare February 25, 2025 04:20
@ffjlabo ffjlabo marked this pull request as ready for review February 25, 2025 04:26
@ffjlabo ffjlabo changed the title Implement k8s sync for multi cluster plugin Implement BuildPipelineSyncStages for k8s multi sync Feb 25, 2025
Copy link
Member

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

Almost LGTM. I commented on nits.

return a.Index - b.Index
}).Index

// we copy the predefined stage to avoid modifying the original one.
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be no predefined stage and no copy.

Suggested change
// we copy the predefined stage to avoid modifying the original one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed on 4a9ce2f

Comment on lines +24 to +28
// StageK8sSync represents the state where
// all resources should be synced with the Git state.
StageK8sMultiSync string = "K8S_MULTI_SYNC"
// StageK8sRollback represents the state where all deployed resources should be rollbacked.
StageK8sMultiRollback string = "K8S_MULTI_ROLLBACK"
Copy link
Member

Choose a reason for hiding this comment

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

It might be forgotten to change the comment.

Suggested change
// StageK8sSync represents the state where
// all resources should be synced with the Git state.
StageK8sMultiSync string = "K8S_MULTI_SYNC"
// StageK8sRollback represents the state where all deployed resources should be rollbacked.
StageK8sMultiRollback string = "K8S_MULTI_ROLLBACK"
// StageK8sMultiSync represents the state where
// all resources should be synced with the Git state.
StageK8sMultiSync string = "K8S_MULTI_SYNC"
// StageK8sMultiRollback represents the state where all deployed resources should be rollbacked.
StageK8sMultiRollback string = "K8S_MULTI_ROLLBACK"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed on 4a9ce2f

Comment on lines +115 to +116
t.Run(tt.name, func(t *testing.T) {
actual := BuildPipelineStages(tt.input)
Copy link
Member

Choose a reason for hiding this comment

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

It's nice to have t.Parallel() here.

Suggested change
t.Run(tt.name, func(t *testing.T) {
actual := BuildPipelineStages(tt.input)
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
actual := BuildPipelineStages(tt.input)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed on
8c98e8a


type deployTargetConfig struct{}

var _ sdk.StagePlugin[config, deployTargetConfig] = (*plugin)(nil)
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
@ffjlabo ffjlabo requested a review from Warashi February 25, 2025 05:09
Copy link
Member

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

LGTM

@ffjlabo ffjlabo enabled auto-merge (squash) February 25, 2025 06:25
Copy link
Member

@t-kikuc t-kikuc left a comment

Choose a reason for hiding this comment

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

LGTM, Go Go!

@ffjlabo ffjlabo merged commit c1a62c5 into master Feb 25, 2025
18 checks passed
@ffjlabo ffjlabo deleted the implement-k8s-sync-for-multi-cluster-plugin branch February 25, 2025 08:57
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.

3 participants