Remove repos.cache from branch_update trigger on branches

[joseph-sentry]

Purpose/Motivation

branch_update should no longer reference repos.cache since the cache column is being removed from the repos table.

Links to relevant tickets

codecov/engineering-team#92

What does this PR do?

• Create migration that runs SQL to drop branch_update trigger.

Notes to Reviewer

Depends on: #35

[codecov]

Codecov Report

Merging #53 (75edf57) into main (c3e59da) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##            main     #53     +/-   ##
=======================================
+ Coverage   95.30   95.31   +0.01     
=======================================
  Files        693     694      +1     
  Lines      14736   14741      +5     
=======================================
+ Hits       14044   14049      +5     
  Misses       692     692             

Flag	Coverage Δ
unit	95.25% <100.00%> (?)
unit-latest-uploader	95.25% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
core/migrations/0032_auto_20230731_1641.py	100.00% <100.00%> (ø)

[hootener]

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed	Coverage Δ
core/migrations/0032_auto_20230731_1641.py	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[scott-codecov]

There's some other stuff going on in the branches_update function and dropping the trigger will mean we no longer execute any of the other code. I'm not entirely sure if we rely on that timestamp updating logic but I think I'd be inclined to be a bit more conservative here and just remove the logic that pertains to the repos.cache column.

The current source of that SQL function is:

        create or replace function branches_update() returns trigger as $$
        declare _ownerid int;
        begin
            -- update repos cache if main branch
            update repos
            set updatestamp = now(),
                cache = update_json(cache::jsonb, 'commit', get_commit_minimum(new.repoid, new.head)::jsonb)
            where repoid = new.repoid
                and branch = new.branch
            returning ownerid into _ownerid;

            if found then
            -- default branch updated, so we can update the owners timestamp
            -- to refresh the team list
            update owners
            set updatestamp=now()
            where ownerid=_ownerid;
            end if;

            return null;
        end;
        $$ language plpgsql;

(from

codecov-api/legacy_migrations/migrations/legacy_sql/main/triggers/branches.py

Line 4 in 030c0ca

create or replace function branches_update() returns trigger as $$

)

We could replace that function w/ a new function that preserves the timestamp updating:

        create or replace function branches_update() returns trigger as $$
        declare _ownerid int;
        begin
            -- update repos if main branch
            update repos
            set updatestamp = now()
            where repoid = new.repoid
                and branch = new.branch
            returning ownerid into _ownerid;

            if found then
            -- default branch updated, so we can update the owners timestamp
            -- to refresh the team list
            update owners
            set updatestamp=now()
            where ownerid=_ownerid;
            end if;

            return null;
        end;
        $$ language plpgsql;

Alternatively - can we replicate that timestamp updating inside the application code (i.e. in the model layer somewhere)? I do think it would be preferable to drop the whole function + trigger if possible so we don't have to think about it going forward.

[joseph-sentry]

@scott-codecov I locally tested out updating a branch in a way that would normally activate the trigger after having dropped it and the timestamp was still being updated, which is actually frustrating because I was unable to find out what was updating it. I agree that the better solution would be to have this logic in the application code, but I also suspect that it's already there. I will look into it some more before I merge this change. If we want to get this change in soon I agree that we could just remove the reference to repos.cache in the trigger.