From 1e0ac289f4415b8e3b1fbb01102367be259bfd2d Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Thu, 12 Dec 2024 11:39:18 +0000
Subject: [PATCH] Refactoring and add notes for further changes

---
 controlpanel/api/helm.py            |  3 +++
 controlpanel/api/models/tool.py     |  2 ++
 controlpanel/frontend/views/tool.py | 40 +++--------------------------
 3 files changed, 8 insertions(+), 37 deletions(-)

diff --git a/controlpanel/api/helm.py b/controlpanel/api/helm.py
index e6b704961..e91512a0f 100644
--- a/controlpanel/api/helm.py
+++ b/controlpanel/api/helm.py
@@ -158,6 +158,7 @@ def update_helm_repository(force=False):
             _execute("repo", "update", timeout=None)  # timeout = infinity.
 
 
+# TODO no longer used, remove
 def get_default_image_tag_from_helm_chart(chart_url, chart_name):
     proc = _execute("show", "values", chart_url)
     if proc:
@@ -171,6 +172,8 @@ def get_default_image_tag_from_helm_chart(chart_url, chart_name):
         return None
 
 
+# TODO this is no longer called from the Your Tools page
+# consider removing as part of further refactoring
 def get_helm_entries():
     # Update repository metadata.
     update_helm_repository()
diff --git a/controlpanel/api/models/tool.py b/controlpanel/api/models/tool.py
index 20ab02032..e347bdb00 100644
--- a/controlpanel/api/models/tool.py
+++ b/controlpanel/api/models/tool.py
@@ -73,6 +73,8 @@ def url(self, user):
     def save(self, *args, **kwargs):
         helm.update_helm_repository(force=True)
 
+        # TODO description is now required when creating a release, so this is unlikely to be called
+        # Consider removing
         if not self.description:
             self.description = helm.get_chart_app_version(self.chart_name, self.version) or ""
 
diff --git a/controlpanel/frontend/views/tool.py b/controlpanel/frontend/views/tool.py
index f636bd462..091c16ddf 100644
--- a/controlpanel/frontend/views/tool.py
+++ b/controlpanel/frontend/views/tool.py
@@ -13,11 +13,6 @@
 
 # First-party/Local
 from controlpanel.api import cluster
-from controlpanel.api.helm import (
-    get_chart_version_info,
-    get_default_image_tag_from_helm_chart,
-    get_helm_entries,
-)
 from controlpanel.api.models import Tool, ToolDeployment
 from controlpanel.oidc import OIDCLoginRequiredMixin
 from controlpanel.utils import start_background_task
@@ -85,10 +80,6 @@ def _add_new_item_to_tool_box(self, user, tool_box, tool, tools_info):
                 "deployment": None,
                 "releases": {},
             }
-        # TODO We should update model to always store an image tag
-        # image_tag = tool.image_tag
-        # if not image_tag:
-        #     image_tag = charts_info.get(tool.version, {}) or "unknown"
         if tool.id not in tools_info[tool_box]["releases"]:
             tools_info[tool_box]["releases"][tool.id] = {
                 "tool_id": tool.id,
@@ -110,6 +101,7 @@ def _add_deployed_charts_info(self, tools_info, user, id_token):
         # Get list of deployed tools
         # TODO this sets what tool the user currently has deployed. If we were to refactor to store
         # deployed tools in the database, we could remove a lot of this logic
+        # See https://github.com/ministryofjustice/analytical-platform/issues/6266
         deployments = cluster.ToolDeployment.get_deployments(user, id_token)
         for deployment in deployments:
             chart_name, chart_version = deployment.metadata.labels["chart"].rsplit("-", 1)
@@ -142,7 +134,8 @@ def _add_deployed_charts_info(self, tools_info, user, id_token):
             }
 
     def _retrieve_detail_tool_info(self, user, tools):
-        # TODO why do we need this? We could change so that all information required about available tools comes from the DB  # noqa: E501
+        # TODO when deployed tools are tracked in the DB this will not be needed
+        # see https://github.com/ministryofjustice/analytical-platform/issues/6266 # noqa: E501
         tools_info = {}
         for tool in tools:
             # Work out which bucket the chart should be in
@@ -152,31 +145,6 @@ def _retrieve_detail_tool_info(self, user, tools):
                 self._add_new_item_to_tool_box(user, tool_box, tool, tools_info)
         return tools_info
 
-    def _get_charts_info(self, tool_list):
-        # We may need the default image_tag from helm chart
-        # unless we configure it specifically in parameters of tool release
-        # TODO if we make sure that we always have an image_tag for a tool, then building charts_info is redundant and could be removed  # noqa: E501
-        charts_info = {}
-        chart_entries = None
-        for tool in tool_list:
-            if tool.version in charts_info:
-                continue
-
-            image_tag = tool.image_tag
-            if image_tag:
-                continue
-
-            if chart_entries is None:
-                # load the potential massive helm chart yaml only it is necessary
-                chart_entries = get_helm_entries()
-            chart_app_version = get_chart_version_info(chart_entries, tool.chart_name, tool.version)
-            if chart_app_version:
-                image_tag = get_default_image_tag_from_helm_chart(
-                    chart_app_version.chart_url, tool.chart_name
-                )
-            charts_info[tool.version] = image_tag
-        return charts_info
-
     def get_context_data(self, *args, **kwargs):
         """
         Retrieve information about tools and arrange them for the
@@ -238,8 +206,6 @@ def get_context_data(self, *args, **kwargs):
         context["managed_airflow_dev_url"] = f"{settings.AWS_SERVICE_URL}/?{args_airflow_dev_url}"
         context["managed_airflow_prod_url"] = f"{settings.AWS_SERVICE_URL}/?{args_airflow_prod_url}"
 
-        # Arrange tools information
-        # charts_info = self._get_charts_info(context["tools"])
         tools_info = self._retrieve_detail_tool_info(user, context["tools"])
 
         if "vscode" in tools_info: