Validate nil pointer exceptions during BuildRun Reconcile #396
Conversation
qu1queee
requested review from
HeavyWombat and
SaschaSchwarze0
and removed request for
sbose78 and
zhangtbj
When generating the rampup metrics, we needed to validate for nil pointers in order to avoid some panic() scenarios. Adding a test case for this validation. When metrics are not used, some variables are not initialized, leading to panic() scenarios when using them later in the reconciliation of the BuildRun controller. Adding some if conditionals to validate nils.
qu1queee
force-pushed
the
qu1queee/test_cases_metrics
branch
from
e3260a2 to
f082e14
Compare
|
/assign @qu1queee |
|
|
||
| // taskrun ramp-up duration (time between taskrun creation and taskrun pod creation) | ||
| buildmetrics.TaskRunRampUpDurationObserve( | ||
| if buildRun.Status.BuildSpec.StrategyRef != nil { |
There was a problem hiding this comment.
Hm, I think you are uncovering a bug in the BuildSpec. The StrategyRef is not optional and therefore should not be a reference - similar to Source and Output that are also mandatory. Can you change this in build_types.go and remove this nil check here again ?
There was a problem hiding this comment.
removed, thats a good point. Unit tests know nothing about the tags. Also, when you say "Can you change this in build_types.go" , what do you meant?
There was a problem hiding this comment.
I mean this loc:
. It should not be*StrategyRef but StrategyRef. But, I just looked around, there are more similar problems like the BuildRef in the build run. So, you can also leave it as is and I open an issue to separately address all this problems. If you quickly confirm, I will approve this PR and address the wrong references separately.
There was a problem hiding this comment.
Yes, pls open the issue first. I would like to have the discussion around the pointer or not, interesting.
There was a problem hiding this comment.
Issue opened: #397
Let's provide feed back there. This PR is fine then.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SaschaSchwarze0 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 |
openshift-ci-robot
added
the
approved
When generating the rampup metrics, we needed to validate for nil
pointers in order to avoid some panic() scenarios. Adding a test
case for this validation.
When metrics are not used, some variables are not initialized, leading
to panic() scenarios when using them later in the reconciliation of the
BuildRun controller. Adding some if conditionals to validate nils.