Remove pulls trigger (#450)

* remove pulls trigger

* add test

* put cleanup method into cron job

* fix migration conflict

* remove the changes to should_write_to_storage

* make repository non-required for ArchiveService, add bulk delete to ArchiveService, remove default value from paths

Author: nora-shap

shared/api_archive/archive.py

@@ -48,7 +48,7 @@ class ArchiveService(object):
     In s3 terms, this would be the name of the bucket
     """
 
-    storage_hash: str
+    storage_hash: str | None
     """
     A hash key of the repo for internal storage
     """
@@ -64,7 +64,10 @@ def __init__(self, repository, ttl=None):
         self.ttl = ttl or int(get_config("services", "minio", "ttl", default=self.ttl))
 
         self.storage = StorageService()
-        self.storage_hash = self.get_archive_hash(repository)
+        if repository:
+            self.storage_hash = self.get_archive_hash(repository)
+        else:
+            self.storage_hash = None
 
     @classmethod
     def get_archive_hash(cls, repository) -> str:
@@ -98,6 +101,8 @@ def write_json_data_to_storage(
         *,
         encoder=ReportEncoder,
     ):
+        if not self.storage_hash:
+            raise ValueError("No hash key provided")
         if commit_id is None:
             # Some classes don't have a commit associated with them
             # For example Pull belongs to multiple commits.
@@ -147,20 +152,29 @@ def read_file(self, path: str) -> str:
         return contents.decode()
 
     @sentry_sdk.trace
-    def delete_file(self, path):
+    def delete_file(self, path: str) -> None:
         """
         Generic method to delete a file from the archive.
         """
         self.storage.delete_file(self.root, path)
 
+    @sentry_sdk.trace
+    def delete_files(self, paths: list[str]) -> None:
+        """
+        Generic method to delete files from the archive.
+        """
+        self.storage.delete_files(bucket_name=self.root, paths=paths)
+
     def read_chunks(self, commit_sha: str) -> str:
         """
         Convenience method to read a chunks file from the archive.
         """
+        if not self.storage_hash:
+            raise ValueError("No hash key provided")
         path = MinioEndpoints.chunks.get_path(
             version="v4", repo_hash=self.storage_hash, commitid=commit_sha
         )
         return self.read_file(path)
 
-    def create_presigned_put(self, path):
+    def create_presigned_put(self, path: str) -> str:
         return self.storage.create_presigned_put(self.root, path, self.ttl)

shared/celery_config.py

@@ -113,6 +113,7 @@
 brolly_stats_rollup_task_name = (
     f"app.cron.{TaskConfigGroup.daily.value}.BrollyStatsRollupTask"
 )
+flare_cleanup_task_name = f"app.cron.{TaskConfigGroup.daily.value}.FlareCleanupTask"
 
 
 def get_task_group(task_name: str) -> Optional[str]:
@@ -169,7 +170,7 @@ class BaseCeleryConfig(object):
     worker_prefetch_multiplier = int(
         get_config("setup", "tasks", "celery", "prefetch", default=1)
     )
-    # !!! NEVER 0 !!! 0 == infinate
+    # !!! NEVER 0 !!! 0 == infinite
 
     # http://celery.readthedocs.org/en/latest/configuration.html#celeryd-task-soft-time-limit
     task_soft_time_limit = int(

shared/django_apps/core/models.py

@@ -418,6 +418,11 @@ def external_id(self):
         return self.pullid
 
     def should_write_to_storage(self) -> bool:
+        """
+        This only applies to the flare field.
+        Flare is used to draw static graphs (see GraphHandler view in api) and can be large.
+        Flare cleanup is handled by FlareCleanupTask in worker.
+        """
         if self.repository is None or self.repository.author is None:
             return False
         is_codecov_repo = self.repository.author.username == "codecov"
@@ -430,7 +435,8 @@ def should_write_to_storage(self) -> bool:
     _flare = models.JSONField(db_column="flare", null=True)
     _flare_storage_path = models.URLField(db_column="flare_storage_path", null=True)
     flare = ArchiveField(
-        should_write_to_storage_fn=should_write_to_storage, default_value_class=dict
+        should_write_to_storage_fn=should_write_to_storage,
+        default_value_class=dict,
     )
 
     def save(self, *args, **kwargs):

shared/django_apps/legacy_migrations/migrations/0008_delete_pull_update_trigger.py

@@ -0,0 +1,21 @@
+# Generated by Django 4.2.16 on 2024-12-06 00:15
+
+from django.db import migrations
+
+from shared.django_apps.migration_utils import RiskyRunSQL
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("legacy_migrations", "0007_delete_repo_trigger_stats"),
+    ]
+
+    operations = [
+        RiskyRunSQL(
+            """
+            DROP TRIGGER IF EXISTS pulls_before_update_drop_flare ON pulls;
+            DROP FUNCTION IF EXISTS pulls_drop_flare();
+            """,
+            reverse_sql=migrations.RunSQL.noop,
+        )
+    ]

shared/django_apps/legacy_migrations/migrations/legacy_sql/main/triggers/main.py

@@ -1,11 +1,9 @@
 from .commits import run_sql as commits_run_sql
 from .owners import run_sql as owners_run_sql
-from .pulls import run_sql as pulls_run_sql
 from .repos import run_sql as repos_run_sql
 
 
 def run_sql(schema_editor):
     commits_run_sql(schema_editor)
     owners_run_sql(schema_editor)
     repos_run_sql(schema_editor)
-    pulls_run_sql(schema_editor)

shared/django_apps/legacy_migrations/migrations/legacy_sql/main/triggers/pulls.py

@@ -57,24 +57,6 @@ def run_sql(schema_editor):
         )
         execute procedure owner_yaml_updated();
 
-
-        drop trigger pulls_before_insert on pulls;
-        drop function pulls_insert();
-        drop trigger pulls_before_update on pulls;
-        drop function pulls_update();
-
-        create or replace function pulls_drop_flare() returns trigger as $$
-        begin
-            new.flare = null;
-            return new;
-        end;
-        $$ language plpgsql;
-
-        create trigger pulls_before_update_drop_flare before update on pulls
-        for each row
-        when (new.state != 'open'::pull_state)
-        execute procedure pulls_drop_flare();
-
         ---- Function Changes -----
         drop function if exists get_new_repos(int);
         drop function if exists get_pull(int, int);

shared/django_apps/legacy_migrations/migrations/legacy_sql/upgrades/v440.py

@@ -38,7 +38,7 @@ def get_commitid(self) -> Optional[str]:
 
 class ArchiveField:
     """This is a helper class that transparently handles models' fields that are saved in storage.
-    Classes that use the ArchiveField MUST implement ArchiveFieldInterface. It ill throw an error otherwise.
+    Classes that use the ArchiveField MUST implement ArchiveFieldInterface. It will throw an error otherwise.
     It uses the Descriptor pattern: https://docs.python.org/3/howto/descriptor.html
 
     Arguments:
@@ -58,7 +58,7 @@ class ArchiveField:
             rehydrate_fn=rehidrate_data,
             default_value='default'
         )
-    For a full example check utils/tests/unit/test_model_utils.py
+    For a full example check utils/tests/unit/test_model_utils.py in worker
     """
 
     def __init__(

shared/django_apps/utils/model_utils.py

@@ -147,7 +147,7 @@ def delete_file(self, bucket_name, path):
         except ClientError:
             raise
 
-    def delete_files(self, bucket_name, paths=[]):
+    def delete_files(self, bucket_name: str, paths: list[str]) -> list[bool]:
         """Batch deletes a list of files from a given bucket
 
         Note:

shared/storage/aws.py

@@ -89,7 +89,7 @@ def delete_file(self, bucket_name, path):
         second_deletion = self.fallback_service.delete_file(bucket_name, path)
         return first_deletion and second_deletion
 
-    def delete_files(self, bucket_name, paths=[]):
+    def delete_files(self, bucket_name: str, paths: list[str]) -> list[bool]:
         """Batch deletes a list of files from a given bucket
             (what happens to the files that don't exist?)