Refactors code to set and get status from object #118
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/assign @justinsb |
| Errors []string `json:"errors,omitempty"` | ||
| Phase string `json:"phase,omitempty"` | ||
| VersionCheckErrors []string | ||
| StatusErrors []string |
There was a problem hiding this comment.
Is this intended? Should we add these two fields in a separate PR?
| @@ -9,48 +9,71 @@ import ( | |||
| addonsv1alpha1 "sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/addon/pkg/apis/v1alpha1" | |||
| ) | |||
|
|
|||
| func GetCommonVersion(instance runtime.Object) (string, error) { | |||
| func genError(v runtime.Object) error { | |||
| return fmt.Errorf("instance %T is not addonsv1alpha1.CommonObject or unstructured", v) | |||
There was a problem hiding this comment.
I probably would have just inlined this function each time, but I guess it's nice that it is consistent! 👍
There was a problem hiding this comment.
Thanks. I could change it if that would be more concise.
| log.Error(err, "failed to get status") | ||
| } | ||
| compiledStatus := utils.CompileErrors(status) | ||
| fmt.Println("compiling errors") |
There was a problem hiding this comment.
Should be log, rather than using fmt which will go to stdout.
There was a problem hiding this comment.
Sure. I will change it
| func CompileErrors(status addonsv1alpha1.CommonStatus) addonsv1alpha1.CommonStatus { | ||
| fmt.Println(status.StatusErrors) | ||
| status.Errors = []string{} | ||
| status.Errors = append(status.Errors, status.VersionCheckErrors...) |
There was a problem hiding this comment.
I'm a little uneasy about introducing this in this refactor PR
|
Is it possible to split the version errors into a different PR and keep this just as a refactor? (I can show you how I would do it in git if you'd like...) There's also some ongoing discussions about more structured error handling: kubernetes/community#4521 so I think we should be careful when introducing new status! |
| @@ -47,9 +47,11 @@ type CommonSpec struct { | |||
|
|
|||
| // CommonSpec is a set of status attributes that must be exposed on all addons. | |||
There was a problem hiding this comment.
nit: replace CommonSpec with CommonStatus
Sure. I will separate it into different PR and then we can discuss the change there |
somtochiama
force-pushed
the
helper-functions
branch
from
af3c7ad to
462d6f7
Compare
| log.WithValues("name", src.GetName()).WithValues("phase", aggregatedPhase).Info("updating status") | ||
| err := k.client.Status().Update(ctx, src) | ||
| err = k.client.Status().Update(ctx, src) |
There was a problem hiding this comment.
Nit - if you're not going to need error out side the block, it's probably better to do either err := ... or if err := foo(); err != nil
|
Thanks @somtochiama - looks great :-) /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, SomtochiAma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
No description provided.