feat(status): add waiting status #1730
Conversation
| private fun List<ResourceHistoryEvent>.isWaiting(): Boolean { | ||
| // we expect to have only two events, but we will accept several different "unresolvable" events | ||
| // in order to be less brittle and show the user this status instead of an error | ||
| return all { it is ResourceCreated || it is ResourceCheckUnresolvable } && size < 5 |
There was a problem hiding this comment.
Maybe reorder this so you don't actually look at a bunch of events unnecessarily?
| return all { it is ResourceCreated || it is ResourceCheckUnresolvable } && size < 5 | |
| return size < 5 && all { it is ResourceCreated || it is ResourceCheckUnresolvable } |
| } | ||
|
|
||
| test("returns CURRENTLY_UNRESOLVABLE") { | ||
| expectThat(subject.getStatus(resource.id)).isEqualTo(CURRENTLY_UNRESOLVABLE) | ||
| context("more than created and unresolvable events") { |
There was a problem hiding this comment.
You're missing a test for your size < 5 condition if you want to cover all cases.
|
@luispollo addressed your comments! Thanks. |
| // we expect to have only two events, but we will accept several different "unresolvable" events | ||
| // in order to be less brittle and show the user this status instead of an error | ||
| return size < 5 && all { it is ResourceCreated || it is ResourceCheckUnresolvable } |
There was a problem hiding this comment.
The 5 seems arbitrary here. What's the thinking there?
There was a problem hiding this comment.
+1. Maybe make it a constant with a self-explanatory name?
There was a problem hiding this comment.
Totally arbitrary. I feel like if the environment is actually new it'll only have a couple of events. I wanted to make sure this new status only shows up in cases where it is definitely new. So, I chose 5. Does that make sense / seem reasonable?
There was a problem hiding this comment.
@robfletcher @luispollo I was hoping my comment of // we expect to have only two events, but we will accept several different "unresolvable" events would convey why I chose 5, is there a way I could make this comment more clear?
There was a problem hiding this comment.
My understanding
Context
If there are no approved images associated with a managed cluster, the cluster will be in a CURRENTLY_UNRESOLVABLE state.
Problem
When a user deploys a new app using Managed Delivery and looks at the environment view, their first environment will have a cluster with zero approved images, which will put it in a CURRENTLY_UNRESOLVABLE state.
This is a poor user experience for new users, because the way that CURRENTLY_UNRESOLVABLE is rendered in the UI, it looks like something is broken, even though this is expected behavior.
Proposed solution
When a cluster resource has zero approved images, show its status as WAITING instead of CURRENTLY_UNRESOLVABLE. The new WAITING status is rendered in the UI in such a way that it communicates that this is expected behavior.
The way this is implemented is that a cluster resource is in the WAITING state if the only events in its history are:
- resource was created
- resource check is unresolvable
Feedback
I agree with @robfletcher's comment that the threshold of 5 events feels arbitrary, otherwise looks good to me.
There was a problem hiding this comment.
Other than explaining why the 5 to cap events as @robfletcher mentioned, LGTM.
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Back end for spinnaker/deck#8836
Introducing a new status called "waiting" for when a resource was created and then has an resource check unresolvable status. This happens primarily (maybe only) to clusters when there is no approved image. I am updating the status so new customers don't immediately see an error.
I'll re-evaluate this in the future if it turns out that other resources w/o artifacts also hit this condition.