Save child record for replicable model conditionally
The following discussion from !69301 (merged) should be addressed:
-
@cat started a discussion: (+2 comments) question: testing this on my local GDK, before enabling any feature flag etc, I do have:
gitlabhq_development=# select count(*) from merge_request_diff_details; count ------- 71 (1 row)Calling
::Geo::VerificationStateBackfillWorker.new.perform(Geo::MergeRequestDiffReplicator.replicable_name)will attempt to remove all of them:(byebug) for_creation_ids [] (byebug) verification_details_ids [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71]I presume that's because we only look at external diffs, right and we remove not-verifiable ones? I believe this is expected, but wanted to confirm (as I'm not sure why I had the previous records)
Michael Kozono
🔶 @mkozono · 13 hours ago MaintainerI think the existing records were created because of https://gitlab.com/gitlab-org/gitlab/-/blob/7c4cc67ddf6b16298c9ccd2a75570f312af0aee2/ee/app/models/ee/merge_request_diff.rb#L16. I think autosave: true conflicts with our intent in this change since it will insert merge_request_diff_details rows even if the merge_request_diffs row is not in available_verifiables.
My first thought is to remove autosave: true, but this is not a trivial change.
🤔 Given that this is feature flagged, maybe it's best to move forward with this MR scoped to just the new worker.
The next MR can remove the existing autosave behavior and fix things that break because of it. WDYT?