Followup on 1221 - further condition improvements #1226
Conversation
evankanderson
requested review from
matthewmcnew,
tomkennedy513,
chenbh and
pviraj59
as code owners
| case buildNeeded == corev1.ConditionUnknown && sourceResolver.Ready(): | ||
| ready.Status = corev1.ConditionUnknown | ||
| ready.Reason = UnknownStateReason | ||
| ready.Message = "Build status unknown" |
There was a problem hiding this comment.
Excitingly, this code isn't covered, as I changed the reason and no tests failed.
(I've been carrying this state around, but I'm not sure what it's supposed to represent...)
There was a problem hiding this comment.
@matthewmcnew is this related to that libgit2 bug where the source resolver hangs forever? I can't think of any scenarios that would result in status unknown when both builder and source resolver are ready.
There was a problem hiding this comment.
This was https://github.com/pivotal/kpack/blob/7857820a20481e34b338c345ee4640ada53bb195/pkg/reconciler/image/reconcile_build.go#L74-L88, which would have had a message of "" and status Unknown with no Reason.
This is now explicit enough that at least it's obvious that we don't know how we got into this state. 😁
There was a problem hiding this comment.
Looking at the source, it appears that today the only way to get to this state is when SourceResolver or Builder are not ready. Of course, a future refactor could change that, so I'd like to have some sort of default "not okay" status.
evankanderson
force-pushed
the
unconditional-logic
branch
from
05a070b to
2cc25cc
Compare
Also improve failed build error message Signed-off-by: Evan Anderson <evana@vmware.com>
evankanderson
force-pushed
the
unconditional-logic
branch
from
2cc25cc to
3cc0616
Compare
Codecov Report
@@ Coverage Diff @@
## main #1226 +/- ##
==========================================
- Coverage 67.67% 67.56% -0.12%
==========================================
Files 132 132
Lines 8053 8037 -16
==========================================
- Hits 5450 5430 -20
- Misses 2173 2177 +4
Partials 430 430
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I incorporated Daniel's suggestion, which caused me to also re-evaluate the failed build error message. I incorporated the name of the build, which seemed like an improvement.