Skip to content

Commit 566d35a

Browse files
britaniarbritaniar
committed
address comments
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
1 parent e63165c commit 566d35a

File tree

6 files changed

+100
-105
lines changed

6 files changed

+100
-105
lines changed

‎apis/placement/v1beta1/commons.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,12 @@ const (
170170
// TargetUpdatingStageNameLabel indicates the updating stage name on a staged run related object.
171171
TargetUpdatingStageNameLabel = FleetPrefix + "targetUpdatingStage"
172172

173+
// BeforeStageTaskName is the name of the before stage task label value.
174+
BeforeStageTaskName = "beforeStage"
175+
176+
// AfterStageTaskName is the name of the after stage task label value.
177+
AfterStageTaskName = "afterStage"
178+
173179
// BeforeStageApprovalTaskNameFmt is the format of the before stage approval task name.
174180
BeforeStageApprovalTaskNameFmt = "%s-before-%s"
175181

‎pkg/controllers/updaterun/execution.go‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ func (r *Reconciler) checkBeforeStageTasksStatus(ctx context.Context, updatingSt
121121
for i, task := range updatingStage.BeforeStageTasks {
122122
switch task.Type {
123123
case placementv1beta1.StageTaskTypeApproval:
124-
approved, err := r.handleStageApprovalTask(ctx, &updatingStageStatus.BeforeStageTaskStatus[i], updatingStage, updateRun)
124+
approved, err := r.handleStageApprovalTask(ctx, &updatingStageStatus.BeforeStageTaskStatus[i], updatingStage, updateRun, placementv1beta1.BeforeStageTaskName)
125125
if err != nil {
126126
return false, err
127127
}
@@ -404,7 +404,7 @@ func (r *Reconciler) checkAfterStageTasksStatus(ctx context.Context, updatingSta
404404
klog.V(2).InfoS("The after stage wait task has completed", "stage", updatingStage.Name, "updateRun", updateRunRef)
405405
}
406406
case placementv1beta1.StageTaskTypeApproval:
407-
approved, err := r.handleStageApprovalTask(ctx, &updatingStageStatus.AfterStageTaskStatus[i], updatingStage, updateRun)
407+
approved, err := r.handleStageApprovalTask(ctx, &updatingStageStatus.AfterStageTaskStatus[i], updatingStage, updateRun, placementv1beta1.AfterStageTaskName)
408408
if err != nil {
409409
return false, -1, err
410410
}
@@ -426,6 +426,7 @@ func (r *Reconciler) handleStageApprovalTask(
426426
stageTaskStatus *placementv1beta1.StageTaskStatus,
427427
updatingStage *placementv1beta1.StageConfig,
428428
updateRun placementv1beta1.UpdateRunObj,
429+
stageTaskName string,
429430
) (bool, error) {
430431
updateRunRef := klog.KObj(updateRun)
431432

@@ -436,7 +437,7 @@ func (r *Reconciler) handleStageApprovalTask(
436437
}
437438

438439
// Check if the approval request has been created.
439-
approvalRequest := buildApprovalRequestObject(types.NamespacedName{Name: stageTaskStatus.ApprovalRequestName, Namespace: updateRun.GetNamespace()}, updatingStage.Name, updateRun.GetName())
440+
approvalRequest := buildApprovalRequestObject(types.NamespacedName{Name: stageTaskStatus.ApprovalRequestName, Namespace: updateRun.GetNamespace()}, updatingStage.Name, updateRun.GetName(), stageTaskName)
440441
requestRef := klog.KObj(approvalRequest)
441442
if err := r.Client.Create(ctx, approvalRequest); err != nil {
442443
if apierrors.IsAlreadyExists(err) {
@@ -606,11 +607,7 @@ func checkClusterUpdateResult(
606607

607608
// buildApprovalRequestObject creates an approval request object for before-stage or after-stage tasks.
608609
// It returns a ClusterApprovalRequest if namespace is empty, otherwise returns an ApprovalRequest.
609-
func buildApprovalRequestObject(namespacedName types.NamespacedName, stageName, updateRunName string) placementv1beta1.ApprovalRequestObj {
610-
stageTask := "before"
611-
if strings.Contains(namespacedName.Name, "after") {
612-
stageTask = "after"
613-
}
610+
func buildApprovalRequestObject(namespacedName types.NamespacedName, stageName, updateRunName, stageTaskName string) placementv1beta1.ApprovalRequestObj {
614611
var approvalRequest placementv1beta1.ApprovalRequestObj
615612
if namespacedName.Namespace == "" {
616613
approvalRequest = &placementv1beta1.ClusterApprovalRequest{
@@ -619,7 +616,7 @@ func buildApprovalRequestObject(namespacedName types.NamespacedName, stageName,
619616
Labels: map[string]string{
620617
placementv1beta1.TargetUpdatingStageNameLabel: stageName,
621618
placementv1beta1.TargetUpdateRunLabel: updateRunName,
622-
placementv1beta1.TargetStageTaskLabel: stageTask,
619+
placementv1beta1.TargetStageTaskLabel: stageTaskName,
623620
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
624621
},
625622
},
@@ -636,7 +633,7 @@ func buildApprovalRequestObject(namespacedName types.NamespacedName, stageName,
636633
Labels: map[string]string{
637634
placementv1beta1.TargetUpdatingStageNameLabel: stageName,
638635
placementv1beta1.TargetUpdateRunLabel: updateRunName,
639-
placementv1beta1.TargetStageTaskLabel: stageTask,
636+
placementv1beta1.TargetStageTaskLabel: stageTaskName,
640637
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
641638
},
642639
},

‎pkg/controllers/updaterun/execution_integration_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
172172
Labels: map[string]string{
173173
placementv1beta1.TargetUpdatingStageNameLabel: updateRun.Status.StagesStatus[0].StageName,
174174
placementv1beta1.TargetUpdateRunLabel: updateRun.Name,
175-
placementv1beta1.TargetStageTaskLabel: "before",
175+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.BeforeStageTaskName,
176176
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
177177
},
178178
},
@@ -324,7 +324,7 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
324324
Labels: map[string]string{
325325
placementv1beta1.TargetUpdatingStageNameLabel: updateRun.Status.StagesStatus[0].StageName,
326326
placementv1beta1.TargetUpdateRunLabel: updateRun.Name,
327-
placementv1beta1.TargetStageTaskLabel: "after",
327+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.AfterStageTaskName,
328328
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
329329
},
330330
},
@@ -380,7 +380,7 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
380380
Labels: map[string]string{
381381
placementv1beta1.TargetUpdatingStageNameLabel: updateRun.Status.StagesStatus[1].StageName,
382382
placementv1beta1.TargetUpdateRunLabel: updateRun.Name,
383-
placementv1beta1.TargetStageTaskLabel: "before",
383+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.BeforeStageTaskName,
384384
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
385385
},
386386
},
@@ -533,7 +533,7 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
533533
Labels: map[string]string{
534534
placementv1beta1.TargetUpdatingStageNameLabel: updateRun.Status.StagesStatus[1].StageName,
535535
placementv1beta1.TargetUpdateRunLabel: updateRun.Name,
536-
placementv1beta1.TargetStageTaskLabel: "after",
536+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.AfterStageTaskName,
537537
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
538538
},
539539
},
@@ -1088,7 +1088,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
10881088
Labels: map[string]string{
10891089
placementv1beta1.TargetUpdatingStageNameLabel: updateRun.Status.StagesStatus[0].StageName,
10901090
placementv1beta1.TargetUpdateRunLabel: updateRun.Name,
1091-
placementv1beta1.TargetStageTaskLabel: "after",
1091+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.AfterStageTaskName,
10921092
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
10931093
},
10941094
},
@@ -1310,7 +1310,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
13101310
Labels: map[string]string{
13111311
placementv1beta1.TargetUpdatingStageNameLabel: updateRun.Status.StagesStatus[0].StageName,
13121312
placementv1beta1.TargetUpdateRunLabel: updateRun.Name,
1313-
placementv1beta1.TargetStageTaskLabel: "before",
1313+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.BeforeStageTaskName,
13141314
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
13151315
},
13161316
},
@@ -1344,7 +1344,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
13441344
Labels: map[string]string{
13451345
placementv1beta1.TargetUpdatingStageNameLabel: updateRun.Status.StagesStatus[0].StageName,
13461346
placementv1beta1.TargetUpdateRunLabel: updateRun.Name,
1347-
placementv1beta1.TargetStageTaskLabel: "before",
1347+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.BeforeStageTaskName,
13481348
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
13491349
},
13501350
},
@@ -1447,7 +1447,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
14471447
Labels: map[string]string{
14481448
placementv1beta1.TargetUpdatingStageNameLabel: updateRun.Status.StagesStatus[0].StageName,
14491449
placementv1beta1.TargetUpdateRunLabel: updateRun.Name,
1450-
placementv1beta1.TargetStageTaskLabel: "after",
1450+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.AfterStageTaskName,
14511451
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
14521452
},
14531453
},

‎pkg/controllers/updaterun/execution_test.go‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -346,23 +346,25 @@ func TestBuildApprovalRequestObject(t *testing.T) {
346346
namespacedName types.NamespacedName
347347
stageName string
348348
updateRunName string
349+
stageTaskName string
349350
want placementv1beta1.ApprovalRequestObj
350351
}{
351352
{
352353
name: "should create ClusterApprovalRequest when namespace is empty",
353354
namespacedName: types.NamespacedName{
354-
Name: "test-before-approval-request",
355+
Name: fmt.Sprintf(placementv1beta1.BeforeStageApprovalTaskNameFmt, "test-update-run", "test-stage"),
355356
Namespace: "",
356357
},
357358
stageName: "test-stage",
358359
updateRunName: "test-update-run",
360+
stageTaskName: placementv1beta1.BeforeStageTaskName,
359361
want: &placementv1beta1.ClusterApprovalRequest{
360362
ObjectMeta: metav1.ObjectMeta{
361-
Name: "test-before-approval-request",
363+
Name: fmt.Sprintf(placementv1beta1.BeforeStageApprovalTaskNameFmt, "test-update-run", "test-stage"),
362364
Labels: map[string]string{
363365
placementv1beta1.TargetUpdatingStageNameLabel: "test-stage",
364366
placementv1beta1.TargetUpdateRunLabel: "test-update-run",
365-
placementv1beta1.TargetStageTaskLabel: "before",
367+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.BeforeStageTaskName,
366368
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
367369
},
368370
},
@@ -375,19 +377,20 @@ func TestBuildApprovalRequestObject(t *testing.T) {
375377
{
376378
name: "should create namespaced ApprovalRequest when namespace is provided",
377379
namespacedName: types.NamespacedName{
378-
Name: "test-after-approval-request",
380+
Name: fmt.Sprintf(placementv1beta1.AfterStageApprovalTaskNameFmt, "test-update-run", "test-stage"),
379381
Namespace: "test-namespace",
380382
},
381383
stageName: "test-stage",
382384
updateRunName: "test-update-run",
385+
stageTaskName: placementv1beta1.AfterStageTaskName,
383386
want: &placementv1beta1.ApprovalRequest{
384387
ObjectMeta: metav1.ObjectMeta{
385-
Name: "test-after-approval-request",
388+
Name: fmt.Sprintf(placementv1beta1.AfterStageApprovalTaskNameFmt, "test-update-run", "test-stage"),
386389
Namespace: "test-namespace",
387390
Labels: map[string]string{
388391
placementv1beta1.TargetUpdatingStageNameLabel: "test-stage",
389392
placementv1beta1.TargetUpdateRunLabel: "test-update-run",
390-
placementv1beta1.TargetStageTaskLabel: "after",
393+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.AfterStageTaskName,
391394
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
392395
},
393396
},
@@ -401,7 +404,7 @@ func TestBuildApprovalRequestObject(t *testing.T) {
401404

402405
for _, test := range tests {
403406
t.Run(test.name, func(t *testing.T) {
404-
got := buildApprovalRequestObject(test.namespacedName, test.stageName, test.updateRunName)
407+
got := buildApprovalRequestObject(test.namespacedName, test.stageName, test.updateRunName, test.stageTaskName)
405408

406409
// Compare the whole objects using cmp.Diff with ignore options
407410
if diff := cmp.Diff(test.want, got); diff != "" {
@@ -950,7 +953,6 @@ func TestCalculateMaxConcurrencyValue(t *testing.T) {
950953
func TestCheckBeforeStageTasksStatus_NegativeCases(t *testing.T) {
951954
stageName := "stage-0"
952955
testUpdateRunName = "test-update-run"
953-
stageTask := "before"
954956
approvalRequestName := fmt.Sprintf(placementv1beta1.BeforeStageApprovalTaskNameFmt, testUpdateRunName, stageName)
955957
tests := []struct {
956958
name string
@@ -1038,7 +1040,7 @@ func TestCheckBeforeStageTasksStatus_NegativeCases(t *testing.T) {
10381040
Labels: map[string]string{
10391041
placementv1beta1.TargetUpdatingStageNameLabel: stageName,
10401042
placementv1beta1.TargetUpdateRunLabel: testUpdateRunName,
1041-
placementv1beta1.TargetStageTaskLabel: stageTask,
1043+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.BeforeStageTaskName,
10421044
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
10431045
},
10441046
},
@@ -1095,7 +1097,7 @@ func TestCheckBeforeStageTasksStatus_NegativeCases(t *testing.T) {
10951097
Labels: map[string]string{
10961098
placementv1beta1.TargetUpdatingStageNameLabel: stageName,
10971099
placementv1beta1.TargetUpdateRunLabel: testUpdateRunName,
1098-
placementv1beta1.TargetStageTaskLabel: stageTask,
1100+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.BeforeStageTaskName,
10991101
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
11001102
},
11011103
},
@@ -1152,7 +1154,7 @@ func TestCheckBeforeStageTasksStatus_NegativeCases(t *testing.T) {
11521154
Labels: map[string]string{
11531155
placementv1beta1.TargetUpdatingStageNameLabel: stageName,
11541156
placementv1beta1.TargetUpdateRunLabel: testUpdateRunName,
1155-
placementv1beta1.TargetStageTaskLabel: stageTask,
1157+
placementv1beta1.TargetStageTaskLabel: placementv1beta1.BeforeStageTaskName,
11561158
placementv1beta1.IsLatestUpdateRunApprovalLabel: "true",
11571159
},
11581160
},

0 commit comments

Comments
 (0)