Gitlab housekeeper initial implementation with finalize BBM
What does this MR do and why?
This is an initial implementation of the problem and solutions described in this blueprint !134487 (merged) and it was extracted from a POC implementation in !134485 (closed) which contained other work we don't want to include in the first iteration.
This is a gem which can be run locally or in CI to do static and dynamic analysis of the codebase and using a list of predefined "keeps" it will automatically create merge requests for things that developers would have otherwise needed to remember to do themselves.
It is analogous to a mix of rubocop -a
and GitLab Dependency Bot.
The word "keep" is used to describe a specific rule to apply to the code to match a required change and actually edit the code. The word "keep" was chosen as it sounds like "cop" and is very similar to implementing a rubocop rule as well as code to autocorrect the rule.
Today we have a complete implementation of the OverdueFinalizeBackgroundMigration
keep which finds all batched background migrations added before the last required stop upgrade point and then checks in Postgres.ai to confirm that this migration is complete on GitLab.com and then generates a migration to finalize the batched background migration. It has generated many migration MRs already.
How the code is organized
The code is organized in a very similar way to RuboCop in that we have a overall gem called gitlab-housekeeper
that contains the generic logic of looping over all keeps
(analogous to Cops) which are rules for how to detect changes that can be made to the code and then actually how to correct them.
This MR includes the following high level pieces:
-
gems/gitlab-housekeeper
which is the generic gem. It should be responsible for runningkeeps
which will return aChange
object. The housekeeper then uses this change object to create a git branch, push the branch, create a merge request- The
gitlab-housekeeper
command which just calls theRunner
- The
Runner
which contains the high level operation of thegitlab-housekeeper
gem. It looks at the options provided and runs the correct keeps. It then takes theChange
results from the keeps and creates merge requests - The
Git
client is just a wrapper around shelling out to git - The
GitlabClient
is just a thin wrapper aroundHTTParty
to make HTTP requests to create merge requests - The
Shell
class is just a wrapper around shelling out. Mainly this is useful for stubbing in specs for all the other code.
- The
- The
keeps/
directory which is analogous to ourrubocop
directory. It contains implementations of specific keeps and also ahelpers
directory that we expect to be used in multiple keeps-
keeps/helpers/postgres_ai.rb
is a wrapper around thePG
client that we can use to encapsulate code that looks up things from the current state of our production database. For now it is used to check if a batched background migration is finished in production -
keeps/overdue_finalize_background_migration.rb
: This is our first keep we've implemented. It has been used successfully to generate at least 15 merged migrations already. This is a pretty complicated piece of code but at a high level it is looking fordb/docs/batched_background_migrations
files and trying to find ones that were added before the last required stop and have already finished on GitLab.com . This means it's safe to finalize them. It then generates a migration with the change necessary to finalize the BBM.
-
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
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
assigned to @DylanGriffith
- A deleted user
added backend label
6 Warnings This merge request is quite big (1344 lines changed), please consider splitting it into multiple merge requests. 622a8768: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 924b0167: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. d2b56172: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. d2b56172: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. d957423b: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 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 @wandering_person
(UTC+7, 4 hours behind author)
@dstull
(UTC-5, 16 hours behind author)
Please check reviewer's status!
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.
Rubygems
This merge request adds, or changes a Rubygems dependency. Please review the Gemfile guidelines.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@DylanGriffith - please see the following guidance and update this merge request.1 Error Please add typebug typefeature, or typemaintenance label to this merge request. Edited by 🤖 GitLab Bot 🤖
added maintenanceworkflow label
added typemaintenance label
added grouptenant scale label
added devopsdata stores sectioncore platform labels
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for 6cc6e7cdexpand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Govern | 67 | 0 | 0 | 0 | 67 | ✅ | | Create | 54 | 0 | 7 | 0 | 61 | ✅ | | Verify | 31 | 0 | 0 | 0 | 31 | ✅ | | Plan | 55 | 0 | 0 | 0 | 55 | ✅ | | Data Stores | 23 | 0 | 0 | 0 | 23 | ✅ | | Package | 5 | 0 | 1 | 0 | 6 | ✅ | | Monitor | 8 | 0 | 0 | 0 | 8 | ✅ | | Release | 5 | 0 | 0 | 0 | 5 | ✅ | | Analytics | 2 | 0 | 0 | 0 | 2 | ✅ | | Framework sanity | 0 | 0 | 1 | 0 | 1 | ➖ | | Manage | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 250 | 0 | 10 | 0 | 260 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for 6cc6e7cdexpand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Plan | 249 | 0 | 10 | 3 | 259 | ✅ | | Create | 551 | 0 | 67 | 8 | 618 | ✅ | | Data Stores | 119 | 0 | 1 | 0 | 120 | ✅ | | GitLab Metrics | 2 | 0 | 1 | 0 | 3 | ✅ | | Monitor | 39 | 0 | 7 | 0 | 46 | ✅ | | Analytics | 7 | 0 | 0 | 0 | 7 | ✅ | | Fulfillment | 8 | 0 | 69 | 0 | 77 | ✅ | | Package | 226 | 0 | 17 | 0 | 243 | ✅ | | Govern | 298 | 0 | 20 | 4 | 318 | ✅ | | Systems | 8 | 0 | 0 | 0 | 8 | ✅ | | Manage | 40 | 0 | 10 | 1 | 50 | ✅ | | Secure | 6 | 0 | 3 | 0 | 9 | ✅ | | Verify | 138 | 0 | 27 | 6 | 165 | ✅ | | Release | 15 | 0 | 3 | 0 | 18 | ✅ | | Configure | 1 | 0 | 9 | 0 | 10 | ✅ | | ModelOps | 0 | 0 | 6 | 0 | 6 | ➖ | | Framework sanity | 0 | 0 | 5 | 0 | 5 | ➖ | | Growth | 0 | 0 | 6 | 0 | 6 | ➖ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 1707 | 0 | 261 | 22 | 1968 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+
mentioned in merge request !134485 (closed)
changed milestone to %16.8
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
assigned to @tigerwnz
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Tiger Watson
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
@tkuah @splattael would you mind reviewing this merge request.
Both of you have some familiarity with what we're trying to do here and have contributed some of the code.
I realise this is a really large merge request and we've already extracted some parts from the original POC to cut this down to the minimal of what we need.
This is probably not in a totally polished condition now but there are many things we still need to add to this tool in the next few weeks for Cells and I don't want the scope of the Gem to keep growing so large that reviewing it later on is even more challenging.
Please reach out to me or @tigerwnz if you want to have a sync code review of this work considering it might be a little overwhelming to review.
requested review from @tkuah and @splattael
mentioned in commit tigerwnz/gitlab@412993ee
mentioned in merge request tigerwnz/gitlab!1 (closed)
mentioned in commit tigerwnz/gitlab@0be40370
mentioned in merge request tigerwnz/gitlab!2 (closed)
mentioned in commit tigerwnz/gitlab@17ec9785
mentioned in merge request tigerwnz/gitlab!3 (closed)
mentioned in commit tigerwnz/gitlab@da62f7eb