Skip to content

Commit 9ce9a81

Browse files
ffjlaboffjlabo
committed
Replace old determineStrategy
Signed-off-by: Yoshiki Fujikane <[email protected]>
1 parent 83bf848 commit 9ce9a81

File tree

4 files changed

+39
-78
lines changed

4 files changed

+39
-78
lines changed

‎pkg/app/pipedv1/plugin/kubernetes/deployment/determine.go

+14-30
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323

2424
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/config"
2525
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/provider"
26-
"github.com/pipe-cd/pipecd/pkg/model"
2726
"github.com/pipe-cd/pipecd/pkg/plugin/diff"
2827
"github.com/pipe-cd/pipecd/pkg/plugin/sdk"
2928
)
@@ -157,16 +156,17 @@ func checkReplicasChange(ns diff.Nodes) (before, after string, changed bool) {
157156
return node.StringX(), node.StringY(), true
158157
}
159158

159+
// determineStrategy decides the sync strategy and summary message based on the given manifests.
160160
// First up, checks to see if the workload's `spec.template` has been changed,
161161
// and then checks if the configmap/secret's data.
162-
func determineStrategy(olds, news []provider.Manifest, workloadRefs []config.K8sResourceReference, logger *zap.Logger) (strategy model.SyncStrategy, summary string) {
162+
func determineStrategy(olds, news []provider.Manifest, workloadRefs []config.K8sResourceReference, logger *zap.Logger) (strategy sdk.SyncStrategy, summary string) {
163163
oldWorkloads := findWorkloadManifests(olds, workloadRefs)
164164
if len(oldWorkloads) == 0 {
165-
return model.SyncStrategy_QUICK_SYNC, "Quick sync by applying all manifests because it was unable to find the currently running workloads"
165+
return sdk.SyncStrategyQuickSync, "Quick sync by applying all manifests because it was unable to find the currently running workloads"
166166
}
167167
newWorkloads := findWorkloadManifests(news, workloadRefs)
168168
if len(newWorkloads) == 0 {
169-
return model.SyncStrategy_QUICK_SYNC, "Quick sync by applying all manifests because it was unable to find workloads in the new manifests"
169+
return sdk.SyncStrategyQuickSync, "Quick sync by applying all manifests because it was unable to find workloads in the new manifests"
170170
}
171171

172172
workloads := provider.FindSameManifests(oldWorkloads, newWorkloads)
@@ -177,17 +177,17 @@ func determineStrategy(olds, news []provider.Manifest, workloadRefs []config.K8s
177177
// do progressive deployment with the specified pipeline.
178178
diffResult, err := provider.Diff(w.Old, w.New, logger)
179179
if err != nil {
180-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
180+
return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
181181
}
182182
diffNodes := diffResult.Nodes()
183183
diffs[w.New.Key()] = diffNodes
184184

185185
templateDiffs := diffNodes.FindByPrefix("spec.template")
186186
if len(templateDiffs) > 0 {
187187
if msg, changed := checkImageChange(templateDiffs); changed {
188-
return model.SyncStrategy_PIPELINE, msg
188+
return sdk.SyncStrategyPipelineSync, msg
189189
}
190-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because pod template of workload %s was changed", w.New.Name())
190+
return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because pod template of workload %s was changed", w.New.Name())
191191
}
192192
}
193193

@@ -196,22 +196,22 @@ func determineStrategy(olds, news []provider.Manifest, workloadRefs []config.K8s
196196
oldConfigs := provider.FindConfigsAndSecrets(olds)
197197
newConfigs := provider.FindConfigsAndSecrets(news)
198198
if len(oldConfigs) > len(newConfigs) {
199-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %d configmap/secret deleted", len(oldConfigs)-len(newConfigs))
199+
return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because %d configmap/secret deleted", len(oldConfigs)-len(newConfigs))
200200
}
201201
if len(oldConfigs) < len(newConfigs) {
202-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because new %d configmap/secret added", len(newConfigs)-len(oldConfigs))
202+
return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because new %d configmap/secret added", len(newConfigs)-len(oldConfigs))
203203
}
204204
for k, oc := range oldConfigs {
205205
nc, ok := newConfigs[k]
206206
if !ok {
207-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %s %s was deleted", oc.Kind(), oc.Name())
207+
return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because %s %s was deleted", oc.Kind(), oc.Name())
208208
}
209209
result, err := provider.Diff(oc, nc, logger)
210210
if err != nil {
211-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
211+
return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
212212
}
213213
if result.HasDiff() {
214-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %s %s was updated", oc.Kind(), oc.Name())
214+
return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because %s %s was updated", oc.Kind(), oc.Name())
215215
}
216216
}
217217

@@ -225,24 +225,8 @@ func determineStrategy(olds, news []provider.Manifest, workloadRefs []config.K8s
225225
}
226226
if len(scales) > 0 {
227227
slices.Sort(scales)
228-
return model.SyncStrategy_QUICK_SYNC, fmt.Sprintf("Quick sync to scale %s", strings.Join(scales, ", "))
228+
return sdk.SyncStrategyQuickSync, fmt.Sprintf("Quick sync to scale %s", strings.Join(scales, ", "))
229229
}
230230

231-
return model.SyncStrategy_QUICK_SYNC, "Quick sync by applying all manifests"
232-
}
233-
234-
// determineStrategySDK decides the sync strategy and summary message based on the given manifests.
235-
// TODO: rewrite this function to determineStrategy after the current determineStrategy is removed.
236-
func determineStrategySDK(olds, news []provider.Manifest, workloadRefs []config.K8sResourceReference, logger *zap.Logger) (strategy sdk.SyncStrategy, summary string) {
237-
mStrategy, summary := determineStrategy(olds, news, workloadRefs, logger)
238-
239-
var s sdk.SyncStrategy
240-
switch mStrategy {
241-
case model.SyncStrategy_QUICK_SYNC:
242-
s = sdk.SyncStrategyQuickSync
243-
case model.SyncStrategy_PIPELINE:
244-
s = sdk.SyncStrategyQuickSync
245-
}
246-
247-
return s, summary
231+
return sdk.SyncStrategyQuickSync, "Quick sync by applying all manifests"
248232
}

‎pkg/app/pipedv1/plugin/kubernetes/deployment/determine_test.go

+23-24
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626

2727
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/config"
2828
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/provider"
29-
"github.com/pipe-cd/pipecd/pkg/model"
3029
"github.com/pipe-cd/pipecd/pkg/plugin/sdk"
3130
)
3231

@@ -914,13 +913,13 @@ spec:
914913
}
915914
}
916915

917-
func TestDetermineStrategy(t *testing.T) {
916+
func Test_determineStrategy(t *testing.T) {
918917
tests := []struct {
919918
name string
920919
olds []string
921920
news []string
922921
workloadRefs []config.K8sResourceReference
923-
wantStrategy model.SyncStrategy
922+
wantStrategy sdk.SyncStrategy
924923
wantSummary string
925924
}{
926925
{
@@ -940,7 +939,7 @@ spec:
940939
image: nginx:1.19.3
941940
`,
942941
},
943-
wantStrategy: model.SyncStrategy_QUICK_SYNC,
942+
wantStrategy: sdk.SyncStrategyQuickSync,
944943
wantSummary: "Quick sync by applying all manifests because it was unable to find the currently running workloads",
945944
},
946945
{
@@ -960,7 +959,7 @@ spec:
960959
`,
961960
},
962961
news: []string{},
963-
wantStrategy: model.SyncStrategy_QUICK_SYNC,
962+
wantStrategy: sdk.SyncStrategyQuickSync,
964963
wantSummary: "Quick sync by applying all manifests because it was unable to find workloads in the new manifests",
965964
},
966965
{
@@ -993,7 +992,7 @@ spec:
993992
image: nginx:1.19.4
994993
`,
995994
},
996-
wantStrategy: model.SyncStrategy_PIPELINE,
995+
wantStrategy: sdk.SyncStrategyPipelineSync,
997996
wantSummary: "Sync progressively because of updating image nginx from 1.19.3 to 1.19.4",
998997
},
999998
{
@@ -1042,7 +1041,7 @@ data:
10421041
key: new-value
10431042
`,
10441043
},
1045-
wantStrategy: model.SyncStrategy_PIPELINE,
1044+
wantStrategy: sdk.SyncStrategyPipelineSync,
10461045
wantSummary: "Sync progressively because ConfigMap my-config was updated",
10471046
},
10481047
{
@@ -1077,7 +1076,7 @@ spec:
10771076
image: nginx:1.19.3
10781077
`,
10791078
},
1080-
wantStrategy: model.SyncStrategy_QUICK_SYNC,
1079+
wantStrategy: sdk.SyncStrategyQuickSync,
10811080
wantSummary: "Quick sync to scale Deployment/nginx-deployment from 3 to 5",
10821081
},
10831082
{
@@ -1111,7 +1110,7 @@ spec:
11111110
image: nginx:1.19.3
11121111
`,
11131112
},
1114-
wantStrategy: model.SyncStrategy_QUICK_SYNC,
1113+
wantStrategy: sdk.SyncStrategyQuickSync,
11151114
wantSummary: "Quick sync to scale Deployment/nginx-deployment from <nil> to 1",
11161115
},
11171116
{
@@ -1145,7 +1144,7 @@ spec:
11451144
image: nginx:1.19.3
11461145
`,
11471146
},
1148-
wantStrategy: model.SyncStrategy_QUICK_SYNC,
1147+
wantStrategy: sdk.SyncStrategyQuickSync,
11491148
wantSummary: "Quick sync to scale Deployment/nginx-deployment from 1 to <nil>",
11501149
},
11511150
{
@@ -1206,7 +1205,7 @@ spec:
12061205
image: redis:6.0.9
12071206
`,
12081207
},
1209-
wantStrategy: model.SyncStrategy_QUICK_SYNC,
1208+
wantStrategy: sdk.SyncStrategyQuickSync,
12101209
wantSummary: "Quick sync to scale Deployment/nginx-deployment from 3 to 5, Deployment/redis-deployment from 2 to 4",
12111210
},
12121211
{
@@ -1243,7 +1242,7 @@ spec:
12431242
image: redis:6.0.10
12441243
`,
12451244
},
1246-
wantStrategy: model.SyncStrategy_PIPELINE,
1245+
wantStrategy: sdk.SyncStrategyPipelineSync,
12471246
wantSummary: "Sync progressively because of updating image nginx from 1.19.3 to 1.19.4, image redis from 6.0.9 to 6.0.10",
12481247
},
12491248
{
@@ -1280,7 +1279,7 @@ spec:
12801279
image: nginx:1.19.3
12811280
`,
12821281
},
1283-
wantStrategy: model.SyncStrategy_PIPELINE,
1282+
wantStrategy: sdk.SyncStrategyPipelineSync,
12841283
wantSummary: "Sync progressively because of updating image nginx:1.19.3 to redis:6.0.9, image redis:6.0.9 to nginx:1.19.3",
12851284
},
12861285
{
@@ -1343,7 +1342,7 @@ spec:
13431342
Name: "nginx-deployment",
13441343
},
13451344
},
1346-
wantStrategy: model.SyncStrategy_PIPELINE,
1345+
wantStrategy: sdk.SyncStrategyPipelineSync,
13471346
wantSummary: "Sync progressively because of updating image nginx from 1.19.3 to 1.19.4",
13481347
},
13491348
{
@@ -1376,7 +1375,7 @@ spec:
13761375
image: nginx:1.19.3
13771376
`,
13781377
},
1379-
wantStrategy: model.SyncStrategy_PIPELINE,
1378+
wantStrategy: sdk.SyncStrategyPipelineSync,
13801379
wantSummary: "Sync progressively because pod template of workload nginx-deployment was changed",
13811380
},
13821381
{
@@ -1417,7 +1416,7 @@ spec:
14171416
memory: "1Gi"
14181417
`,
14191418
},
1420-
wantStrategy: model.SyncStrategy_PIPELINE,
1419+
wantStrategy: sdk.SyncStrategyPipelineSync,
14211420
wantSummary: "Sync progressively because pod template of workload nginx-deployment was changed",
14221421
},
14231422
{
@@ -1458,7 +1457,7 @@ spec:
14581457
image: nginx:1.19.3
14591458
`,
14601459
},
1461-
wantStrategy: model.SyncStrategy_PIPELINE,
1460+
wantStrategy: sdk.SyncStrategyPipelineSync,
14621461
wantSummary: "Sync progressively because 1 configmap/secret deleted",
14631462
},
14641463
{
@@ -1499,7 +1498,7 @@ spec:
14991498
image: nginx:1.19.3
15001499
`,
15011500
},
1502-
wantStrategy: model.SyncStrategy_PIPELINE,
1501+
wantStrategy: sdk.SyncStrategyPipelineSync,
15031502
wantSummary: "Sync progressively because 1 configmap/secret deleted",
15041503
},
15051504
{
@@ -1540,7 +1539,7 @@ data:
15401539
key: value
15411540
`,
15421541
},
1543-
wantStrategy: model.SyncStrategy_PIPELINE,
1542+
wantStrategy: sdk.SyncStrategyPipelineSync,
15441543
wantSummary: "Sync progressively because new 1 configmap/secret added",
15451544
},
15461545
{
@@ -1581,7 +1580,7 @@ data:
15811580
key: dmFsdWU=
15821581
`,
15831582
},
1584-
wantStrategy: model.SyncStrategy_PIPELINE,
1583+
wantStrategy: sdk.SyncStrategyPipelineSync,
15851584
wantSummary: "Sync progressively because new 1 configmap/secret added",
15861585
},
15871586
{
@@ -1630,7 +1629,7 @@ data:
16301629
key: new-value
16311630
`,
16321631
},
1633-
wantStrategy: model.SyncStrategy_PIPELINE,
1632+
wantStrategy: sdk.SyncStrategyPipelineSync,
16341633
wantSummary: "Sync progressively because ConfigMap old-config was deleted",
16351634
},
16361635
{
@@ -1664,7 +1663,7 @@ spec:
16641663
image: nginx:1.19.3
16651664
`,
16661665
},
1667-
wantStrategy: model.SyncStrategy_QUICK_SYNC,
1666+
wantStrategy: sdk.SyncStrategyQuickSync,
16681667
wantSummary: "Quick sync by applying all manifests",
16691668
},
16701669
{
@@ -1697,7 +1696,7 @@ spec:
16971696
image: nginx:1.19.3
16981697
`,
16991698
},
1700-
wantStrategy: model.SyncStrategy_QUICK_SYNC,
1699+
wantStrategy: sdk.SyncStrategyQuickSync,
17011700
wantSummary: "Quick sync by applying all manifests",
17021701
},
17031702
}
@@ -1713,7 +1712,7 @@ spec:
17131712
}
17141713
logger := zap.NewNop()
17151714
gotStrategy, gotSummary := determineStrategy(oldManifests, newManifests, tt.workloadRefs, logger)
1716-
assert.Equal(t, tt.wantStrategy.String(), gotStrategy.String())
1715+
assert.Equal(t, tt.wantStrategy, gotStrategy)
17171716
assert.Equal(t, tt.wantSummary, gotSummary)
17181717
})
17191718
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,7 @@ func (p *Plugin) DetermineStrategy(ctx context.Context, _ *sdk.ConfigNone, input
386386
return nil, err
387387
}
388388

389-
strategy, summary := determineStrategySDK(runnings, targets, cfg.Spec.Workloads, logger)
389+
strategy, summary := determineStrategy(runnings, targets, cfg.Spec.Workloads, logger)
390390

391391
return &sdk.DetermineStrategyResponse{
392392
Strategy: strategy,

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

+1-23
Original file line numberDiff line numberDiff line change
@@ -78,29 +78,7 @@ func (a *DeploymentService) Register(server *grpc.Server) {
7878

7979
// DetermineStrategy implements deployment.DeploymentServiceServer.
8080
func (a *DeploymentService) DetermineStrategy(ctx context.Context, request *deployment.DetermineStrategyRequest) (*deployment.DetermineStrategyResponse, error) {
81-
cfg, err := config.DecodeYAML[*kubeconfig.KubernetesApplicationSpec](request.GetInput().GetTargetDeploymentSource().GetApplicationConfig())
82-
if err != nil {
83-
return nil, status.Error(codes.InvalidArgument, err.Error())
84-
}
85-
86-
runnings, err := a.loadManifests(ctx, request.GetInput().GetDeployment(), cfg.Spec, request.GetInput().GetRunningDeploymentSource())
87-
88-
if err != nil {
89-
return nil, status.Error(codes.Internal, err.Error())
90-
}
91-
92-
targets, err := a.loadManifests(ctx, request.GetInput().GetDeployment(), cfg.Spec, request.GetInput().GetTargetDeploymentSource())
93-
94-
if err != nil {
95-
return nil, status.Error(codes.Internal, err.Error())
96-
}
97-
98-
strategy, summary := determineStrategy(runnings, targets, cfg.Spec.Workloads, a.logger)
99-
100-
return &deployment.DetermineStrategyResponse{
101-
SyncStrategy: strategy,
102-
Summary: summary,
103-
}, nil
81+
return &deployment.DetermineStrategyResponse{}, nil
10482

10583
}
10684

0 commit comments

Comments
 (0)