Add filter by ref to project jobs resolver
What does this MR do and why?
For #367294
This MR modifies the parameters that you can provide when getting the list of jobs for a project via GraphQL: it adds a ref
filter that should allow you to get only the jobs that ran for that ref.
Screenshots or screen recordings
Screenshots from GraphiQL
jobs query without ref | jobs query with ref |
---|---|
![]() |
![]() |
How to set up and validate locally
git checkout 367294-project-jobs-resolver-filter-by-ref
- find/make/import a project with a pipeline with a job that generates artifacts
you can use this CI yaml to generate some sample artifacts:
# .gitlab-ci.yml
potato:
stage: build
script:
- echo 'potato' >> potato.txt
artifacts:
expose_as: 'potato'
paths: ['potato.txt']
tomato:
stage: build
script:
- echo 'tomato' >> tomato.txt
artifacts:
expose_as: 'tomato'
paths: ['tomato.txt']
pineapple:
stage: build
script:
- echo 'pineapple' >> pineapple.txt
artifacts:
expose_as: 'pineapple'
paths: ['pineapple.txt']
apple:
stage: build
script:
- echo 'apple' >> apple.txt
artifacts:
expose_as: 'apple'
paths: ['apple.txt']
toblerone:
stage: build
script:
- echo 'toblerone' >> toblerone.txt
artifacts:
expose_as: 'toblerone'
paths: ['toblerone.txt']
- run a pipeline for one branch in that project - for this you'll need to set up a local runner for GDK or enable runners in gitpod
- run a pipeline for another branch
- using the GraphQL explorer (
http://gdk.test:3000/-/graphql-explorer
), query for a project's jobs by ref:
query getJobArtifacts {
project(fullPath: "path/to/project") { # substitute the full path of your project
jobs(ref: "main") { # substitute the names of your branches
nodes {
id
status
name
refName
}
}
}
}
- when a ref is provided, only jobs that ran for that ref should be returned
- when a ref is not provided, jobs for any ref should be returned
Database details
Raw SQL
If I use this as the execute
method:
def execute
builds = init_collection.order_id_desc
builds = filter_by_with_artifacts(builds)
builds = filter_by_ref(builds)
builds = filter_by_scope(builds)
puts builds.to_sql
builds
rescue Gitlab::Access::AccessDeniedError
type.none
end
... and run bundle exec rspec spec/finders/ci/jobs_finder_spec.rb:85
(and format the SQL it outputs with pgFormatter), then I get:
SELECT
"ci_builds".*
FROM
"ci_builds"
WHERE
"ci_builds"."type" = 'Ci::Build'
AND "ci_builds"."project_id" = 23
AND ("ci_builds"."status" NOT IN ('created'))
AND "ci_builds"."ref" = 'dev'
ORDER BY
"ci_builds"."id" DESC
... and as a control, if I comment out builds = filter_by_ref(builds)
in the execute
method and run it again, I get:
SELECT
"ci_builds".*
FROM
"ci_builds"
WHERE
"ci_builds"."type" = 'Ci::Build'
AND "ci_builds"."project_id" = 29
AND ("ci_builds"."status" NOT IN ('created'))
ORDER BY
"ci_builds"."id" DESC
Query plans
Adding in the gitlab-org/gitlab
project id (278964) and the ref name master
for testing gives us these two (with and without "ci_builds"."ref" = 'master'
):
query | link to plan |
---|---|
SELECT "ci_builds".* FROM "ci_builds" WHERE "ci_builds"."type" = 'Ci::Build' AND "ci_builds"."project_id" = 278964 AND ("ci_builds"."status" NOT IN ('created')) AND "ci_builds"."ref" = 'master' ORDER BY "ci_builds"."id" DESC |
https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/13900/commands/48651 |
SELECT "ci_builds".* FROM "ci_builds" WHERE "ci_builds"."type" = 'Ci::Build' AND "ci_builds"."project_id" = 278964 AND ("ci_builds"."status" NOT IN ('created')) ORDER BY "ci_builds"."id" DESC |
https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/13900/commands/48652 |
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %15.6
assigned to @mfluharty
Please wait for Reviewer Roulette to suggest a designer for UX review, and then assign them as Reviewer. This helps evenly distribute reviews across UX.
This message was generated automatically. You're welcome to improve it.
- A deleted user
added database databasereview pending labels
2 Messages This merge request adds or changes files that require a review from the Database team. This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. This merge request requires a database review. To make sure these changes are reviewed, take the following steps:
- Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
- Prepare your MR for database review according to the docs.
- Assign and mention the database reviewer suggested by Reviewer Roulette.
The following files require a review from the Database team:
app/finders/ci/jobs_finder.rb
Documentation review
The following files require a review from a technical writer:
-
doc/api/graphql/reference/index.md
(Link to current live version)
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Alina Mihaila (
@alinamihaila
) (UTC+2, 9 hours ahead of@mfluharty
)Douglas Barbosa Alexandre (
@dbalexandre
) (UTC+0, 7 hours ahead of@mfluharty
)database Jon Jenkins (
@jon_jenkins
) (UTC-6, 1 hour ahead of@mfluharty
)Mayra Cabrera (
@mayra-cabrera
) (UTC-6, 1 hour ahead of@mfluharty
)UX Veethika M (
@v_mishra
) (UTC+0, 7 hours ahead of@mfluharty
)Maintainer review is optional for UX To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangerchanged milestone to %15.7
added missed-deliverable missed:15.6 labels
added 2285 commits
-
eac6dfa2...38fb2dd9 - 2284 commits from branch
master
- 264e6093 - Add filter by ref to project jobs resolver
-
eac6dfa2...38fb2dd9 - 2284 commits from branch
added 1154 commits
-
264e6093...8ba06140 - 1153 commits from branch
master
- 9d67cf6d - Add filter by ref to project jobs resolver
-
264e6093...8ba06140 - 1153 commits from branch
- Resolved by Miranda Fluharty
72 73 end 73 74 end 74 75 76 # rubocop: disable CodeReuse/ActiveRecord 77 def filter_by_ref(builds) 78 if params[:ref].present? 79 builds.where(ref: params[:ref]) 80 else 81 builds 82 end 83 end 84 # rubocop: enable CodeReuse/ActiveRecord 85 - Comment on lines +76 to +85
@mfluharty could you try creating a scope
Ci::Build.with_ref(ref)
and test that scope in isolation? Maybe it'll help to unearth the problem.Edited by Albert Sure - scope created in a new commit
added 2 commits
- Resolved by Miranda Fluharty
added 489 commits
-
e71e4e6d...10956828 - 488 commits from branch
master
- 44412a20 - Add filter by ref to project jobs resolver
-
e71e4e6d...10956828 - 488 commits from branch
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
- Resolved by Leonardo da Rosa
added workflowin review label and removed workflowin dev label
requested review from @l.rosa
@alinamihaila Would you review the backend here?
Thank you @mfluharty! LGTM
@dbalexandre could you please take the maintainer review when you have the chance? Thank you
Thanks, @mfluharty! I have nothing to add besides the @l.rosa notes.
requested review from @alinamihaila
mentioned in merge request !106499 (closed)
mentioned in issue #367294
requested review from @dbalexandre and removed review request for @alinamihaila
- Resolved by Leonardo da Rosa
- Resolved by Leonardo da Rosa
- Resolved by Leonardo da Rosa
- Resolved by Leonardo da Rosa
- Resolved by Leonardo da Rosa
I liked the overall approach. I left some comments on the MR that follows the DB review guidelines.
PS: We could split the
Ci::JobsFinder
into separated classes, maybe using the strategy pattern (based on each type of filtering), but that is out of the scope of this MR.Edited by Leonardo da Rosa
removed review request for @l.rosa
removed review request for @dbalexandre
added 798 commits
-
44412a20...74eefb77 - 796 commits from branch
master
- a9d5307f - Add filter by ref to project jobs resolver
- 83e53a23 - Changes from review feedback
-
44412a20...74eefb77 - 796 commits from branch
requested review from @l.rosa
@l.rosa
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
removed databasereview pending label
added databaseapproved label
removed review request for @l.rosa
requested review from @krasio
removed databaseapproved label
added databasereviewed label
@mfluharty code looks pretty good!
It follows the DB guidelines and the queries performance are excelent.
Raw SQL Query Plans Code Tests !104191 (closed) !104191 (closed) /cc @krasio I'm assigning you to do the final approval here
~"database::approved"
@mfluharty @l.rosa Seems like this is not the final SQL that will be generated, as the GraphQL API should add pagination, and we do not have
LIMIT
in this queries. To get the actual SQL it's usually better to execute a request against our local app, and then find the query in the logs. Once we have it, we can replace parameters likeproject_id
and check the plan in DB Lab.As is, the above queries will load all rows matching the filters, which is probably not what we want, and may hit the timeout if there are too many, or it's too slow to find them. This is not obvious from the execution plans we have, as they return 0 rows, and the reason for this is that these plans are from the
main
database (namedgitlab-production-tunnel-pg12
in postgres.ai), and notci
(which is calledgitlab-production-ci
here). CI related tables onmain
were truncated, hence we get no results.Can you please try again with the actual SQL and using the
ci
database, and go through review with Leo once again. Assign back for maintainer review when ready.Nice catch @krasio! @mfluharty could you please, take a look?