-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unstructured support in different functions #112
Unstructured support in different functions #112
Conversation
Hi @somtochiama. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
pkg/patterns/addon/application.go
Outdated
} | ||
version = v | ||
|
||
healthy, _, err = unstructured.NestedBool(unstruct.Object, "spec", "healthy") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "status" "healthy". We probably also should check the error (as you do with version)
pkg/patterns/addon/patch.go
Outdated
return fmt.Errorf("provided object (%T) does not implement Patchable type", object) | ||
} | ||
|
||
var p addonsv1alpha1.Patchable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Do you need to declare this here? Looks like you could declare it in on line 54 with else if p, ok := object....
?
pkg/patterns/addon/patch.go
Outdated
if ok { | ||
patch, _, err := unstructured.NestedSlice(unstruct.Object, "spec", "patches") | ||
if err != nil { | ||
return fmt.Errorf("unable to get patches from unstruct: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: unstructured, not unstruct, as we expect this will eventually bubble-up to the user.
@@ -46,50 +47,90 @@ type aggregator struct { | |||
func (a *aggregator) Reconciled(ctx context.Context, src declarative.DeclarativeObject, objs *manifest.Objects) error { | |||
log := log.Log | |||
|
|||
instance, ok := src.(addonv1alpha1.CommonObject) | |||
if !ok { | |||
unstruct, ok := src.(*unstructured.Unstructured) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be clearer if you constructed the healthy
state first, and then applied it to the object. i.e. move the type-checking on src to around line 89
return fmt.Errorf("unable to get status from unstructured: %v", err) | ||
} | ||
if !reflect.DeepEqual(status, s) { | ||
err = unstructured.SetNestedField(unstruct.Object, statusHealthy, "status", "healthy") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed error-check here
|
||
log.WithValues("name", unstruct.GetName()).WithValues("status", status).Info("updating status") | ||
|
||
err = a.client.Update(ctx, unstruct) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make this a.client.Update(ctx, src)
, can you eliminate the duplication between the two if branches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this would work as the status wasn't changed on src
. Unlike the SetNestedField
in unstruct
and SetCommonStatus
in instance
that changes the status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know where we don't change src
? src
and unstruct
are just a pointer-cast, so they should be the same object.
(Also, if we're changing status we might have to use client.Status().Update(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems every time we run the update function, src
has changed. I will change to client.Status().Update(...)
in another PR
/ok-to-test A few nits, but it's exciting to see this. |
4aa1e96
to
9e95327
Compare
var healthy bool | ||
var addonObject addonsv1alpha1.CommonObject | ||
|
||
if unstruct, ok := instance.(*unstructured.Unstructured); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the other PRs, we might want to create helper functions here. e.g. GetVersion
and GetHealthy
(or maybe GetCommonSpec
and GetCommonStatus
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could definitely work on this. I think it is a good idea
|
||
log.WithValues("name", unstruct.GetName()).WithValues("status", status).Info("updating status") | ||
|
||
err = a.client.Update(ctx, unstruct) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know where we don't change src
? src
and unstruct
are just a pointer-cast, so they should be the same object.
(Also, if we're changing status we might have to use client.Status().Update(...)
One query about unstruct != src (and one suggestion for future PRs to refactor out some helper methods) but this lgtm. Looks like we need a goimports fix though /approve |
[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 |
I will check on that. When we update src/unstruct, do we update the main instance? |
No description provided.