Use warning icon for stages/jobs that are allowed to fail

mentioned in issue #21925 (closed)

I think it should only be displayed when its failed not?

Just an idea for graphic implementation in the mini pipeline graph inside the pipeline overview. I think the straight forward line is pretty easy to implement in https://gitlab.com/gitlab-org/gitlab-ce/issues/21925 from the start

as for icons:

cc: @tauriedavis

Mentioned in issue #21950 (moved)

The green going through the red really messes with my eyes. IMO the mini graph is easier to look at when the connecting line is grey.

I'm not entirely sure what that new icon is. Is A suppose to represent allowed to fail?

From @markpundsack's description it sounded like we already had an icon we used for this scenario. Can you post that Mark? cc @hazelyang any thoughts?

It's in app/views/shared/icons/_icon_status_warning.svg.

I agree that the graph looks nicer with the gray connecting line. And that the lines going through the icons doesn't work.

The first mockup with line going around the X is kinda cool, and conveys a lot of meaning well, but is probably overkill. Also, we still need an icon on the full pipeline graph, and in build lists, etc. where we might not be able to convey the information with lines. So I think we still need to go with the warning icon everywhere. If we then also want to do the fancy graphic treatment, that can be a separate bonus.

Updated description.

Milestone changed to %8.13

Also, hover text should be clear that it failed, but it's allowed to, or succeeded with warnings, or whatever the phrasing is.

I love the green line under the red stage btw, its super nice.

Added feature proposal label

Mentioned in issue #21131 (closed)

Mentioned in issue #22445 (closed)

@markpundsack @tauriedavis

I agree this should just need the warning icon, with a tooltip:

Mini pipeline graph in pipeline list:

build view builds list:

I first had an idea with a double border:

that with warning icon results in just over designing imo, also the warning icon works in the mini graph in pipeline list as well as the builds list in the build view! :) :

Looks good @dimitrieh

lets get this to front-end ^^ :) @annabeldunstone @lbennett

updated original issue body with the correct design mocks

Added frontend and removed ~19492 labels

Looks great!

@filipa would you care to take this up? :)

Reassigned to @filipa

@filipa could you check for me, and if it is the case, see if #E75E40 is the color used for

When a job fails that is allowed to fail, we use a warning symbol for the status of the pipeline as a whole

I would like the same color used for this issue's edits

sooo see if:

or this color is used:

@filipa reminded me if the allowed to fail tag is stil necessary.. imo it isn't with this improvement! :)

cc: @markpundsack

Mentioned in issue #22191 (closed)

Sounds good.

@filipa you were busy on this not?

@dimitrieh Yes, I started changing the icon but haven't created a MR yet. Plan to do so until Friday.

If it is blocking anyone I can push the branch and this can be reassigned

@dimitrieh can you help me list all the places this needs to be changed?

Pipelines list, in the mini pipeline graph in the column 'stages'- https://gitlab.com/gitlab-org/gitlab-ce/pipelines
Builds List - https://gitlab.com/gitlab-org/gitlab-ce/builds
Build view in top message and right sidebar - https://gitlab.com/gitlab-org/gitlab-ce/builds/4867934
Pipeline Graph - https://gitlab.com/gitlab-org/gitlab-ce/pipelines/4521196
Build List in Merge request - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6691/builds
Project page in last commit message - https://gitlab.com/gitlab-org/gitlab-ce

Are this all the places?

@filipa

Pipelines list, in the mini pipeline graph in the column 'stages'- https://gitlab.com/gitlab-org/gitlab-ce/pipelines
Pipelines list, in the status button in the column 'status'- https://gitlab.com/gitlab-org/gitlab-ce/pipelines
Builds List, in the status button in the column 'status' - https://gitlab.com/gitlab-org/gitlab-ce/builds
Build view, in top message status button - https://gitlab.com/gitlab-org/gitlab-ce/builds/4867934
Build view, in right sidebar builds list - https://gitlab.com/gitlab-org/gitlab-ce/builds/4867934
Pipeline view, in the pipeline Graph - https://gitlab.com/gitlab-org/gitlab-ce/pipelines/4521196
Pipeline view, in the status button in the build list below the pipeline Graph - https://gitlab.com/gitlab-org/gitlab-ce/pipelines/4521196
Merge request view, in the builds tab in the pipeline graph - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6691/builds
Merge request view, in the builds tab in the build list below the pipeline Graph - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6691/builds
Merge request view, in the builds commit tab - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6691/commits
Project page in last commit message - https://gitlab.com/gitlab-org/gitlab-ce
Project forks page if a project has a build status - https://gitlab.com/gitlab-org/gitlab-ce/forks
Projects page status icon if a project has a build status - https://gitlab.com/dashboard/projects
Profile view, in the projects tab - https://gitlab.com/u/filipa/projects
Profile view, in the contributed tab - https://gitlab.com/u/filipa/contributed

I think that concludes it.. I think.. At least all different visual variations are in here. Probably a lot of these are updated by the same functions?!?

As you are busy on these icons :)

could you fix the color on the created icon in the pipeline tab in the MR view?

Currently it looks like image, it should have the color: #8f8f8f

omg! thank you so much @dimitrieh !

This is a huge help!

Yes I can fix the icon color :)

One more question, regarding the color of the warning icon. Which one should it be:

$orange-light: #fc8a51;
$orange-normal: #e75e40;

the icon is the $orange-light

@dimitrieh @filipa It seems to me that there is also a build list in the administration area.

@alexispires thank you! Can you provide a link please?

@filipa /admin/builds

@ayufan @grzesiek I believe there are some backend changes that need to be done to be able to show this icon regarding to stages, am I correct?

@filipa

I found #E75E40 was used else where, however the color used in my mockups is the one I prefer ($orange-light), but it should be used consistently that is most important:)

can you check where else the $orange-normal is used?

@dimitrieh I don't think it is. There is no css for the warning icon yet I believe.

But I'll make sure we only use one color

Mentioned in merge request !6801 (closed)

@markpundsack in order to show the correct icon in all this places this could have some backend changes to keep the logic out of the frontend. @grzesiek mentioned the logic of knowing if the stage failed could use some work on the backend.

Can you please assign someone from the backend to make all the changes needed to show this in the frontend?

Meanwhile I started changing some code, where is the MR. It's still in the beginning. !6801 (closed) I will keep updating it for the places where I do have all the info needed.

@ayufan can assign someone to the backend.

@grzesiek @godfat Are you opt for challenge of doing that?

@ayufan Now I think I followed and I would love to take this. I think I saw the discussion somewhere saying that we could introduce PipelineStage, and I agree this could be a good idea. However to get this done, we don't really need it. So my question is should we do that with this, or let's just do it simple for now?

I think the above discussion was on Slack: https://gitlab.slack.com/archives/ci/p1476099636001954

Marked the task Pipeline view, in the status button in the build list below the pipeline Graph - https://gitlab.com/gitlab-org/gitlab-ce/pipelines/4521196 as completed

Marked the task Pipeline view, in the pipeline Graph - https://gitlab.com/gitlab-org/gitlab-ce/pipelines/4521196 as completed

Actually I think we should introduce PipelineStage later, since stage status could be done easily with just an extra method, so I don't see if we would want to take this chance to complicate things. We should probably only do it when it's really getting nasty or there's a feature that we really need it. If database query is a concern, I think we could just do one query and calculate the status from Ruby.

How about:

adding to Ci::Build the new status: success_with_warnings,
extend state machine with:

    event :drop do
      transition [:created, :pending, :running] => :success_with_warnings, if: :allow_failure?
      transition [:created, :pending, :running] => :failed
    end

extend HasStatus#status_sql with support for success_with_warnings:

success_with_warnings = scope.success_with_warnings.select('count(*)').to_sql
...
WHEN (#{builds})=(#{success})+(#{success_with_warnings}) THEN 'success_with_warnings'

In all places we will now have the success_with_warnings if at least of on builds were allowed to fail.
Remove from MergeRequestsController#ci_status the status = "success_with_warnings" if pipeline.success? && pipeline.has_warnings? as this will be calculated now automatically.

If do it like this we will not need the PipelineStage for now.

Maybe the CI::Build status should not be named success_with_warnings, but more like failed_but_allowed_to_fail or allowed_fail.

I think that's a good idea to solve this since we're indeed using this over the places. I would prefer the name success_with_warnings because it's similar to what we're showing on the UI and the other alternative names are rather long or a bit unclear, not really specifying it's actually successful.

@godfat

How about: failed_but_allowed?

We also have a cancel. If you do a cancel this status should also be ignored, so we would also need: canceled_but_allowed.

@ayufan Umm... does that mean allow_failure would also be considered ok if it's cancelled? Then I think we probably need to go with failed_but_allowed and cancelled_but_allowed if we're showing them differently.

@godfat

Yes

@godfat

Could you also document the purpose of all internal statuses?

@ayufan Yes, sure, I'll write comments in the state machine, and we'll see if we need to put them into documentation as well.

Marked the task Build view, in top message status button - https://gitlab.com/gitlab-org/gitlab-ce/builds/4867934 as completed

Marked the task Build view, in right sidebar builds list - https://gitlab.com/gitlab-org/gitlab-ce/builds/4867934 as completed

Reassigned to @godfat

Added ~480950 label

@ayufan After fiddling a bit, I think let's stick with success_with_warnings because it's a bit hard to tell whether it's failed_but_allowed or canceled_but_allowed, for example what if there are 2 failed but allowed and 2 cancelled but allowed, which status it should be? Sure we could favour one over the other, but I think we're not showing it differently for now and we could see how we want to differentiate them later. Personally as a user I don't really care which one it is.

@godfat

I'm not sure in using success_with_warnings as a sink to failed and canceled, because we lose the information.

failed it's a build failure,
canceled it's a user action.

Also success_with_warnings for a build, it's also misleading, because this build did not succeeed, it did fail.

@ayufan Umm... however we don't want to change the status from database to success_with_warnings for both cancelled and failed jobs. So I think we should keep the original status, but only give success_with_warnings for pipelines or collection of jobs via HasStatus.status.

@ayufan Yes, exactly what I was talking above. Not sure why your comment wasn't shown before I wrote the above one.

@godfat

It is getting complicated. So you propose to introduce a new status for pipeline and pipeline_stage: success_with_warnings?

I start to think that this is MVP and it makes sense. So we would only make change in HasStatus.status.

I also start to belive that we should look into splitting HasStatus to have separate one for Build and separate for Pipeline.

@ayufan Yes, because I feel builds already know it's allow_failure or not, combining with the old failed and success states, we already have all the information. There's little points to duplicate information and make the state machine even more complicated. Also, we don't want to show success_with_warnings when we're showing individual job. We want to know exactly it's successful or failed.

However pipelines and pipeline_stages are different (well, actually grouped jobs might also need this!), because they're mixture of states. It's very reasonable to have different states for different icons. We don't want to do the same thing for individual job.

I think indeed at some point we should split HasStatus for Build and Pipeline, because they're getting more and more different. Perhaps... HasStatus and HaveStatuses :P Well I don't really like this name but that's the idea. One for individual state, and the other for a group of states, combined to a single state.

@markpundsack @dimitrieh In the process of implementing this, I realized that while it makes sense to show "warnings" for pipelines and pipeline stages, it would lose some information for individual jobs, because allow_failure also allow cancellation. I think for example when showing an individual job, we should show the real status that is failed or cancelled. However, I don't think we would want to show inconsistent icons between showing individual jobs and in the pipeline graph. So I think we should probably still show failed or cancelled in the pipeline graph, and perhaps also some other ways to indicate that those jobs were allow_failure.

Or alternatively, we could just show cancelled regardless allow_failure, and only show warnings if and only if it's failed and allow_failure. This way it would still show consistent icons, sacrificing the case where it's cancelled while allow_failure.

What do you think?

/cc @filipa @ayufan

@ayufan If we choose to do the alternative way, that is only show warnings for failed and allow_failure, and still show cancelled even it's allow_failure, then I think we could still do this:

transition [:created, :pending, :running] => :success_with_warnings, if: :allow_failure?

And keep canceled status, don't make it success_with_warnings even it's allow_failure.

The downside of this would be that it's hard to tell if it's allowing failure or not for canceled status.

Mentioned in commit e50ba9fb

@dimitrieh @tauriedavis What do you think about using icon that contains both statuses' colors? Like green circle (because it is actually successful build) with exclamation mark inside with color of the actual status (red for failed build, gray for canceled)? I tried to prepare mockup but it is soo bad, that I won't show it here

@godfat From the implementation perspective, I agree that we should not lose information about real build status. allowed_to_fail is just a decorator, so I would prefer to a avoid using success_with_warnings status.

@godfat

I agree with your comment. While implementing this I felt that a warning icon without more explanation was not enough to understand what really happened. The failed status and the "allowed to fail" label were clear to me.

Like @grzesiek said, I do think we should not lose information about real build status.

Maybe we should put this on hold and think better about it

//cc @ayufan @dimitrieh

Let's try to solve one problem at a time, because we have a mixture here:

Mark a build with a warning,
Mark a group of builds (for pipeline or for stage) with success_with_warnings.

As for the 1., I'm not sure if introducing extra statuses for it: failed_but_allowed and success_but_allowed or success_with_warnings or warning is a good thing. I think oposite. Maybe we can ignore that one for now and concentrate on 2.

The second is to include failed + allow_failure in stage and pipeline to show success_with_warnings (icon with exclamation mark). I belive that this makes sense to have that as a first-class pipeline status, because it's not decor as allow_failure.

To do 2., we have to split HasStatus to have separate implementation for Ci::Build and separate one for Ci::Pipeline as I don't really want to mess a statuses there. When we do that we can easily add a new pipeline-level status: success_with_warnings, the same status would be used to to calculate pipeline stage. We could show then exclamation mark.

When we solve the 2. which seems to be simpler, we could think about 1. as this will be separated from 2.

Thanks everyone!

@grzesiek I like the idea of using the same color of circle for the same status. I think it should be clear as long as we know what it does mean. Not sure for a new comer if that's clear enough though. This might or might not be a problem.

@ayufan We could start splitting HasStatus from here, though I think it's not really strictly required here. As you can see, the only difference for now is the check against exclude_ignored and failed_but_allowed, the rest could all be shared. And indeed I already introduced success_with_warnings for pipeline :P

@ayufan @filipa @godfat @dimitrieh

#2 (closed) from Kamil's comment is the highest priority since it's the mini pipeline graph that is the biggest problem. So if we have to cut scope, let's get that shipped.

However, I don't believe #1 (closed) can be ignored for long. When looking at a pipeline graph with a build that failed, but the graph continues on processing, it's really confusing to the user. It looks like a bug in GitLab. There's no indication in the graph that the job is allowed to fail. Using the success_with_warning exclamation mark was determined to be the best option. Several other icons were proposed, but none that worked any better.

And in hindsight, we're now converging on a principal that the pipeline status, pipeline stage status, and build status should all have parity. So using the same exclamation mark for all three feels right.

Unfortunately, we never considered canceled builds in that discussion. So I do see the problem with canceled builds that are allowed to fail. If we show both failed and canceled builds with the same icon, then we lose information. And if we go with Jen-Shin's alternate proposal then we lose information about the canceled job being allowed to fail.

In terms of priority, considering that the canceled job problem never even came up, but the failed job problem certainly did, I'd lean towards doing Jen-Shin's alternate proposal and only handling this for failed_but_allowed and ignoring canceled jobs (for now). Especially since he's already implemented it.

But I'm also fine with cutting #1 (closed) and thinking about it more. Perhaps we can come up with distinct icons for failed_but_allowed and canceled_but_allowed, or come up with some other way of indicating the attribute allowed_to_fail as meta information on the pipeline graph nodes. (On the build list we use a tag, but there's no room for that in the graph.)

Also, perhaps the pipeline status (and pipeline stage status) needs to differentiate between success-with-warnings-because-of-failures and success-with-warnings-because-of-canceled (and success-with-multiple-warnings)? I dislike the explosion of states we've got going on. Perhaps we have less states, but show verbose information on hover.

I like the idea with the same icon but different mouseover. I won't try to do this unless a decision is made though. The alternative proposal is somehow simple to implement (the commit which reverted it), but I am not very sure if keeping both information would be the same effort.

@dimitrieh I think using the same icon would make sense. What are your thoughts here?

@tauriedavis My first thoughts towards this is indeed using the same icons

I dislike the explosion of states we've got going on

This is a big part of why.

Also the icons are represented on the pipeline graph and pipeline mini graph in the pipeline list. These should be easily digestible and not need a legend on icon definitions

cc: @markpundsack

Mentioned in issue #23492 (closed)

Mentioned in issue #23069 (moved)

Milestone changed to %8.14

Mentioned in issue #23583 (closed)

Added ~901060 and removed frontend labels

Mentioned in issue #24344 (closed)

Milestone changed to %8.15

Added Deliverable label

Mentioned in issue #24163 (closed)

Mentioned in issue #24643 (closed)

assigned to @ayufan

@godfat We recently merged new pipeline and stage statuses, so it should be doable already.

@grzesiek Yes, @ayufan said he would take it over though :) Let me know if @ayufan still wants me to do it.

It didn't happen in 8.15 :(

changed milestone to %8.16

assigned to @grzesiek

mentioned in issue #25139 (closed)

@dimitrieh Can we exclude changing the status of failed build (which is allowed to fail) to "warning" in this iteration? I'm not sure if this is something we should do. Can we change icon for stage only, and then iterate on that?

mentioned in merge request !8503 (merged)

Merge request: !8503 (merged).

Can we exclude changing the status of failed build (which is allowed to fail) to "warning" in this iteration? I'm not sure if this is something we should do. Can we change icon for stage only, and then iterate on that?

@grzesiek i am of the same mind in this ;)

this will most probably need an alteration anyway with the discussion going on here: https://gitlab.com/gitlab-org/gitlab-ce/issues/25139#note_21079893

@dimitrieh Fortunately this MR and icons redesign shouldn't be mutually exclusive, as we already have icon for success with warnings status. We can change how it looks, because from the backend perspective we only refer to the icon name, not particular SVG. Does it sound reasonable?

closed via merge request !8503 (merged)

mentioned in issue #13349 (moved)

Use warning icon for stages/jobs that are allowed to fail

Description

Proposal

Single Source of Truth

Links / references

Places where it needs to be changed

Designs

Child items ...

Activity

Admin message

Admin message

Use warning icon for stages/jobs that are allowed to fail

Description

Proposal

Single Source of Truth

Links / references

Places where it needs to be changed

Activity