Add feature to delete inactive projects
What does this MR do and why?
-
This MR currently targets !86211 (merged), once that MR is merged toThe dependent MR has been merged and this MR now targetsmaster
we can update the base branch tomaster
for this MR as well.master
. - Add inactive projects deletion feature.
- Add a cronjob that runs once daily, finds the inactive projects and perform the following action:
- Send deletion warning email to the inactive project's maintainers and owners. Store the date when the email was sent in redis hash.
- If we've already sent an email then it checks whether the project is still inactive for a duration greater than
inactive_projects_delete_after_months
defined in theapplication_settings
table. In case the project is still inactive it permanently deletes the project. - When an activity is performed on an inactive project after the deletion warning email was sent, we remove that project's key from the redis hash.
- The documentation is added in a separate MR !86907 (merged)
Screenshots or screen recordings
HTML version | Plain text |
---|---|
![]() |
![]() |
Instance audit events | Project audit events |
---|---|
![]() |
![]() |
![]() |
How to set up and validate locally
- Create a project under a group A. Let's call it as Project A.
- Create another project in group B. Let's call it Project B.
- Modify the
last_activity_at
of both the projects so that they are considered as inactive. For example:
project = Project.find(<id of Project A>)
project.last_activity_at = 3.years.ago
project.save!
- Enable feature flag for group A only.
Feature.enable(:inactive_projects_deletion, Namespace.without_project_namespaces.find_by_full_path('<name of Group A>'))
- Enable inactive projects deletion settings using the application_settings API. Example :
https://gdk.test:3000/api/v4/application/settings?delete_inactive_projects=true&inactive_projects_send_warning_email_after_months=6&inactive_projects_delete_after_months=10&inactive_projects_min_size_mb=0
- Connect to your local redis using CLI -
redis-cli -s <path-to-gitlab-development-kit>/redis/redis.socket
. Verify that redis hash is empty by runninghgetall inactive_projects_deletion_warning_email_notified
inside the redis CLI. - Manually modify
1_settings.rb
file and update theinactive_projects_deletion_cron_worker
cron timing to run every minute. This will help us test the feature fast. Restart the background jobs usinggdk restart rails-background-jobs
- Settings.cron_jobs['inactive_projects_deletion_cron_worker']['cron'] ||= '0 1 * * *'
- Settings.cron_jobs['inactive_projects_deletion_cron_worker']['cron'] ||= '* * * * *'
- Wait for
Projects::InactiveProjectsDeletionCronWorker
to run. You can tail the logsgdk tail rails-background-jobs | grep -i inactive
. - After the cron has executed once, visit
https://gdk.test:3000/rails/letter_opener/
and verify that we've received the deletion warning email for Project A. Also verify that no email should have been received for Project B (we didn't enable the feature flag for this project). - Verify that the redis key has been updated. Run
hgetall inactive_projects_deletion_warning_email_notified
in redis-cli and verify that the key for project A exists. The value should be the current date which signifies the date when the deletion warning email was sent. - Manually update the redis value to simulate that the deletion warning email was sent more than (
inactive_projects_delete_after_months - inactive_projects_send_warning_email_after_months
) months ago (4 months as per our example). Run the following to update via redis-clihset inactive_projects_deletion_warning_email_notified project:<project_if of Project A> 2021-04-25
- Wait for
Projects::InactiveProjectsDeletionCronWorker
to run again. This time the project should get deleted immediately or should get marked for deletion if the project had enabled delayed project deletion. - Visit the homepage of Project A and verify that the project is either deleted or marked for deletion.
- Visit the homepage of Project B and verify that the project is neither deleted nor marked for deletion.
- Verify that the redis key has been deleted.
hgetall inactive_projects_deletion_warning_email_notified
- Visit the instance audit events dashboard and verify that audit events for deletion warning emails are captured successfully.
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.
Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/357376
Merge request reports
Activity
changed milestone to %15.0
added backend devopsmanage featureaddition groupcompliance sectiondev typefeature labels
assigned to @huzaifaiftikhar1
Suggested Reviewers (beta)
This is an experimental ML-based code reviewer recommendation system created by ~"group::applied ml".
The individuals below may be good candidates to participate in the review based on various factors.
After you review all recommendations, please assign reviewers manually, as this is not done automatically.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Reviewers @rspeicher
,@DouweM
,@kushalpandya
,@rymai
,@smcgivern
If you do not believe these recommendations are useful or if you do not want to use any of the suggestions, 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
Edited by GitLab Reviewer-Recommender Bot- Resolved by Michael Kozono
@mwoolf Can you please take an initial look into this MR? This is still in draft and I need to add RSpecs and documentation changes. Would you help in validating that there are no design flaws?
Edited by Huzaifa Iftikhar
requested review from @mwoolf
5 Warnings This MR changes code in ee/
, but its Changelog commit is missing theEE: true
trailer. Consider adding it to your Changelog commits.This merge request is quite big (790 lines changed), please consider splitting it into multiple merge requests. c55d158c: 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. 604d0faa: 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. featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
If needed, you can retry the
danger-review
job that generated this comment.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 Tarun Vellishetty ( @tvellishetty
) (UTC+5.5, same timezone as@huzaifaiftikhar1
)Luke Duncalfe ( @.luke
) (UTC+12, 6.5 hours ahead of@huzaifaiftikhar1
)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.
Sidekiq queue changes
This merge request contains changes to Sidekiq queues. Please follow the documentation on changing a queue's urgency.
These queues were added:
cronjob:projects_inactive_projects_deletion_cron
projects_inactive_projects_deletion_notification
Generated by
Danger- Resolved by Huzaifa Iftikhar
Allure report
allure-report-publisher
generated test report!review-qa-blocking:
test reportreview-qa-blocking:
test report for 17569f44expand test summary
+-------------------------------------------------------------------+ | suites summary | +----------------------+--------+--------+---------+-------+--------+ | | passed | failed | skipped | flaky | result | +----------------------+--------+--------+---------+-------+--------+ | Create | 24 | 0 | 2 | 22 | ❗ | | Manage | 31 | 0 | 2 | 32 | ❗ | | Plan | 41 | 0 | 1 | 41 | ❗ | | Verify | 12 | 0 | 1 | 12 | ❗ | | Configure | 0 | 0 | 1 | 0 | ➖ | | Protect | 2 | 0 | 0 | 2 | ❗ | | Version sanity check | 0 | 0 | 1 | 0 | ➖ | | Package | 0 | 0 | 1 | 0 | ➖ | +----------------------+--------+--------+---------+-------+--------+ | Total | 110 | 0 | 9 | 109 | ❗ | +----------------------+--------+--------+---------+-------+--------+
package-and-qa-ff-enabled:
test report for 17569f44expand test summary
+-------------------------------------------------------------------+ | suites summary | +----------------------+--------+--------+---------+-------+--------+ | | passed | failed | skipped | flaky | result | +----------------------+--------+--------+---------+-------+--------+ | Manage | 91 | 0 | 6 | 0 | ✅ | | Verify | 33 | 0 | 8 | 5 | ❗ | | Create | 143 | 6 | 6 | 11 | ❌ | | Secure | 13 | 0 | 6 | 7 | ❗ | | Package | 0 | 0 | 3 | 0 | ➖ | | Plan | 52 | 1 | 0 | 2 | ❌ | | Release | 6 | 0 | 0 | 0 | ✅ | | Protect | 2 | 0 | 0 | 0 | ✅ | | Fulfillment | 2 | 0 | 10 | 0 | ✅ | | Non-devops | 2 | 0 | 0 | 0 | ✅ | | Configure | 0 | 0 | 3 | 0 | ➖ | | Version sanity check | 0 | 0 | 1 | 0 | ➖ | +----------------------+--------+--------+---------+-------+--------+ | Total | 344 | 7 | 43 | 25 | ❌ | +----------------------+--------+--------+---------+-------+--------+
package-and-qa-ff-disabled:
test report for 17569f44expand test summary
+-------------------------------------------------------------------+ | suites summary | +----------------------+--------+--------+---------+-------+--------+ | | passed | failed | skipped | flaky | result | +----------------------+--------+--------+---------+-------+--------+ | Create | 144 | 5 | 6 | 11 | ❌ | | Verify | 33 | 0 | 8 | 5 | ❗ | | Manage | 91 | 0 | 6 | 0 | ✅ | | Secure | 13 | 0 | 6 | 7 | ❗ | | Non-devops | 2 | 0 | 0 | 0 | ✅ | | Plan | 52 | 1 | 0 | 2 | ❌ | | Release | 6 | 0 | 0 | 0 | ✅ | | Configure | 0 | 0 | 3 | 0 | ➖ | | Package | 0 | 0 | 3 | 0 | ➖ | | Fulfillment | 2 | 0 | 10 | 0 | ✅ | | Version sanity check | 0 | 0 | 1 | 0 | ➖ | | Protect | 2 | 0 | 0 | 0 | ✅ | +----------------------+--------+--------+---------+-------+--------+ | Total | 345 | 6 | 43 | 25 | ❌ | +----------------------+--------+--------+---------+-------+--------+
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
removed review request for @mwoolf
added 533 commits
-
1ef46cb6...d9c59dea - 532 commits from branch
master
- 07ddaef1 - Add inactive projects deletion feature
-
1ef46cb6...d9c59dea - 532 commits from branch
- Resolved by Robert Hunt
added 254 commits
-
bc84c9b1...19b1ab4d - 253 commits from branch
master
- f1acf38a - Add inactive projects deletion feature
-
bc84c9b1...19b1ab4d - 253 commits from branch
added 333 commits
-
f1acf38a...0f5db9e0 - 332 commits from branch
master
- c20942f3 - Add inactive projects deletion feature
-
f1acf38a...0f5db9e0 - 332 commits from branch
- The
gitlab-qa-mirror
downstream pipeline for !85689 (c20942f3) passed. - The
gitlab-qa-mirror
downstream pipeline for !85689 (c20942f3) passed. - The
gitlab-qa-mirror
downstream pipeline for !85689 (b8cf25a5) failed! - The
gitlab-qa-mirror
downstream pipeline for !85689 (b8cf25a5) passed. - The
gitlab-qa-mirror
downstream pipeline for !85689 (17569f44) failed! - The
gitlab-qa-mirror
downstream pipeline for !85689 (17569f44) failed! - The
gitlab-qa-mirror
downstream pipeline for !85689 (17569f44) failed! - The
gitlab-qa-mirror
downstream pipeline for !85689 (17569f44) failed! - The
gitlab-qa-mirror
downstream pipeline for !85689 (17569f44) failed! - The
gitlab-qa-mirror
downstream pipeline for !85689 (17569f44) failed!
- The
- Resolved by Huzaifa Iftikhar
added 197 commits
-
c20942f3...3b0114ee - 196 commits from branch
master
- 5faeeedf - Add inactive projects deletion feature
-
c20942f3...3b0114ee - 196 commits from branch
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
added 96 commits
-
8f601018...e3a60158 - 93 commits from branch
master
- 10eab92e - Add deletion warning email template for inactive projects
- 44ae2ce2 - Make the date of deletion bold in the deletion warning email
- 3a296f61 - Add inactive projects deletion feature
Toggle commit list-
8f601018...e3a60158 - 93 commits from branch
requested review from @mwoolf
- Resolved by Nick Malcolm
- Resolved by Nick Malcolm
- Resolved by Huzaifa Iftikhar
@huzaifaiftikhar1 FYI you can access the local redis session using
gdk redis-cli
Ok, I tested the feature locally and it works exactly as expected. Nice work @huzaifaiftikhar1
I'll start reviewing the code itself now!- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
removed review request for @mwoolf
added 852 commits
-
3a296f61...8c1b15be - 848 commits from branch
356805_add_inactive_projection_deletion_warning_email_template
- 79da7736 - Add inactive projects deletion feature
- 0d10e057 - Remove the feature flag from `inactive?` method in project.rb
- c1e82605 - Update RSpecs to use `to_date` method to make the examples descriptive
- cd0c653b - Add audit event for sending deletion warning email to inactive projects
Toggle commit list-
3a296f61...8c1b15be - 848 commits from branch
requested review from @mwoolf
added 4 commits
- be08aaf0 - Remove the feature flag from `inactive?` method in project.rb
- 8125999a - Update RSpecs to use `to_date` method to make the examples descriptive
- 52ec2288 - Add audit event for sending deletion warning email to inactive projects
- db92450c - Use the first active administrator user for inactive projects deletion
Toggle commit list@mwoolf
, 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:
added 1 commit
- ba26c1c4 - Remove default_enabled as per the new guideline
added 358 commits
-
ba26c1c4...82dd0471 - 352 commits from branch
master
- a6d2bf5e - Add inactive projects deletion feature
- 844ee8d3 - Remove the feature flag from `inactive?` method in project.rb
- 8410fec9 - Update RSpecs to use `to_date` method to make the examples descriptive
- bc673922 - Add audit event for sending deletion warning email to inactive projects
- 5636f3d4 - Use the first active administrator user for inactive projects deletion
- b8cf25a5 - Remove default_enabled as per the new guideline
Toggle commit list-
ba26c1c4...82dd0471 - 352 commits from branch
mentioned in merge request !86907 (merged)
- Resolved by Huzaifa Iftikhar
- Resolved by Michael Kozono
- Resolved by Michael Kozono
- Resolved by Michael Kozono
- Resolved by Michael Kozono
- Resolved by Michael Kozono
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
- Resolved by Michael Kozono
- Resolved by Huzaifa Iftikhar
- Resolved by Huzaifa Iftikhar
- Resolved by Michael Kozono
- Resolved by Michael Kozono
@huzaifaiftikhar1 Thanks! There is a lot to consider on this feature. I've added some comments/questions.
removed review request for @mkozono
added 759 commits
-
b8cf25a5...e9caf579 - 752 commits from branch
master
- 0a7455d6 - Add inactive projects deletion feature
- 604d0faa - Remove the feature flag from `inactive?` method in project.rb
- efa7196b - Update RSpecs to use `to_date` method to make the examples descriptive
- c55d158c - Add audit event for sending deletion warning email to inactive projects
- 0ed0329f - Use the first active administrator user for inactive projects deletion
- b1a6d8bf - Remove default_enabled as per the new guideline
- 17569f44 - Add InactiveProjectsDeletionWarningTracker and minor refactoring
Toggle commit list-
b8cf25a5...e9caf579 - 752 commits from branch
requested review from @mkozono
@mkozono I see the undercoverage job is failing, however, the lines it shows as uncovered do have a coverage.
I also verified the same by running the following locally
SIMPLECOV=true bundle exec rspec ee/spec/models/project_spec.rb ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb
We can also see that previous pipelines were successful and after that no additional code changes related to those files have been made. I would suggest we add pipeline:skip-undercoverage label before setting MWPS.
Edited by Huzaifa Iftikharadded pipeline:skip-undercoverage label
enabled an automatic merge when the pipeline for 981292d7 succeeds
mentioned in commit cc1fa6f3
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
added releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!1040 (merged)
mentioned in merge request !88751 (merged)
mentioned in merge request !104946 (merged)