Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

Update apply.go #232

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update apply.go #232

wants to merge 1 commit into from

Conversation

xishengcai
Copy link
Contributor

如果自定义的scope 对象 第一次创建的时候, 没有加入spec字段, 会报错,此时不应该return, 而应该继续向下执行, 第一次patch 成功后, 这一段 pave 就不会报错,然后检查workload ref 是否重复。

如果自定义的scope 对象 第一次创建的时候, 没有加入spec字段, 会报错,此时不应该return, 而应该继续向下执行, 第一次patch 成功后, 这一段 pave 就不会报错,然后检查workload ref 是否重复。
Copy link
Collaborator

@ryanzhang-oss ryanzhang-oss left a comment

Choose a reason for hiding this comment

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

We would like to understand the behavior better with a full explanation in English and clean code with tests.

@@ -222,8 +222,10 @@ func (a *workloads) applyScope(ctx context.Context, wl Workload, s unstructured.
return nil
}
}
} else {
return errors.Wrapf(err, errFmtGetScopeWorkloadRef, s.GetAPIVersion(), s.GetKind(), s.GetName(), workloadRefsPath)
}else if err.Error() == "spec: no such field"{
Copy link
Collaborator

Choose a reason for hiding this comment

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

compare err with some magic string is an anti-pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't proceed if the workloadRef path is provided but doesn't work

@captainroy-hy
Copy link
Contributor

AppConfig depends on the field path from ScopeDefinition's spec.workloadRefs to pass workload references to scope controllers. So if that field is missed, AppConfig controller is supposed to return error. It's not responsible for AppConfig controller to handle this through patching HealthScope forcefully.

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

Successfully merging this pull request may close these issues.

3 participants