Remove commit status running and warning to align GitHub #25839

CaiCandong · 2023-07-12T02:42:51Z

In the discussion of #25776, @wolfogre's suggestion was to remove the commit status of running and warning to keep it consistent with github.

references:

https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#about-commit-statuses

⚠️ BREAKING ⚠️

So the commit status of Gitea will be consistent with GitHub, only pending, success, error and failure, while warning and running are not supported anymore.

wolfogre

Please check also templates/repo/commit_status.tmpl

services/actions/commit_status.go

services/convert/status.go

Co-authored-by: Jason Song <i@wolfogre.com>

delvh · 2023-07-12T09:03:19Z

Hmm… I'm conflicted about that decision.
Sure, it would be compatible with GitHub.
On the other hand, it would also remove functionality that GitHub is basically lacking at the moment.
(At least running, I haven't quite understood yet either when warning would be triggered)

models/git/commit_status.go

wolfogre · 2023-07-12T10:11:59Z

Hmm… I'm conflicted about that decision. Sure, it would be compatible with GitHub. On the other hand, it would also remove functionality that GitHub is basically lacking at the moment. (At least running, I haven't quite understood yet either when warning would be triggered)

I don't think people care about whether a commit status is running or pending.

Yeah, for Gitea Actions, pending could be Blocked or Waiting, but the commit status is not designed for Actions. For most applications, I believe it's enough to just say "it's pending".

Co-authored-by: Jason Song <i@wolfogre.com>

wolfogre · 2023-07-12T10:40:57Z

Since it's my original idea, I won't approve it until it has been approved by the other two maintainers.

If most people are against this one and want more functionality than GitHub, maybe #25826 is a good choice.

Please feel free to approve or request changes, this one or #25826

silverwind · 2023-07-12T17:13:48Z

From my experience GitHub UI definitely has pending and warning states, even if the API does not expose it:

pending: Yellow dot, not yet started because no worker has picked it up or other dependencies not satisfied. Might be a job-only state.
warning: Orange exclamation triangle. Happens when PRs need approval to run. State is seen on a per-PR basis.

I guess it makes some sense to be API-compatible with GitHub, but internally I guess we would still need them for above two reasons.

CaiCandong · 2023-07-13T03:11:57Z

From my experience GitHub UI definitely has pending and warning states, even if the API does not expose it:

According to the github documentation:create-a-commit-status, creating a status only accepts error, failure, pending, success. My opinion is that our status definition should be consistent with github, but the specific semantic information we can express in description.

silverwind · 2023-07-13T10:58:27Z

You're right we have to distinguish:

Commit status can be error, failure, pending, success.
Workflow run status can be completed, action_required, cancelled, failure, neutral, skipped, stale, success, timed_out, in_progress, queued, requested, waiting, pending.

So I'm fine with reducing the commit status to those GitHub-compatible values.

silverwind · 2023-07-13T11:01:20Z

Can we ensure that for our actions workflow runs, the statuses there are correctly mapped down to those 4 values?

wolfogre · 2023-07-14T01:56:46Z

Can we ensure that for our actions workflow runs, the statuses there are correctly mapped down to those 4 values?

I'm sure, but we should be aware that it could lose some detail, you know, it's “mapping down”.

lunny · 2023-07-14T02:41:11Z

Even if we don't reduce those two statuses, Gitea is also compatible with Github. Why these two statuses are unnecessary?

wolfogre · 2023-07-20T04:41:05Z

So pending and running are exactly the same now, see #25935 (comment)

silverwind · 2023-07-21T08:40:08Z

There might be some remaining unused conditionals in UI now for the icon, need to check.

…#26036) Also added comments so the next time the dashboard repo list won't be forgotten Follows #25839 Signed-off-by: Yarden Shoham <git@yardenshoham.com>

* giteaoffical/main: (22 commits) Serve pre-defined files in "public", add "security.txt", add CORS header for ".well-known" (go-gitea#25974) Use frontend fetch for branch dropdown component (go-gitea#25719) Remove commit status running and warning from the dashboard repo list (go-gitea#26036) Refactor to use urfave/cli/v2 (go-gitea#25959) Remove commit status running and warning to align GitHub (go-gitea#25839) Fix escape problems in the branch selector (go-gitea#25875) Update README.md to fix the broken link of Hugo (go-gitea#26008) Support copy protected branch from template repository (go-gitea#25889) Update JS dependencies (go-gitea#26025) Reduce margins on admin pages (go-gitea#26026) Actions Artifacts support uploading multiple files and directories (go-gitea#24874) [skip ci] Updated translations via Crowdin Remove redundant "RouteMethods" method (go-gitea#26024) Adding remaining enum for migration repo model type. (go-gitea#26021) RPM Registry: Show zypper commands for SUSE based distros as well (go-gitea#25981) Fix the route for pull-request's authors (go-gitea#26016) Remove nfnt/resize and oliamb/cutter (go-gitea#25999) Correctly refer to dev tags as nightly in the docker docs (go-gitea#26004) Fix env config parsing for "GITEA____APP_NAME" (go-gitea#26001) Add file status for API "Get a single commit from a repository" (go-gitea#16205) (go-gitea#25831) ...

This problem occurs because in #25839, the warning status has been removed, but there is something in the tmpl that hasn't been changed. related #25839 close #26118

jpraet · 2023-09-21T07:24:24Z

Noticed this PR in the 1.21-rc0 changelog. I think commit status "warning" was useful for checks where you want to draw the attention of the developer, but not block the merge of the PR. e.g. sonar static code analysis has severities, and we want to block the merge for newly introduced [Blocker, Critical, Major] violations, and give a warning for [Minor] violations.
Or for example block the merge on code coverage < 50%, but give a warning on code coverage < 70%.

And jenkinsci/gitea-plugin will also break because it uses warning to report unstable build (test failures):

jpraet · 2023-09-21T17:48:35Z

Besides jenkins, also other CI integrations use warning commit status like:

silverwind · 2023-09-21T17:55:04Z

I guess these have to migrate to failure now. Overall I think we're better off being compatible with GitHub, so that integrations can be seamlessly be integrated in both Gitea and GitHub.

silverwind · 2023-09-21T18:28:49Z

On second thought, warning does seem like a valid use case. I wonder how GitHub avoided adding it so far. They do have inline annotations with Gitea does not have, and I think those can be both warnings and errors.

silverwind · 2023-09-21T18:37:33Z

I checked another data point, GitLab. Here is a table illustrating the commit statuses of each:

	pending	running	cancelled	warning	failed	success	error
github and gitlab	pending	-	-	-	failure	success	error
gitlab	pending	running	cancelled	-	failed	success	error
gitea before	pending	running	-	warning	failure	success	error

lafriks · 2023-09-21T20:10:44Z

Personally I don't see any benefit from this change. Why do we need this to be compatible with GitHub in expense of breaking existing integrations?

silverwind · 2023-09-21T21:02:02Z

I guess I agree to restoring warning, but do we need running?

jpraet · 2023-09-21T21:08:48Z

running was only introduced quite recently (in v1.19) #21937, so less risk of breaking any existing integrations

silverwind · 2023-09-22T10:21:28Z

So, partial revert of this that edits the migration as well? Of course the damage to existing statuses is already done for anyone using prereleases.

harryzcy · 2023-10-04T02:21:18Z

Do we actually want to revert this? If we do, it's better to do this sooner so the damage is minimal

silverwind · 2023-10-04T10:21:33Z

Yes, someone please raise a PR with the revert. We also need to disable this part of the migration.

If no one raises the revert, I can probably do it in a few days.

delvh · 2023-10-04T10:23:52Z

I think there is already an existing revert PR.
I simply don't know which exactly anymore.

silverwind · 2023-10-07T13:17:15Z

#27504 for the partial revert.

Partial revert of #25839. This commit status is used by a number of external integrations, so I think we should not remove it (See #25839 (comment)). This is a rare case where an existing migration needed to be alterted to avoid data loss. --------- Co-authored-by: delvh <dev.lh@web.de> Co-authored-by: Giteabot <teabot@gitea.io>

Partial revert of go-gitea#25839. This commit status is used by a number of external integrations, so I think we should not remove it (See go-gitea#25839 (comment)). This is a rare case where an existing migration needed to be alterted to avoid data loss. --------- Co-authored-by: delvh <dev.lh@web.de> Co-authored-by: Giteabot <teabot@gitea.io>

@silverwind

Backport #27504 by @silverwind Partial revert of #25839. This commit status is used by a number of external integrations, so I think we should not remove it (See #25839 (comment)). This is a rare case where an existing migration needed to be alterted to avoid data loss. Co-authored-by: silverwind <me@silverwind.io> Co-authored-by: delvh <dev.lh@web.de>

Remove commit status running and warning

bcee9d2

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 12, 2023

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 12, 2023

wolfogre reviewed Jul 12, 2023

View reviewed changes

services/actions/commit_status.go Outdated Show resolved Hide resolved

services/convert/status.go Outdated Show resolved Hide resolved

wolfogre added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Jul 12, 2023

wolfogre added this to the 1.21.0 milestone Jul 12, 2023

CaiCandong and others added 2 commits July 12, 2023 12:10

Update services/convert/status.go

5905217

Co-authored-by: Jason Song <i@wolfogre.com>

Remove useless front-end template code

1207a46

CaiCandong changed the title ~~Remove commit status running and warning to align github~~ WIP: Remove commit status running and warning to align github Jul 12, 2023

CaiCandong marked this pull request as draft July 12, 2023 05:47

CaiCandong added 3 commits July 12, 2023 15:07

fix compatible with old version status

27beba3

fix lint

e008032

test remove commit status running and warning

ba86521

CaiCandong marked this pull request as ready for review July 12, 2023 07:10

fix remove test

5c7b4ff

CaiCandong changed the title ~~WIP: Remove commit status running and warning to align github~~ Remove commit status running and warning to align github Jul 12, 2023

wolfogre reviewed Jul 12, 2023

View reviewed changes

models/git/commit_status.go Outdated Show resolved Hide resolved

Deprecated: it will be removed after v1.22.0

ee67bad

Co-authored-by: Jason Song <i@wolfogre.com>

CaiCandong changed the title ~~Remove commit status running and warning to align github~~ Proposal remove commit status running and warning to align github Jul 14, 2023

wolfogre mentioned this pull request Jul 20, 2023

Make pending commit status yellow again #25935

Merged

yardenshoham mentioned this pull request Jul 21, 2023

Remove commit status running and warning from the dashboard repo list #26036

Merged

CaiCandong deleted the feature-remove-commit-status-running-and-warning branch July 24, 2023 07:38

CaiCandong mentioned this pull request Jul 25, 2023

remove IsWarning in tmpl #26120

Merged

yp05327 mentioned this pull request Jul 25, 2023

Fix wrong commit status in web ui #26121

Merged

lunny pushed a commit that referenced this pull request Jul 25, 2023

remove IsWarning in tmpl (#26120)

ab72f7e

This problem occurs because in #25839, the warning status has been removed, but there is something in the tmpl that hasn't been changed. related #25839 close #26118

silverwind mentioned this pull request Oct 7, 2023

Restore warning commit status #27504

Merged

GiteaBot added the kind/breaking label Oct 7, 2023

delvh removed the kind/breaking label Oct 7, 2023

GiteaBot added the kind/breaking label Oct 7, 2023

GiteaBot mentioned this pull request Oct 8, 2023

Restore warning commit status (#27504) #27529

Merged

go-gitea locked as resolved and limited conversation to collaborators Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove commit status running and warning to align GitHub #25839

Remove commit status running and warning to align GitHub #25839

CaiCandong commented Jul 12, 2023 •

edited by wolfogre

wolfogre left a comment

delvh commented Jul 12, 2023

wolfogre commented Jul 12, 2023

wolfogre commented Jul 12, 2023 •

edited

silverwind commented Jul 12, 2023 •

edited

CaiCandong commented Jul 13, 2023

silverwind commented Jul 13, 2023 •

edited

silverwind commented Jul 13, 2023

wolfogre commented Jul 14, 2023

lunny commented Jul 14, 2023

wolfogre commented Jul 20, 2023

silverwind commented Jul 21, 2023

jpraet commented Sep 21, 2023

jpraet commented Sep 21, 2023

silverwind commented Sep 21, 2023 •

edited

silverwind commented Sep 21, 2023 •

edited

silverwind commented Sep 21, 2023

lafriks commented Sep 21, 2023

silverwind commented Sep 21, 2023

jpraet commented Sep 21, 2023

silverwind commented Sep 22, 2023 •

edited

harryzcy commented Oct 4, 2023 •

edited

silverwind commented Oct 4, 2023

delvh commented Oct 4, 2023

silverwind commented Oct 7, 2023

Remove commit status running and warning to align GitHub #25839

Remove commit status running and warning to align GitHub #25839

Conversation

CaiCandong commented Jul 12, 2023 • edited by wolfogre

⚠️ BREAKING ⚠️

wolfogre left a comment

Choose a reason for hiding this comment

delvh commented Jul 12, 2023

wolfogre commented Jul 12, 2023

wolfogre commented Jul 12, 2023 • edited

silverwind commented Jul 12, 2023 • edited

CaiCandong commented Jul 13, 2023

silverwind commented Jul 13, 2023 • edited

silverwind commented Jul 13, 2023

wolfogre commented Jul 14, 2023

lunny commented Jul 14, 2023

wolfogre commented Jul 20, 2023

silverwind commented Jul 21, 2023

jpraet commented Sep 21, 2023

jpraet commented Sep 21, 2023

silverwind commented Sep 21, 2023 • edited

silverwind commented Sep 21, 2023 • edited

silverwind commented Sep 21, 2023

lafriks commented Sep 21, 2023

silverwind commented Sep 21, 2023

jpraet commented Sep 21, 2023

silverwind commented Sep 22, 2023 • edited

harryzcy commented Oct 4, 2023 • edited

silverwind commented Oct 4, 2023

delvh commented Oct 4, 2023

silverwind commented Oct 7, 2023

CaiCandong commented Jul 12, 2023 •

edited by wolfogre

wolfogre commented Jul 12, 2023 •

edited

silverwind commented Jul 12, 2023 •

edited

silverwind commented Jul 13, 2023 •

edited

silverwind commented Sep 21, 2023 •

edited

silverwind commented Sep 21, 2023 •

edited

silverwind commented Sep 22, 2023 •

edited

harryzcy commented Oct 4, 2023 •

edited