Skip to content

[WIP] Improve CommitStatus #26247

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

Closed
wants to merge 10 commits into from
Closed

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Jul 31, 2023

Mentioned in #25990 (comment)
Fix #25990

Changes:

  1. Using Int to save CommitStatus in DB
  2. Using min in sql to get the CommitStatus instead of calculate CommitStatus in Gitea side
  3. Adding a pager bar in web ui

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 31, 2023
@yp05327 yp05327 marked this pull request as draft July 31, 2023 08:55
{{svg "gitea-exclamation" 18 "commit-status icon text red"}}
{{end}}
{{if eq .State "failure"}}
{{if eq .State 2}}
Copy link
Member

Choose a reason for hiding this comment

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

I believe that using direct numbers for judgment here leads to poor code readability and maintainability. My opinion is to continue using the previous code on the frontend page.When passing the state to the frontend, perform a conversion from numbers to strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, forgot that we have CommitStatusState.IsXXXX.
I think this is the best solution.

}

// NoBetterThan returns true if this State is no better than the given State
// This function only handles the states defined in CommitStatusPriorities
func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool {
Copy link
Member

Choose a reason for hiding this comment

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

This requires checking for special values, enhancing the robustness of the function, and avoiding issues caused by incorrect usage by the caller, as seen in #26121.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally, I want to remove this function.
It is only used in MergeRequiredContextsCommitStatus. But have no good idea about how to rewrite it now.
Or maybe I will add CommitStatusState.IsValid which should be called before we compare them.

@pull-request-size pull-request-size bot added size/XL and removed size/L labels Aug 1, 2023
@yp05327 yp05327 closed this Dec 13, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pull request check list is limited to 30 entries
3 participants