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

Fix duplicate status check contexts #30660

Merged
merged 8 commits into from
Apr 30, 2024

Conversation

Zettat123
Copy link
Contributor

Caused by #30076.

There may be some duplicate status check contexts when setting status checks for a branch protection rule. The duplicate contexts should be removed.

Before:

After:

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 23, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 23, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 23, 2024
@lunny lunny added backport/v1.21 This PR should be backported to Gitea 1.21 backport/v1.22 This PR should be backported to Gitea 1.22 labels Apr 23, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 23, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 25, 2024

It doesn't seem right.

Think about a case (index, context) in which all the records satisfy "updated_unix":

100, context1
100, context2
101, context3

Your SQL gets all 3 contexts, but only context3 should be used because it is the last.

@Zettat123
Copy link
Contributor Author

Your SQL gets all 3 contexts, but only context3 should be used because it is the last.

Since the title of the list is "Status checks found in the last week for this repository", I think we should show all unique contexts whose updated_unix is in the last week. So maybe we should show the context[1-3] in the case you mentioned?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 26, 2024

But for all cases, a new status check should override an old one? Why it should show an outdated status context1 to users?

(updated: not related to failure or success)

@Zettat123
Copy link
Contributor Author

But for all cases, a new status check should override an old one? Why it should show an outdated status context1 to users?

I see. I guess now I understand the case.

For example, I initially had two contexts: test/front and test/back. In commit bbbb, I modified the workflow files and rename the two contexts to test/frontend and test/backend.

index context sha updated_unix
1 test/front aaaa 1711600000
2 test/back aaaa 1711600000
1 test/frontend bbbb 1711600100
2 test/backend bbbb 1711600100

If now I call the function, I should only get test/frontend and test/backend. Do I understand correctly?

@wxiaoguang
Copy link
Contributor

Yup, I think so

@Zettat123 Zettat123 marked this pull request as draft April 29, 2024 01:50
@Zettat123
Copy link
Contributor Author

Zettat123 commented Apr 29, 2024

I agree that we shouldn't show contexts that no longer exist, but after some testing I found that it's not easy to do so.

In the previous example, I renamed two contexts. Then if I rerun the first two jobs, the records become:

index context sha updated_unix
1 test/front aaaa 1711600000
2 test/back aaaa 1711600000
1 test/frontend bbbb 1711600100
2 test/backend bbbb 1711600100
3 test/front aaaa 1711600200
4 test/back aaaa 1711600200

In this case, it's difficult to determine which contexts are the most recent. Since the contexts displayed in the list are only for reference, I think here we can show all the unique contexts and let users determine which contexts should be matched.

@Zettat123 Zettat123 marked this pull request as ready for review April 29, 2024 07:58
@Zettat123 Zettat123 requested a review from wxiaoguang April 29, 2024 08:09
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

No idea whether it is good enough. Approve without really checking.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 29, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 29, 2024
@lunny lunny merged commit 7ad5031 into go-gitea:main Apr 30, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Apr 30, 2024
@GiteaBot
Copy link
Collaborator

I was unable to create a backport for 1.21. @Zettat123, please send one manually. 🍵

go run ./contrib/backport 30660
...  // fix git conflicts if any
go run ./contrib/backport --continue

@GiteaBot GiteaBot added backport/manual No power to the bots! Create your backport yourself! and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Apr 30, 2024
Zettat123 added a commit to Zettat123/gitea that referenced this pull request Apr 30, 2024
Caused by go-gitea#30076.

There may be some duplicate status check contexts when setting status
checks for a branch protection rule. The duplicate contexts should be
removed.

Before:
<img
src="https://github.com/go-gitea/gitea/assets/15528715/97f4de2d-4868-47a3-8a99-5a180f9ac0a3"
width="600px" />

After:
<img
src="https://github.com/go-gitea/gitea/assets/15528715/ff7289c5-9793-4090-ba31-e8cb3c85f8a3"
width="600px" />
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 30, 2024
* giteaofficial/main:
  Fix duplicate status check contexts (go-gitea#30660)
  Fix issue label rendering in the issue popup (go-gitea#30763)
  Fix all rounded borders, change affected tab menus to pills (go-gitea#30707)
  Rename CodeIndexerEnabled to IsRepoIndexerEnabled (go-gitea#30762)
  Remove fomantic dimmer module (go-gitea#30723)
  Resolve lint for unused parameter and unnecessary type arguments (go-gitea#30750)
  Add support for npm bundleDependencies (go-gitea#30751)
  Fix cross-compilation errors when CGO_CFLAGS/CGO_LDFLAGS is set (go-gitea#30749)
lunny pushed a commit that referenced this pull request Apr 30, 2024
Backport #30660.

Caused by #30076.

There may be some duplicate status check contexts when setting status
checks for a branch protection rule. The duplicate contexts should be
removed.

Before:
<img

src="https://github.com/go-gitea/gitea/assets/15528715/97f4de2d-4868-47a3-8a99-5a180f9ac0a3"
width="600px" />

After:
<img

src="https://github.com/go-gitea/gitea/assets/15528715/ff7289c5-9793-4090-ba31-e8cb3c85f8a3"
width="600px" />
Zettat123 added a commit to Zettat123/gitea that referenced this pull request Apr 30, 2024
Caused by go-gitea#30076. 

There may be some duplicate status check contexts when setting status
checks for a branch protection rule. The duplicate contexts should be
removed.

Before:
<img
src="https://github.com/go-gitea/gitea/assets/15528715/97f4de2d-4868-47a3-8a99-5a180f9ac0a3"
width="600px" />

After:
<img
src="https://github.com/go-gitea/gitea/assets/15528715/ff7289c5-9793-4090-ba31-e8cb3c85f8a3"
width="600px" />
@lunny lunny added the backport/done All backports for this PR have been created label Apr 30, 2024
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 30, 2024
* origin/main: (55 commits)
  Fix dashboard commit status null access (go-gitea#30771)
  Fix tautological conditions (go-gitea#30735)
  Get repo assignees and reviewers should ignore deactivated users (go-gitea#30770)
  Right align the "Settings" menu item in overflow-menu (go-gitea#30764)
  Fix duplicate status check contexts (go-gitea#30660)
  Fix issue label rendering in the issue popup (go-gitea#30763)
  Fix all rounded borders, change affected tab menus to pills (go-gitea#30707)
  Rename CodeIndexerEnabled to IsRepoIndexerEnabled (go-gitea#30762)
  Remove fomantic dimmer module (go-gitea#30723)
  Resolve lint for unused parameter and unnecessary type arguments (go-gitea#30750)
  Add support for npm bundleDependencies (go-gitea#30751)
  Fix cross-compilation errors when CGO_CFLAGS/CGO_LDFLAGS is set (go-gitea#30749)
  [skip ci] Updated licenses and gitignores
  add built js files to eslint ignore (go-gitea#30737)
  Gitea with first upper case + typos (go-gitea#30739)
  Fix documentation build problems because of MDX syntax conflicts (go-gitea#30744)
  Remove disk-clean workflow (go-gitea#30741)
  Bump `github.com/google/go-github` to v61 (go-gitea#30738)
  Fix nil dereference on error (go-gitea#30740)
  Use `ProtonMail/go-crypto` for `opengpg` in tests (go-gitea#30736)
  ...
lunny pushed a commit that referenced this pull request May 1, 2024
Backport #30660.

Caused by #30076. 

There may be some duplicate status check contexts when setting status
checks for a branch protection rule. The duplicate contexts should be
removed.

Before:
<img

src="https://github.com/go-gitea/gitea/assets/15528715/97f4de2d-4868-47a3-8a99-5a180f9ac0a3"
width="600px" />

After:
<img

src="https://github.com/go-gitea/gitea/assets/15528715/ff7289c5-9793-4090-ba31-e8cb3c85f8a3"
width="600px" />
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/manual No power to the bots! Create your backport yourself! backport/v1.21 This PR should be backported to Gitea 1.21 backport/v1.22 This PR should be backported to Gitea 1.22 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants