505: fail with message when webhook not enabled and minReplicas is nil #543
Conversation
imaffe
force-pushed
the
affe/505-hpa-nil-pointer
branch
from
0442021 to
9775ea9
Compare
|
@imaffe:Thanks for your contribution. For this PR, do we need to update docs? |
github-actions
bot
added
the
doc-info-missing
imaffe
force-pushed
the
affe/505-hpa-nil-pointer
branch
from
9775ea9 to
4a1cc0f
Compare
controllers/spec/hpa.go
Outdated
| func makeHPA(objectMeta *metav1.ObjectMeta, minReplicas, maxReplicas *int32, podPolicy v1alpha1.PodPolicy, targetRef autov2beta2.CrossVersionObjectReference) *autov2beta2.HorizontalPodAutoscaler { | ||
|
|
||
| panicIfNil(minReplicas, "MinReplicas should not be nil but is nil; This is likely because the webhook is not installed properly so it does not have a default value") | ||
| panicIfNil(maxReplicas, "MaxReplicas should not be nil but is nil; This is likely because the webhook is not installed properly so it does not have a default value") |
There was a problem hiding this comment.
I think the maxReplicas is optional?
There was a problem hiding this comment.
I added this check to make sure they are not null (from the code's perspective), but the message should be changed if the maxReplicas is optional and could not be null in this case.
|
@imaffe do we need to update the docs for this PR? Thanks |
|
I think we can put the logic to determine if |
imaffe
added
no-need-doc
imaffe
force-pushed
the
affe/505-hpa-nil-pointer
branch
2 times, most recently
from
8fde604 to
e921cbb
Compare
imaffe
force-pushed
the
affe/505-hpa-nil-pointer
branch
from
e921cbb to
bd8b2c0
Compare
| func panicIfNil(value interface{}, message string) { | ||
| if value == nil { | ||
| panic(message) | ||
| } |
There was a problem hiding this comment.
we should never panic in a controller's reconciliation loop
|
close by #582 |
We have two methods to avoid the nil pointer exception.
Method 1 is to provide a default value for the possible nil fields just like in the admission webhook. However adding default value in the code could potentially make it harder to find where the config value comes from when debugging. And it also require us to change two places if we were to change the default value logic.
Method 2 is simple: fail early but with hint messages on what caused the issue and how to resolve the issue. It guides the user to use a newer version of function-mesh with webhooks enabled.
Fixes #505
fix #505