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

feat(core/managed): put constraints in a 'skipped' state on skipped versions #8620

Merged
merged 2 commits into from
Oct 5, 2020

Conversation

erikmunson
Copy link
Member

@erikmunson erikmunson commented Oct 3, 2020

When a version becomes skipped in an environment, constraints are no longer evaluated and version is mostly forgotten, never to be heard from again. The exception would be if you manipulate the normal flow of versions via pinning/marking as bad, which would forcefully switch their state to something else.

Despite this relative irrelevance, constraints on skipped versions continue to appear very meaningful and important because they don't actually change state in any way. We've discussed adding a SKIPPED status on the backend, but there are some pretty big, pretty urgent fish to fry in keel that should get attention first. This change makes a low-effort attempt to patch over the lack of a SKIPPED status by checking for the skipped state on environments in client side code.

I've elected to make a few design decisions that may or may not be good:

  • If a version is skipped in an environment, I'm not considering any pending or failed constraints as worthy of showing up in the sidebar. I think failed constraints are the one I'm unsure about here — do people want to see a big red bubble on the sidebar for something that's skipped?
  • When stateful constraints are PENDING or NOT_EVALUATED, I'm switching them to a new skipped state that says something like "manual judgement skipped" and doesn't have buttons/a timestamp. If they are FAIL or PASS, though, I'm retaining that data as I think it's useful, but visually greying it out so it's relative importance is accurately portrayed
  • For stateless constraints, I'm adding a skipped handler that can check whether the constraint is passing. On the depends-on constraint I'm still showing the 'already deployed to...' message if it's passing, because again I think that's useful information
Sidebar

Before
Screen Shot 2020-10-02 at 5 53 38 PM

After
Screen Shot 2020-10-02 at 5 53 13 PM

Pending stateful constraint

Before
Screen Shot 2020-10-02 at 5 53 48 PM

After
Screen Shot 2020-10-02 at 5 57 15 PM

Pass/fail stateful constraint

Before
Screen Shot 2020-10-02 at 6 09 23 PM

After
Screen Shot 2020-10-02 at 5 52 12 PM

@gcomstock I would love your input on any/all of these choices

@gcomstock
Copy link
Contributor

If a version is skipped in an environment, I'm not considering any pending or failed constraints as worthy of showing up in the sidebar. I think failed constraints are the one I'm unsure about here — do people want to see a big red bubble on the sidebar for something that's skipped?

Agreed, let's hide them until we get feedback otherwise

When stateful constraints are PENDING or NOT_EVALUATED, I'm switching them to a new skipped state that says something like "manual judgement skipped" and doesn't have buttons/a timestamp. If they are FAIL or PASS, though, I'm retaining that data as I think it's useful, but visually greying it out so it's relative importance is accurately portrayed

I've been using the outlined version of icons to represent something that hasn't happened. In your first example, where a manual judgement was never given, it makes sense to me. In the second example, where the manual judgement was rejected, by removing the color, I feel we're losing some information unnecessarily. (something happened here but we're conveying that it didn't)

Your inclination to get rid of the red is probably right. Same case if something became pinned then unpinned. It wouldn't feel right to have the same callout as pinned!

As for the skipped constraint, I think I'd also like to know why. See how this feels @erikmunson @emjburns

Screen Shot 2020-10-05 at 10 30 43 AM

@emjburns
Copy link
Contributor

emjburns commented Oct 5, 2020

@gcomstock I like that better. Do you think it would be confusing to show some sort of red / green for pass / fail? When I look at that greyed out bubble it's hard to tell what the status was. I guess I'm not sure if I'd need that information, but if it visually matched a bit more to its non-greyed-out self I think I could better associate them in my head and would have an easier time building a mental model.

@gcomstock
Copy link
Contributor

@gcomstock I like that better. Do you think it would be confusing to show some sort of red / green for pass / fail? When I look at that greyed out bubble it's hard to tell what the status was. I guess I'm not sure if I'd need that information, but if it visually matched a bit more to its non-greyed-out self I think I could better associate them in my head and would have an easier time building a mental model.

I'm not sure! I see the value in the color and the distraction in it. I think I'd want to ask more people and see it in action to have a better opinion.

@erikmunson
Copy link
Member Author

I like reserving the dotted outline exclusively for things that haven't happened yet/at all — re: the open question about how much color would/would not be useful here I agree it's tough to tell in advance of real world data. I basically just guessed at this first pass. Let me add in the solid color treatment in that mock and we'll see what people think day-to-day. This is the sort of stuff we may also discover new information about via watching people use the UI (we haven't done that yet, but I bet we will eventually).

@erikmunson
Copy link
Member Author

Ok I've done a few things, screenshots below:

  • Using filled-in bubbles in the cases discussed (vs. the outline ones)
  • I started using the approved/rejected manual judgement icons where appropriate. This was a bit tedious and required some refactoring, which is why I haven't done it in the past. I think this change (i.e. not using color) makes the status-specific icons more important
  • Modified the status cards to be able to show greyed-out text while showing a filled-in bubble, to match @gcomstock's screenshot in this thread
  • Re-worded the skipped depends-on text because it was awkwardly phrased, "skipped" is now the first word
  • Note: I did not add an explanatory detail to skipped depends-on constraints. This is quite involved to do well, and it's not currently clear to me whether it's worth the technical investment given so much else is signaling 'skipped for a later version' already here in this list. If it's really bothersome I can do it, just some extra tedium

Screen Shot 2020-10-05 at 12 06 41 PM

Screen Shot 2020-10-05 at 12 06 23 PM

Screen Shot 2020-10-05 at 12 07 08 PM

Screen Shot 2020-10-05 at 12 07 17 PM

@emjburns
Copy link
Contributor

emjburns commented Oct 5, 2020

Using the actual status icons is enough for me right now! Thanks @gcomstock and @erikmunson. As I use things I'll point out if it's hard to tie things together in my head

@gcomstock
Copy link
Contributor

Excellent!

@erikmunson erikmunson merged commit 90a2819 into spinnaker:master Oct 5, 2020
@erikmunson erikmunson deleted the skipped-version-constraints branch October 5, 2020 21:08
erikmunson pushed a commit that referenced this pull request Oct 5, 2020
80858e7 feat(core/managed): limit artifact versions to 30 (#8623)
90a2819 feat(core/managed): put constraints in a 'skipped' state on skipped versions (#8620)
815742d feat(core/managed): scroll to selected version in sidebar, update scroll containers (#8618)
de24ef3 feat(core/managed): apply consistent sorting to resources (#8622)
3735483 fix(core/pipeline): Always enable "show revision history" in edit pipeline dialog (#8621)
abffd84 fix(core/managed): fixup some details from new layout (#8619)
f1bb04e fix(appname): encodeURIComponent for app name (#8586)
60c0c7b feat(validation): Allow app name validators to be overridden (#8584)
mergify bot pushed a commit that referenced this pull request Oct 5, 2020
…e@0.0.516 docker@0.0.60 google@0.0.21 oracle@0.0.9 tencentcloud@0.0.6 (#8624)

* chore(amazon): publish amazon@0.0.269

f1bb04e fix(appname): encodeURIComponent for app name (#8586)

* chore(azure): publish azure@0.0.255

f1bb04e fix(appname): encodeURIComponent for app name (#8586)

* chore(cloudfoundry): publish cloudfoundry@0.0.101

f1bb04e fix(appname): encodeURIComponent for app name (#8586)

* chore(core): publish core@0.0.516

80858e7 feat(core/managed): limit artifact versions to 30 (#8623)
90a2819 feat(core/managed): put constraints in a 'skipped' state on skipped versions (#8620)
815742d feat(core/managed): scroll to selected version in sidebar, update scroll containers (#8618)
de24ef3 feat(core/managed): apply consistent sorting to resources (#8622)
3735483 fix(core/pipeline): Always enable "show revision history" in edit pipeline dialog (#8621)
abffd84 fix(core/managed): fixup some details from new layout (#8619)
f1bb04e fix(appname): encodeURIComponent for app name (#8586)
60c0c7b feat(validation): Allow app name validators to be overridden (#8584)

* chore(docker): publish docker@0.0.60

f1bb04e fix(appname): encodeURIComponent for app name (#8586)

* chore(google): publish google@0.0.21

f1bb04e fix(appname): encodeURIComponent for app name (#8586)

* chore(oracle): publish oracle@0.0.9

f1bb04e fix(appname): encodeURIComponent for app name (#8586)

* chore(tencentcloud): publish tencentcloud@0.0.6

f1bb04e fix(appname): encodeURIComponent for app name (#8586)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants