Activate GitlabSchemaValidateConnection for all tests
What does this MR do and why?
After the Database Decomposition Project has been finished, we want to make sure that future features always access the right database for each table. That's why we are adding this feature to both development
and test
environments, to help the developers early on detect any possible issues. FYI, on Staging and Production we raise errors if you try to write to the wrong database.
In order to achieve this, as part of this MR we are enabling the Query Analyzer GitlabSchemasValidateConnection
in Rspecs, to make sure that we are not reading/writing to the wrong database.
We are also suppressing this analyzer using the taint :suppress_gitlab_schemas_validate_connection
on any old database migrations that happened to be writing to the CI tables on the Main database.
Issue: #365761 (closed)
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.2
assigned to @OmarQunsulGitlab
Suggested Reviewers (beta)
The individuals below may be good candidates to participate in the review based on various factors.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Suggested Reviewers @ahegyi
,@pbair
,@DylanGriffith
,@tkuah
,@rpereira2
If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".
Edited by GitLab Reviewer-Recommender Bot1 Warning For the following files, a review from the Data team and Product Intelligence team is recommended
Please check the product intelligence Service Ping guide or the Snowplow guide.For MR review guidelines, see the Service Ping review guidelines or the Snowplow review guidelines.
spec/lib/gitlab/usage/service_ping_report_spec.rb
1 Message CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelog
trailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: true
trailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
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 Roy Zwambag ( @rzwambag
) (UTC+2, same timezone as@OmarQunsulGitlab
)Thong Kuah ( @tkuah
) (UTC+12, 10 hours ahead of@OmarQunsulGitlab
)database Andy Soiron ( @Andysoiron
) (UTC+2, same timezone as@OmarQunsulGitlab
)Alex Ives ( @alexives
) (UTC-5, 7 hours behind@OmarQunsulGitlab
)product intelligence Piotr Skorupa ( @pskorupa
) (UTC+2, same timezone as@OmarQunsulGitlab
)Maintainer review is optional for product intelligence 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
Danger- Resolved by 🤖 GitLab Bot 🤖
@OmarQunsulGitlab - please add typebug typefeature, typemaintenance or a subtype label to this merge request.- typebug: Defects in shipped code and fixes for those defects. This includes all the bug types (availability, performance, security vulnerability, mobile, etc.)
- typefeature: Effort to deliver new features, feature changes & improvements. This includes all changes as part of new product requirements like application limits.
- typemaintenance: Up-keeping efforts & catch-up corrective improvements that are not Features nor Bugs. This includes restructuring for long-term maintainability, stability, reducing technical debt, improving the contributor experience, or upgrading dependencies.
See the handbook for more guidance on classifying.
This message was created with automation and Engineering Productivity is looking for feedback in this issue:
https://gitlab.com/gitlab-org/quality/engineering-productivity/team/-/issues/43
added 60 commits
-
ec670f86...ba07dcba - 59 commits from branch
master
- d083b3a2 - Activate GitlabSchemaValidateConnection for all tests
-
ec670f86...ba07dcba - 59 commits from branch
Allure report
allure-report-publisher
generated test report!review-qa-blocking:
test report for 1a14760fexpand test summary
+-----------------------------------------------------------------------------------------+ | suites summary | +------------------------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Verify | 12 | 0 | 1 | 12 | 13 | ❗ | | Protect | 2 | 0 | 0 | 2 | 2 | ❗ | | Create | 23 | 0 | 2 | 23 | 25 | ❗ | | Plan | 47 | 0 | 1 | 47 | 48 | ❗ | | Manage | 38 | 0 | 2 | 40 | 40 | ❗ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | | Secure | 2 | 0 | 0 | 2 | 2 | ❗ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | | Feature flag handler sanity checks | 9 | 0 | 0 | 9 | 9 | ❗ | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Total | 133 | 0 | 9 | 135 | 142 | ❗ | +------------------------------------+--------+--------+---------+-------+-------+--------+
added 105 commits
-
d083b3a2...43344756 - 104 commits from branch
master
- 9b8de985 - Activate GitlabSchemaValidateConnection for all tests
-
d083b3a2...43344756 - 104 commits from branch
added 9 commits
-
9b8de985...b966fd1d - 8 commits from branch
master
- 8cf1e039 - Activate GitlabSchemaValidateConnection for all tests
-
9b8de985...b966fd1d - 8 commits from branch
added 1 commit
- 09617e21 - Activate GitlabSchemaValidateConnection for all tests
added 47 commits
-
09617e21...f385216f - 46 commits from branch
master
- ce1de434 - Activate GitlabSchemaValidateConnection for all tests
-
09617e21...f385216f - 46 commits from branch
- Resolved by Etienne Baqué
One of the failures I see in this MR pipeline is from https://gitlab.com/gitlab-org/gitlab/-/blob/19c1611258209cd4ce30bb76debf7256a733f3d6/spec/lib/marginalia_spec.rb#L69 . This test is strange. It mocks using the
Ci::ApplicationRecord
but then wants to query theusers
table. This probably needs to be changed to query an actualy CI table and probably doesn't require mocking anyconnection
Edited by Dylan Griffith
- Resolved by Etienne Baqué
I also see another false positive from a test using
connection
directly when it should be using the correct connection for the table https://gitlab.com/gitlab-org/gitlab/-/blob/19c1611258209cd4ce30bb76debf7256a733f3d6/spec/lib/gitlab/usage/service_ping_report_spec.rb#L115 . I think that again this is only a problem in the test.
- Resolved by Etienne Baqué
- Resolved by Etienne Baqué
- Resolved by Etienne Baqué
added 164 commits
-
ce1de434...724d5a5f - 163 commits from branch
master
- e1fe3a53 - Activate GitlabSchemaValidateConnection for all tests
-
ce1de434...724d5a5f - 163 commits from branch
added 59 commits
-
e1fe3a53...52776a2a - 58 commits from branch
master
- abea034a - Activate GitlabSchemaValidateConnection for all tests
-
e1fe3a53...52776a2a - 58 commits from branch
added 34 commits
-
abea034a...d7b152e9 - 33 commits from branch
master
- b079be89 - Activate GitlabSchemaValidateConnection for all tests
-
abea034a...d7b152e9 - 33 commits from branch
added 3271 commits
-
b079be89...a55a8bd1 - 3270 commits from branch
master
- bd60022f - Activate GitlabSchemaValidateConnection for all tests
-
b079be89...a55a8bd1 - 3270 commits from branch
- A deleted user
added pipeline:run-as-if-foss label
added 124 commits
-
bd60022f...9eab817d - 123 commits from branch
master
- 725697d4 - Activate GitlabSchemaValidateConnection for all tests
-
bd60022f...9eab817d - 123 commits from branch
added 1 commit
- eb8bf050 - Activate GitlabSchemaValidateConnection for all tests
changed milestone to %15.3
added typefeature label
added 123 commits
-
eb8bf050...e5feef7e - 122 commits from branch
master
- 012a80a1 - Activate GitlabSchemaValidateConnection for all tests
-
eb8bf050...e5feef7e - 122 commits from branch
added 34 commits
-
012a80a1...ec6b1ccb - 33 commits from branch
master
- cb01bf9c - Activate GitlabSchemaValidateConnection for all tests
-
012a80a1...ec6b1ccb - 33 commits from branch
- Resolved by Etienne Baqué
@rzwambag can you please do a backend review for this MR?
@dgruzd can you please do a database review for this MR?
please don't feel intimated by the number of changed files, many of them are just suppressing the newly activated analyzer on the old database migrations
Edited by Omar Qunsul
- Resolved by Michał Wielich
@michold can you please do a product intelligence review for the MR? apparently it's because of the changed file
spec/lib/gitlab/usage/service_ping_report_spec.rb
requested review from @rzwambag
requested review from @dgruzd
requested review from @michold
- Resolved by Omar Qunsul
- Resolved by Omar Qunsul
- Resolved by Omar Qunsul
removed review request for @dgruzd
added 283 commits
-
cb01bf9c...574fbc80 - 282 commits from branch
master
- 7bd7dbe9 - Activate GitlabSchemaValidateConnection for all tests
-
cb01bf9c...574fbc80 - 282 commits from branch
- Resolved by Etienne Baqué
@dgruzd I have applied the changes you suggested. Thank you for taking the time. I will add you as a reviewer again
requested review from @dgruzd
added databasereviewed label and removed databasereview pending label
@dgruzd
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.
For more info, please refer to the following links:
removed review request for @rzwambag
requested review from @ebaque
added 46 commits
-
7bd7dbe9...4e4bf163 - 45 commits from branch
master
- 6bbb2058 - Activate GitlabSchemaValidateConnection for all tests
-
7bd7dbe9...4e4bf163 - 45 commits from branch
added 33 commits
-
6bbb2058...e1ec0db2 - 32 commits from branch
master
- 1a14760f - Activate GitlabSchemaValidateConnection for all tests
-
6bbb2058...e1ec0db2 - 32 commits from branch
added databaseapproved label and removed databasereviewed label
removed review request for @tigerwnz
removed review request for @michold
enabled an automatic merge when the pipeline for 99ba5128 succeeds
mentioned in commit 7116b1f0
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
mentioned in issue #368848 (closed)
mentioned in merge request !92333 (merged)
added workflowpost-deploy-db-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
added releasedcandidate label
added releasedpublished label and removed releasedcandidate label
mentioned in issue #362345 (closed)