Skip to content
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

Update ASO V2 dependencies #1648

Merged
merged 3 commits into from
Jul 14, 2021

Conversation

matthchr
Copy link
Member

No description provided.

obj = obj.DeepCopyObject()
obj = obj.DeepCopyObject().(client.Object)

gr.Log.V(0).Info("Reconcile invoked", "kind", fmt.Sprintf("%T", obj))
Copy link
Member Author

@matthchr matthchr Jul 14, 2021

Choose a reason for hiding this comment

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

minor: Is there a cleaner way to log the type being reconciled in the logr.Logger interface (since it doesn't support format specifiers?)

Copy link
Member

Choose a reason for hiding this comment

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

The best suggestion I have is to break out an intention revealing local:

Suggested change
gr.Log.V(0).Info("Reconcile invoked", "kind", fmt.Sprintf("%T", obj))
objKind := fmt.Sprintf("%T", obj)
gr.Log.V(0).Info("Reconcile invoked", "kind", objKind)

Another way would be to introduce a function on each of our generated types that generates a description to use here, but that's a lot of overhead.

@@ -6,18 +6,39 @@ cloud.google.com/go v0.44.2/go.mod h1:60680Gw3Yr4ikxnPRS/oxxkBccT6SA1yMk63TGekxK
cloud.google.com/go v0.45.1/go.mod h1:RpBamKRgapWJb87xiFSdk4g1CME7QZg3uwTez+TSTjc=
cloud.google.com/go v0.46.3 h1:AVXDdKsrtX33oR9fbCMu/+c1o8Ofjq6Ku/MInaLVg5Y=
cloud.google.com/go v0.46.3/go.mod h1:a6bKKbmY7er1mI7TEI4lsAkts/mkhTSZK8w33B4RAg0=
cloud.google.com/go v0.50.0/go.mod h1:r9sluTvynVuxRIOHXQEHMFffphuXHOMZMycpNR5e6To=
Copy link
Member Author

Choose a reason for hiding this comment

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

Sure whatever you say golang

@matthchr matthchr force-pushed the feature/v2-controller-runtime-update branch from 67973aa to f32ed3c Compare July 14, 2021 17:39
@codecov-commenter
Copy link

Codecov Report

Merging #1648 (67973aa) into master (77f01f2) will decrease coverage by 0.00%.
The diff coverage is 77.27%.

❗ Current head 67973aa differs from pull request most recent head f32ed3c. Consider uploading reports for the commit f32ed3c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1648      +/-   ##
==========================================
- Coverage   66.97%   66.97%   -0.01%     
==========================================
  Files         205      205              
  Lines       14818    14824       +6     
==========================================
+ Hits         9925     9929       +4     
- Misses       4134     4137       +3     
+ Partials      759      758       -1     
Impacted Files Coverage Δ
...d/pkg/testcommon/error_translating_roundtripper.go 12.50% <0.00%> (-0.55%) ⬇️
...pkg/codegen/pipeline/resource_registration_file.go 0.00% <0.00%> (ø)
...ack/generated/pkg/testcommon/be_deleted_matcher.go 35.29% <66.66%> (ø)
hack/generated/pkg/util/kubeclient/kube_client.go 78.94% <71.42%> (-8.56%) ⬇️
hack/generated/pkg/testcommon/ensure.go 54.28% <77.77%> (+5.56%) ⬆️
.../generated/pkg/testcommon/kube_per_test_context.go 84.17% <90.90%> (ø)
hack/generated/controllers/controller_resources.go 100.00% <100.00%> (ø)
hack/generated/controllers/generic_controller.go 73.46% <100.00%> (ø)
...erated/pkg/testcommon/kube_test_context_envtest.go 83.54% <100.00%> (ø)
hack/generated/pkg/testcommon/test_context.go 62.60% <100.00%> (+0.93%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77f01f2...f32ed3c. Read the comment docs.

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

A couple of questions, but nothing of import.

knownTypes := getKnownStorageTypes()

knownTypes = append(knownTypes, new(resources.ResourceGroup))

return knownTypes
}

func GetKnownTypes() []runtime.Object {
func GetKnownTypes() []client.Object {
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between runtime.Object and client.Object and why the change?

Copy link
Member Author

@matthchr matthchr Jul 14, 2021

Choose a reason for hiding this comment

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

client.Object is actually a combo of runtime.Object and metav1.Object. You can see them explain it here.

This is actually a nice win because we actually want to make sure all of our CRD types are client.Object (have both runtime,Object and metav1.Object), so using this everywhere we can helps ensure that.

They also obviously changed the signatures of a lot of the client methods we use to expect it.

hack/generated/controllers/crd_disk_test.go Show resolved Hide resolved
obj = obj.DeepCopyObject()
obj = obj.DeepCopyObject().(client.Object)

gr.Log.V(0).Info("Reconcile invoked", "kind", fmt.Sprintf("%T", obj))
Copy link
Member

Choose a reason for hiding this comment

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

The best suggestion I have is to break out an intention revealing local:

Suggested change
gr.Log.V(0).Info("Reconcile invoked", "kind", fmt.Sprintf("%T", obj))
objKind := fmt.Sprintf("%T", obj)
gr.Log.V(0).Info("Reconcile invoked", "kind", objKind)

Another way would be to introduce a function on each of our generated types that generates a description to use here, but that's a lot of overhead.

hack/generated/pkg/testcommon/test_context.go Outdated Show resolved Hide resolved
@matthchr matthchr force-pushed the feature/v2-controller-runtime-update branch from f32ed3c to 456bef7 Compare July 14, 2021 20:34
@matthchr matthchr merged commit 4460654 into Azure:master Jul 14, 2021
@matthchr matthchr deleted the feature/v2-controller-runtime-update branch July 14, 2021 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants