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): show failed, pending bubbles for any constraint type #8611

Merged
merged 3 commits into from Oct 1, 2020

Conversation

erikmunson
Copy link
Member

@erikmunson erikmunson commented Oct 1, 2020

We've had some code for a while that will look for manual-judgement constraints in a PENDING status and throw them into status bubbles on the version sidebar. This change expands that logic in two ways:

  1. We'll now surface any constraint types, not just manual-judgement
  2. We'll now show constraints with a FAIL or OVERRIDE_FAIL status using an error-colored bubble

As a bonus, I also threw in a one-liner to fix the z-index issue that made for an awkward scrolling experience (before and after below)

Failed & rejected constraints
Screen Shot 2020-10-01 at 9 45 52 AM

Scrolling (before and after)
Screen Shot 2020-10-01 at 9 45 10 AM
Screen Shot 2020-10-01 at 9 44 30 AM

cc @gcomstock

@erikmunson erikmunson added the Do Not Merge Don't merge yet label Oct 1, 2020
@@ -134,14 +136,29 @@ function getArtifactStatuses({ environments }: IManagedArtifactVersion): Artifac
// NOTE: The order in which entries are added to `statuses` is important. The highest priority
// item must be inserted first.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way i'm stacking these right now is:

  1. pending constraints — this feels like a 'things are actively happening and you should know about it' case
  2. failed constraints — it feels kind of yucky to potentially not see these if other things are pending/running, but I thought 1 was more important
  3. pinned state — important, but hopefully if you don't get a chance to see it here you'll see it in the overview or via our snazzy new warning card on other versions. I'm less worried about people missing this info if there are multiple status bubbles

});
failedConstraintTypes.forEach((type) => {
statuses.push({ appearance: 'error', iconName: getConstraintIcon(type) });
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I've setup the logic here is that we will reduce all the constraints across all the environments into a list of their unique types – so let's say there are three pending manual judgements across three environments, we'll show one single pending manual judgement bubble. If there were any failed manual judgements, we'd show one single error manual judgement bubble. So you'll see that some environments have a constraint in that state, but no context about how many. Same is true for pinning right now.

Longer term we may need to think about supporting quantities on these bubbles so we could e.g. have a pending manual judgement bubble with a little "2" to indicate that there are two environments with that constraint status.

@erikmunson erikmunson removed the Do Not Merge Don't merge yet label Oct 1, 2020
@erikmunson
Copy link
Member Author

I thought this was going to be dependent on #8610, but I ended up being wrong. 🥳

Copy link
Contributor

@vigneshm vigneshm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

@erikmunson erikmunson added the ready to merge Reviewed and ready for merge label Oct 1, 2020
@mergify mergify bot merged commit 012b004 into spinnaker:master Oct 1, 2020
@erikmunson erikmunson deleted the md-env-failed-statuses branch October 1, 2020 17:43
erikmunson pushed a commit that referenced this pull request Oct 1, 2020
34bc7e9 feat(core/managed): switch to abbreviated timestamp format, add version timestamps (#8610)
012b004 feat(core/managed): show failed, pending bubbles for any constraint type (#8611)
7604b3d feat(webhooks): document stausJsonPath is now optional (#8608)
0463ecb feat(core/presentation): add useInterval hook (#8609)
mergify bot pushed a commit that referenced this pull request Oct 1, 2020
…45 (#8612)

* chore(appengine): publish appengine@0.0.19

d36922b feat(provider/appengine): add deploy global configuration stage (#8599)

* chore(core): publish core@0.0.514

34bc7e9 feat(core/managed): switch to abbreviated timestamp format, add version timestamps (#8610)
012b004 feat(core/managed): show failed, pending bubbles for any constraint type (#8611)
7604b3d feat(webhooks): document stausJsonPath is now optional (#8608)
0463ecb feat(core/presentation): add useInterval hook (#8609)

* chore(ecs): publish ecs@0.0.265

748da07 feat(ecs): Enable embedded artifacts for Deploy stage (#8603)
61cf058 feat(ecs): add Amazon ECS Cypress tests

* chore(titus): publish titus@0.0.145

619ad9c (fix): Angular to react prop error (#8607)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Reviewed and ready for merge target-release/1.23
Projects
None yet
3 participants