Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.9k
Add GitHub API compatibility for workflow runs filtering#34894
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Implements additional query parameters for the workflow runs API to match GitHub's REST API specification.- Add `exclude_pull_requests` query parameter- Add `check_suite_id` parameter- Add `created` parameter with date range and comparison support- Add workflow-specific endpoint `/repos/{owner}/{repo}/actions/workflows/{workflow_id}/runs`Builds on the workflow API foundation fromgo-gitea#33964 to provide additional GitHub API compatibility.Reference:https://docs.github.com/en/rest/actions/workflow-runs?apiVersion=2022-11-28#list-workflow-runs-for-a-workflowAdd missing query parameters to the Swagger documentation for the workflow runs listing endpoint to match GitHub's API: actor, branch, event, status, created, exclude_pull_requests, check_suite_id, and head_sha.
528de08 toaeaa2c1CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
✅ Review Comments AddressedThanks for the feedback@wxiaoguang! I've addressed all the review comments: Changes Made:
Quality Checks:
The implementation now fully matches GitHub's API date format specification and follows Gitea's code patterns. |
models/actions/run_list.go Outdated
| cond = cond.And(builder.Neq{"`action_run`.trigger_event": webhook_module.HookEventPullRequest}) | ||
| } | ||
| if opts.CheckSuiteID > 0 { | ||
| cond = cond.And(builder.Eq{"`action_run`.check_suite_id": opts.CheckSuiteID}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is no such column at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So, take that out, that small part of the GitHub API compatibility won't exist, but it's a pretty minor aspect, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It seem that this PR is written by AI, and the non-existing column is caused by AI hallucination?
We need to make sure every line you proposed to change should be fully understood and has a clear meaning, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To be clear: AI is good, I also use AI. I mean: I think we shouldn't keep the non-existing column in code, we need to leave a clear comment for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, yeah - for sure. So, remove it and leave a comment somewhere ... noticeable that the API is compatible except for this field. I'll get to work on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, I think I got a little screwy with the commits, but all the changes seem to be there again. I left thecheck_suite_id in the swagger with a description that it's not supported - does that seem appropriate?
8b7ed3a to63d924dCompareThe CheckSuiteID parameter was referencing a non-existent 'check_suite_id'column in the action_run table. This commit removes the hallucinated databaseschema reference while maintaining API compatibility.Changes:- Remove CheckSuiteID field from FindRunOptions struct- Remove check_suite_id database query condition- Remove parameter handling logic from shared action handler- Remove related tests for non-functional feature- Update Swagger docs to indicate parameter is not supported in Gitea API- Maintain GitHub API compatibility by keeping parameter documentedThe check_suite_id parameter is now silently ignored when provided,with clear documentation that it's not supported in Gitea.
63d924d to1b9b410Compare| // TestListRunsWorkflowFiltering tests that ListRuns properly handles | ||
| // the workflow_id path parameter for filtering runs by workflow. | ||
| func TestListRunsWorkflowFiltering(t *testing.T) { | ||
| unittest.PrepareTestEnv(t) | ||
| ctx, _ := contexttest.MockAPIContext(t, "user2/repo1") | ||
| contexttest.LoadRepo(t, ctx, 1) | ||
| contexttest.LoadUser(t, ctx, 2) | ||
| // Test case 1: With workflow_id parameter (simulating /workflows/{workflow_id}/runs endpoint) | ||
| ctx.SetPathParam("workflow_id", "test-workflow-123") | ||
| // Simulate the FindRunOptions creation that happens in ListRuns | ||
| opts := actions_model.FindRunOptions{ | ||
| OwnerID: 0, | ||
| RepoID: ctx.Repo.Repository.ID, | ||
| WorkflowID: ctx.PathParam("workflow_id"), // This is the key change being tested | ||
| } | ||
| // Verify the WorkflowID is correctly extracted from path parameter | ||
| assert.Equal(t, "test-workflow-123", opts.WorkflowID) | ||
| assert.Equal(t, ctx.Repo.Repository.ID, opts.RepoID) | ||
| assert.Equal(t, int64(0), opts.OwnerID) | ||
| // Test case 2: Without workflow_id parameter (general /runs endpoint) | ||
| ctx2, _ := contexttest.MockAPIContext(t, "user2/repo1") | ||
| contexttest.LoadRepo(t, ctx2, 1) | ||
| contexttest.LoadUser(t, ctx2, 2) | ||
| // No SetPathParam call - simulates general runs endpoint | ||
| opts2 := actions_model.FindRunOptions{ | ||
| RepoID: ctx2.Repo.Repository.ID, | ||
| WorkflowID: ctx2.PathParam("workflow_id"), | ||
| } | ||
| // Verify WorkflowID is empty when path parameter is not set | ||
| assert.Empty(t, opts2.WorkflowID) | ||
| assert.Equal(t, ctx2.Repo.Repository.ID, opts2.RepoID) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This test seem to test, SetPathParam and PathParam instead of the code added into ListRuns.
I would expect this test to depend on the logic Gitea would execute, not on a independent copy.
Yes this test tests that a similar logic in ListRuns could work right now, but does not ensure this works in the future. AI tools are pretty good in generating something that looks very good at first glance
Moving assigning the actions_model.FindRunOptions struct into a private function called by ListRuns and this Test, then you would have some coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch@ChristopherHX - I had actually caught a few other instances where the tests were testing something entirely different or incidental versus the important logic, but I missed this one. Is there some sort of coverage report available where I could do a better audit?
I'll get this fixed up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Addressed in6b074cb
- Extract FindRunOptions construction logic from ListRuns into buildRunOptions function- Update TestListRunsWorkflowFiltering and related tests to use actual production code- Tests now verify production logic instead of duplicating parameter parsing- Improved error handling by returning errors instead of direct HTTP responses- Ensures future changes to parameter parsing are properly testedThis addresses the issue where tests were bypassing production code logic,making them less reliable for catching regressions.
…h/gitea into feature/enhanced-workflow-runs-api* 'feature/enhanced-workflow-runs-api' of github.com:bdruth/gitea: [skip ci] Updated translations via Crowdin Follow file symlinks in the UI to their target (go-gitea#28835) Fix issue filter (go-gitea#34914) Fix: RPM package download routing & missing package version count (go-gitea#34909) Add support for 3D/CAD file formats preview (go-gitea#34794) Add a `login`/`login-name`/`username` disambiguation to affected endpoint parameters and response/request models (go-gitea#34901) Improve tags list page (go-gitea#34898) [skip ci] Updated translations via Crowdin docs: fix typo in pull request merge warning message text (go-gitea#34899) Refactor container package (go-gitea#34877) [skip ci] Updated translations via Crowdin
| @@ -101,6 +105,15 @@ func (opts FindRunOptions) ToConds() builder.Cond { | |||
| if opts.CommitSHA != "" { | |||
| cond = cond.And(builder.Eq{"`action_run`.commit_sha": opts.CommitSHA}) | |||
| } | |||
| if !opts.CreatedAfter.IsZero() { | |||
| cond = cond.And(builder.Gte{"`action_run`.created": opts.CreatedAfter}) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think this will work. It should beopts.CreatedAfter.Unix().
| cond = cond.And(builder.Gte{"`action_run`.created": opts.CreatedAfter}) | ||
| } | ||
| if !opts.CreatedBefore.IsZero() { | ||
| cond = cond.And(builder.Lte{"`action_run`.created": opts.CreatedBefore}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The same as above.
Summary
Implements additional query parameters for the workflow runs API to match GitHub's REST API specification.
exclude_pull_requestsquery parametercheck_suite_idparametercreatedparameter with date range and comparison support/repos/{owner}/{repo}/actions/workflows/{workflow_id}/runsBuilds on the workflow API foundation from#33964 to provide additional GitHub API compatibility.
Test plan
FindRunOptions.ToConds()Reference