Skip to content
Snippets Groups Projects

Nullify merge_request_metrics pipeline_id on pipeline deletion

Merged Patrick Bajao requested to merge 26911-nullify-metrics-pipeline-lfk into master
All threads resolved!

What does this MR do and why?

Before, when a pipeline is deleted, the associated merge_request_metrics record will be deleted as well. This is a destructive thing as merge_request_metrics include other data not related to a pipeline.

To fix this issue, instead of deleting the association record, we nullify the pipeline_id on merge_request_metrics instead. When this happens, related build data (latest_build_started_at and latest_build_finished_at) will be nullified as well. This is to ensure that when those data are requested, it'll still behave the same way as before.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. Run migration.
  2. Create a MR with pipeline.
  3. Let the pipeline run.
  4. Merge the MR.
  5. Delete the pipeline.
  6. Wait for 2 minutes (this is to ensure the job to clean up loose foreign keys has already ran).
  7. Make a request to api/v4/projects/:project_id/merge_requests/:merge_request_iid and check if merged_at is not null.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Patrick Bajao

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • Resolved by Patrick Bair

      Hi @ahegyi, this is the solution I mentioned in #26911 (comment 895582245). What are your thoughts about it?

      I see that we have helpers in the codebase for adding DB triggers and functions so I used them. I don't see any guidelines about them though in the docs (I may be just missing it). Are there things that I need to consider (e.g. performance concerns) regarding triggers?

      Sorry for directly asking you and skipping database reviewer. But since you were already involved in the discussion on the related issue, I feel it'll be easier this way. :bow:

      P.S. This is still in draft as I'm still testing this further.

  • Patrick Bajao changed milestone to %15.0

    changed milestone to %15.0

  • Patrick Bajao requested review from @ahegyi

    requested review from @ahegyi

  • mentioned in issue #26911 (closed)

  • Patrick Bajao marked this merge request as ready

    marked this merge request as ready

  • Patrick Bajao added 1243 commits

    added 1243 commits

    Compare with previous version

  • 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
    @rkadam3, @agarciatesares, @mikolaj_wawrzyniak, @lmejia2, @ali-gitlab

    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
  • requested review from @huzaifaiftikhar1

  • Adam Hegyi approved this merge request

    approved this merge request

  • added databasereviewed label and removed databasereview pending label

  • :wave: @ahegyi, 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:

  • Adam Hegyi requested review from @pbair

    requested review from @pbair

  • Patrick Bair approved this merge request

    approved this merge request

  • added databaseapproved label and removed databasereviewed label

  • Patrick Bair removed review request for @pbair

    removed review request for @pbair

  • Huzaifa Iftikhar approved this merge request

    approved this merge request

  • Huzaifa Iftikhar requested review from @ebaque and removed review request for @huzaifaiftikhar1

    requested review from @ebaque and removed review request for @huzaifaiftikhar1

  • Etienne Baqué approved this merge request

    approved this merge request

  • Etienne Baqué resolved all threads

    resolved all threads

  • no schema change; no impact to data warehouse

  • Patrick Bajao added 366 commits

    added 366 commits

    Compare with previous version

  • Etienne Baqué resolved all threads

    resolved all threads

  • Etienne Baqué mentioned in commit 55fb1d31

    mentioned in commit 55fb1d31

  • added workflowstaging label and removed workflowcanary label

  • Adam Hegyi mentioned in issue #391920

    mentioned in issue #391920

  • Please register or sign in to reply
    Loading