Skipped jobs should be considered successful

What does this MR do?

If all jobs in previous stage are all skipped, the next stage should consider previous stage succeeded.

Why was this MR needed?

Since for now we consider all manual jobs if skipped, should not block the next stage from running.

Does this MR meet the acceptance criteria?

• CHANGELOG entry added
• Documentation created/updated
• API support added
• Tests
  • Added for this feature/bug

What are the relevant issue numbers?

Closes #22598 (closed)

See also #20342 (closed) (because this merge request conflicts with it)

@ayufan I am not sure if this would be the right fix. What do you think?

Reassigned to @ayufan

Interesting. I have to consider this. It seems tempting to fix it this way.

I think the key here is that the reason why manual jobs in the middle won't stop the next stage, which is this line:

WHEN (#{builds})=(#{success})+(#{ignored})+(#{skipped}) THEN 'success' 

along with:

def status_for_prior_stages(index)
  pipeline.builds.where('stage_idx < ?', index).latest.status || 'success'
end

So if there's not just one skipped job, but also some success jobs, then it would be considered the skipped stage success. I don't feel we designed it this way, just a coincident. So to make it consistent, we could fix it by telling skipped jobs are actually successful, which is already somehow a fact now.

But to really fix this, we'll need to "fix" why manual jobs in the middle are skipped. I could do this if you think this should be the way to go.

Added 1 commit:

• f3b02b9e - Or we could simply ignore skipped manual jobs

@ayufan So here's another proposal: https://gitlab.com/gitlab-org/gitlab-ce/commit/f3b02b9e6e79f178b92a65b5cd598a4733b570bd we just explicitly ignore skipped manual jobs. However, the inconsistency would left intact.

If you cancel Stage 2, Stage 3 would be skipped and treated as successful with this MR, wouldn't it?

@connorshea Huh, good point. If that's the case, actually that could happen without this MR, because you could see that currently there's:

WHEN (#{builds})=(#{success})+(#{ignored})+(#{skipped}) THEN 'success' 

which says that even if there are some skipped jobs, it should still be considered successful. And we check if we need to go into the next stage by checking all the jobs before the stage we're checking, effectively means what you're saying could actually happen right now, as long as Stage 1 has at least a successful job.

So to properly fix this, we should treat manual job as a special case, rather than checking skipped status and we should also fix the above line, which shouldn't be considered successful, but maybe skipped instead.

For now I really make it treat manual jobs as a special case, so let's see if we could change it.

Unfortunately it's breaking some tests. I think it's because when: on_failure skipped shouldn't block the next stage either.

Added 1 commit:

• dd01f3a7 - Should use eq because we want orders

Mentioned in commit 33bbfd27

Added 1 commit:

• 33bbfd27 - on_failure should also be ignored, and status_sql should

@connorshea Oh, now I realized that what you're worrying won't happen because status for cancelled jobs are actually in canceled, not skipped. However the concern above that when: on_failure is still valid.

Added 1 commit:

• f0002da0 - Add a test for on_failure jobs in the middle

Reassigned to @godfat

Now we should somehow do the same thing for CommitStatus.ignored and HasStatus.exclude_ignored_jobs, perhaps merging them. I'll take back for now and continue to work on this on Monday.

Marked this merge request as a Work In Progress

Added 1 commit:

• 8dfec32d - Rename ignored to failed_but_allowed, introduce exclude_ignored

Added 1 commit:

• 23e0a3ee - Specify 3 cases we want them to be excluded. [ci skip]

Added 3 commits:

• 705d210c - Style/EmptyLinesAroundBlockBody
• 9c230600 - Remove dead code
• 752a4cce - Add test for CommitStatus.exclude_ignored

Added 1 commit:

• afc0ae5c - Fix tests. Check 'success' first (default status)

Added 154 commits:

• afc0ae5c...8c5701b6 - 153 commits from branch master
• 44558c72 - Merge branch 'master' into all-skipped-equals-success

Added 1 commit:

• 7fb6a73d - We don't need self. there. (sorry, can't resist anymore)

Unmarked this merge request as a Work In Progress

@ayufan Now I believe this is the right fix.

@grzesiek Please help for review this too, thanks!

Reassigned to @grzesiek

Added 1 commit:

• 56f58391 - HasStatus.status is now already aware of that

@godfat I've left just a two questions, but it already looks good to me 👍 If we won't be able to find better solution for exclude_ignored then please pass to an endboss.

Reassigned to @godfat

Reassigned to @rymai

Marked the task CHANGELOG entry added as completed

Added 1 commit:

• 218331e6 - Add an entry in CHANGELOG [ci skip]

Reassigned to @godfat

Added 82 commits:

• 218331e6...c38b85f3 - 80 commits from branch master
• aafb0171 - Merge remote-tracking branch 'upstream/master' into all-skipped-equals-success
• 5c9ac560 - Introduce all_state_names so that we could avoid NOT IN

Added 5 commits:

• 5c9ac560...7887a3da - 4 commits from branch master
• 22aaebdf - Merge remote-tracking branch 'upstream/master' into all-skipped-equals-success

Reassigned to @rymai

Resolved all discussions

Thanks @godfat, LGTM! ❤

Status changed to merged

Mentioned in commit 4dc61dc7

Mentioned in commit gitlab-qa@53d38996

Mentioned in issue #23904 (closed)

Mentioned in issue #22642 (closed)

Mentioned in issue #23935 (closed)

Mentioned in issue #24140 (closed)

mentioned in issue #20342 (closed)