From a3d5f08dfba93298fdbef017b566587bb562dc7e Mon Sep 17 00:00:00 2001
From: "Aldo \"xoen\" Giambelluca" <aldo.giambelluca@digital.justice.gov.uk>
Date: Thu, 25 Jun 2020 15:07:20 +0100
Subject: [PATCH 1/4] Tools Deploy/Upgrade honour version in Tool record

Before this change the Control Panel was a bit "unpredictable" regarding
the version of a tool the user would get:
helm upgrade/install would not specify the version and the latest
version would be installed.

This was not too bad but as we improve the Tools management part of
the Control Panel this needs to be main more specific.

This is preparation work so that when a user tries to upgrade a tool
and we say "This will upgrade it to version x, y, z, OK?" and the user
confirms they don't then get a different version.

Related to ticket: https://trello.com/c/jHI3iCHq
---
 controlpanel/api/cluster.py                 | 3 +--
 controlpanel/api/models/tool.py             | 3 +--
 controlpanel/frontend/consumers.py          | 9 +++++----
 controlpanel/frontend/jinja2/tool-list.html | 3 ++-
 controlpanel/frontend/views/tool.py         | 3 ++-
 tests/api/models/test_tool.py               | 2 +-
 6 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/controlpanel/api/cluster.py b/controlpanel/api/cluster.py
index 6961e43d3..c40110859 100644
--- a/controlpanel/api/cluster.py
+++ b/controlpanel/api/cluster.py
@@ -289,7 +289,7 @@ def install(self, **kwargs):
             return helm.upgrade_release(
                 self.release_name,
                 f'{settings.HELM_REPO}/{self.chart_name}',  # XXX assumes repo name
-                # f'--version', tool.version,
+                f'--version', self.tool.version,
                 f'--namespace', self.k8s_namespace,
                 *set_values,
             )
@@ -310,7 +310,6 @@ def restart(self, id_token):
             self.k8s_namespace,
             label_selector=(
                 f"app={self.chart_name}"
-                # f'-{tool_deployment.tool.version}'
             ),
         )
 
diff --git a/controlpanel/api/models/tool.py b/controlpanel/api/models/tool.py
index c9a4be989..bcb49122b 100644
--- a/controlpanel/api/models/tool.py
+++ b/controlpanel/api/models/tool.py
@@ -55,10 +55,9 @@ def filter(self, **kwargs):
         filter = Q(chart_name=None)  # Always False
         deployments = cluster.ToolDeployment.get_deployments(user, id_token)
         for deployment in deployments:
-            chart_name, version = deployment.metadata.labels["chart"].rsplit("-", 1)
+            chart_name, _ = deployment.metadata.labels["chart"].rsplit("-", 1)
             filter = filter | (
                 Q(chart_name=chart_name)
-                # & Q(version=version)
             )
 
         tools = Tool.objects.filter(filter)
diff --git a/controlpanel/frontend/consumers.py b/controlpanel/frontend/consumers.py
index 7cfb59fb3..dd5f7ae55 100644
--- a/controlpanel/frontend/consumers.py
+++ b/controlpanel/frontend/consumers.py
@@ -159,10 +159,11 @@ def tool_restart(self, message):
             log.debug(f"Restarted {tool.name} for {user}")
 
     def get_tool_and_user(self, message):
-        tool = Tool.objects.get(
-            chart_name=message['tool_name'],
-            # version=message['version'],
-        )
+        tool_args = { "chart_name": message["tool_name"] }
+        if "version" in message:
+            tool_args["version"] = message["version"]
+
+        tool = Tool.objects.get(**tool_args)
         user = User.objects.get(auth0_id=message['user_id'])
         return tool, user
 
diff --git a/controlpanel/frontend/jinja2/tool-list.html b/controlpanel/frontend/jinja2/tool-list.html
index 1636e6f06..f0cb96fa9 100644
--- a/controlpanel/frontend/jinja2/tool-list.html
+++ b/controlpanel/frontend/jinja2/tool-list.html
@@ -56,7 +56,7 @@ <h1 class="govuk-heading-xl">Your tools</h1>
               data-action-name="deploy"
               method="post">
           {{ csrf_input }}
-          {# <input type="hidden" value="{{ tool.version }}" name="version"> #}
+          <input type="hidden" value="{{ tool.version }}" name="version">
           <button class="govuk-button govuk-button--secondary right">Deploy</button>
         </form>
         {% if deployment and deployment.outdated(id_token) %}
@@ -65,6 +65,7 @@ <h1 class="govuk-heading-xl">Your tools</h1>
                 data-action-name="upgrade"
                 method="post">
             {{ csrf_input }}
+            <input type="hidden" value="{{ tool.version }}" name="version">
             <button class="govuk-button govuk-button--secondary right">Upgrade</button>
           </form>
         {% endif %}
diff --git a/controlpanel/frontend/views/tool.py b/controlpanel/frontend/views/tool.py
index 1887756a0..3fe96315b 100644
--- a/controlpanel/frontend/views/tool.py
+++ b/controlpanel/frontend/views/tool.py
@@ -53,7 +53,7 @@ def get_redirect_url(self, *args, **kwargs):
 
         start_background_task('tool.deploy', {
             'tool_name': name,
-            # 'version': self.request.POST['version'],
+            'version': self.request.POST['version'],
             'user_id': self.request.user.id,
             'id_token': self.request.user.get_id_token(),
         })
@@ -73,6 +73,7 @@ def get_redirect_url(self, *args, **kwargs):
 
         start_background_task('tool.upgrade', {
             'tool_name': name,
+            'version': self.request.POST['version'],
             'user_id': self.request.user.id,
             'id_token': self.request.user.get_id_token(),
         })
diff --git a/tests/api/models/test_tool.py b/tests/api/models/test_tool.py
index a6f74c426..1e1b9f699 100644
--- a/tests/api/models/test_tool.py
+++ b/tests/api/models/test_tool.py
@@ -41,7 +41,7 @@ def test_deploy_for_generic(helm, token_hex, tool, users):
     helm.upgrade_release.assert_called_with(
         f'{tool.chart_name}-{user.slug}',
         f'mojanalytics/{tool.chart_name}',
-        # '--version', tool.version,
+        '--version', tool.version,
         '--namespace', user.k8s_namespace,
         '--set', f'username={user.username}',
         '--set', f'Username={user.username}',

From a899b1a223ac629432346fc2c975e942739ca673 Mon Sep 17 00:00:00 2001
From: "Aldo \"xoen\" Giambelluca" <aldo.giambelluca@digital.justice.gov.uk>
Date: Thu, 25 Jun 2020 15:20:50 +0100
Subject: [PATCH 2/4] Formatted some of the Python code with black

We currently don't format all the python code automatically but we
think it may be a good idea to do it more and more.

Eventually we'll get to a point where all the code has the same coding
standards (although some of the formatting decisions may make the code
slightly less readable, e.g. see `test_tool.py` file in this commit)

NOTE: Keeping these formatting changes in separate commits for the
moment to make it explicit in Git history. Would help when investigating
bugs with with Git bisect/blame and reduce the time spent getting to the
real commit which may have caused it and the reasons why these changes
were made.

Keeping these formatting changes with other changes may confusing for
the person investigating.
---
 controlpanel/api/cluster.py         | 80 +++++++++++++----------------
 controlpanel/api/models/tool.py     | 15 +++---
 controlpanel/frontend/consumers.py  | 51 ++++++++----------
 controlpanel/frontend/views/tool.py | 68 +++++++++++++-----------
 tests/api/models/test_tool.py       | 61 ++++++++++------------
 5 files changed, 125 insertions(+), 150 deletions(-)

diff --git a/controlpanel/api/cluster.py b/controlpanel/api/cluster.py
index c40110859..c96be847f 100644
--- a/controlpanel/api/cluster.py
+++ b/controlpanel/api/cluster.py
@@ -15,14 +15,14 @@
 log = logging.getLogger(__name__)
 
 
-TOOL_DEPLOYING = 'Deploying'
-TOOL_DEPLOY_FAILED = 'Failed'
-TOOL_IDLED = 'Idled'
-TOOL_NOT_DEPLOYED = 'Not deployed'
-TOOL_READY = 'Ready'
-TOOL_RESTARTING = 'Restarting'
-TOOL_UPGRADED = 'Upgraded'
-TOOL_STATUS_UNKNOWN = 'Unknown'
+TOOL_DEPLOYING = "Deploying"
+TOOL_DEPLOY_FAILED = "Failed"
+TOOL_IDLED = "Idled"
+TOOL_NOT_DEPLOYED = "Not deployed"
+TOOL_READY = "Ready"
+TOOL_RESTARTING = "Restarting"
+TOOL_UPGRADED = "Upgraded"
+TOOL_STATUS_UNKNOWN = "Unknown"
 
 
 class User:
@@ -31,13 +31,14 @@ class User:
 
     A user is represented by an IAM role, which is assumed by their tools.
     """
+
     def __init__(self, user):
         self.user = user
-        self.k8s_namespace = f'user-{self.user.slug}'
+        self.k8s_namespace = f"user-{self.user.slug}"
 
     @property
     def iam_role_name(self):
-        return f'{settings.ENV}_user_{self.user.username.lower()}'
+        return f"{settings.ENV}_user_{self.user.username.lower()}"
 
     def create(self):
         aws.create_user_role(self.user)
@@ -45,7 +46,8 @@ def create(self):
         helm.upgrade_release(
             f"init-user-{self.user.slug}",
             f"{settings.HELM_REPO}/init-user",
-            f"--set=" + (
+            f"--set="
+            + (
                 f"Env={settings.ENV},"
                 f"NFSHostname={settings.NFS_HOSTNAME},"
                 f"OidcDomain={settings.OIDC_DOMAIN},"
@@ -107,8 +109,7 @@ def url(self):
 
         repo_name = github_repository_name(self.app.repo_url)
         ingresses = k8s.ExtensionsV1beta1Api.list_namespaced_ingress(
-            self.APPS_NS,
-            label_selector=f"repo={repo_name}",
+            self.APPS_NS, label_selector=f"repo={repo_name}",
         ).items
 
         if len(ingresses) != 1:
@@ -119,6 +120,7 @@ def url(self):
 
 class S3Bucket:
     """Wraps a S3Bucket model to provide convenience methods for AWS"""
+
     def __init__(self, bucket):
         self.bucket = bucket
 
@@ -141,6 +143,7 @@ class RoleGroup:
     This is because IAM doesn't allow adding roles to IAM groups
     See https://stackoverflow.com/a/48087433/455642
     """
+
     def __init__(self, iam_managed_policy):
         self.policy = iam_managed_policy
 
@@ -150,18 +153,16 @@ def arn(self):
 
     @property
     def path(self):
-        return f'/{settings.ENV}/group/'
+        return f"/{settings.ENV}/group/"
 
     def create(self):
         aws.create_group(
-            self.policy.name,
-            self.policy.path,
+            self.policy.name, self.policy.path,
         )
 
     def update_members(self):
         aws.update_group_members(
-            self.arn,
-            {user.iam_role_name for user in self.policy.users.all()},
+            self.arn, {user.iam_role_name for user in self.policy.users.all()},
         )
 
     def delete(self):
@@ -194,7 +195,7 @@ def get_repositories(user):
             org = github.get_organization(name)
             repos.extend(org.get_repos())
         except GithubException as err:
-            log.warning(f'Failed getting {name} Github org repos for {user}: {err}')
+            log.warning(f"Failed getting {name} Github org repos for {user}: {err}")
             raise err
     return repos
 
@@ -204,7 +205,7 @@ def get_repository(user, repo_name):
     try:
         return github.get_repo(repo_name)
     except GithubException.UnknownObjectException:
-        log.warning(f'Failed getting {repo_name} Github repo for {user}: {err}')
+        log.warning(f"Failed getting {repo_name} Github repo for {user}: {err}")
         return None
 
 
@@ -212,14 +213,13 @@ class ToolDeploymentError(Exception):
     pass
 
 
-class ToolDeployment():
-
+class ToolDeployment:
     def __init__(self, user, tool):
         self.user = user
         self.tool = tool
 
     def __repr__(self):
-        return f'<ToolDeployment: {self.tool!r} {self.user!r}>'
+        return f"<ToolDeployment: {self.tool!r} {self.user!r}>"
 
     @property
     def chart_name(self):
@@ -252,7 +252,6 @@ def _delete_legacy_release(self):
         if old_release_name in helm.list_releases(old_release_name):
             helm.delete(True, old_release_name)
 
-
     def _set_values(self, **kwargs):
         """
         Return the list of `--set KEY=VALUE` helm upgrade arguments
@@ -275,8 +274,8 @@ def _set_values(self, **kwargs):
         values.update(kwargs)
         set_values = []
         for key, val in values.items():
-            escaped_val = val.replace(',', '\,')
-            set_values.extend(['--set', f'{key}={escaped_val}'])
+            escaped_val = val.replace(",", "\,")
+            set_values.extend(["--set", f"{key}={escaped_val}"])
 
         return set_values
 
@@ -288,9 +287,11 @@ def install(self, **kwargs):
 
             return helm.upgrade_release(
                 self.release_name,
-                f'{settings.HELM_REPO}/{self.chart_name}',  # XXX assumes repo name
-                f'--version', self.tool.version,
-                f'--namespace', self.k8s_namespace,
+                f"{settings.HELM_REPO}/{self.chart_name}",  # XXX assumes repo name
+                f"--version",
+                self.tool.version,
+                f"--namespace",
+                self.k8s_namespace,
                 *set_values,
             )
 
@@ -300,17 +301,13 @@ def install(self, **kwargs):
     def uninstall(self, id_token):
         deployment = self.get_deployment(id_token)
         helm.delete(
-            deployment.metadata.name,
-            f"--namespace={self.k8s_namespace}",
+            deployment.metadata.name, f"--namespace={self.k8s_namespace}",
         )
 
     def restart(self, id_token):
         k8s = KubernetesClient(id_token=id_token)
         return k8s.AppsV1Api.delete_collection_namespaced_replica_set(
-            self.k8s_namespace,
-            label_selector=(
-                f"app={self.chart_name}"
-            ),
+            self.k8s_namespace, label_selector=(f"app={self.chart_name}"),
         )
 
     @classmethod
@@ -345,7 +342,6 @@ def get_deployment(self, id_token):
 
         return deployments[0]
 
-
     def get_installed_chart_version(self, id_token):
         """
         Returns the installed helm chart version of the tool
@@ -361,7 +357,6 @@ def get_installed_chart_version(self, id_token):
         except ObjectDoesNotExist:
             return None
 
-
     def get_status(self, id_token):
         try:
             deployment = self.get_deployment(id_token)
@@ -375,8 +370,7 @@ def get_status(self, id_token):
             return TOOL_STATUS_UNKNOWN
 
         conditions = {
-            condition.type: condition
-            for condition in deployment.status.conditions
+            condition.type: condition for condition in deployment.status.conditions
         }
 
         if "Available" in conditions:
@@ -385,14 +379,12 @@ def get_status(self, id_token):
                     return TOOL_IDLED
                 return TOOL_READY
 
-        if 'Progressing' in conditions:
-            progressing_status = conditions['Progressing'].status
+        if "Progressing" in conditions:
+            progressing_status = conditions["Progressing"].status
             if progressing_status == "True":
                 return TOOL_DEPLOYING
             elif progressing_status == "False":
                 return TOOL_DEPLOY_FAILED
 
-        log.warning(
-            f"Unknown status for {self!r}: {deployment.status.conditions}"
-        )
+        log.warning(f"Unknown status for {self!r}: {deployment.status.conditions}")
         return TOOL_STATUS_UNKNOWN
diff --git a/controlpanel/api/models/tool.py b/controlpanel/api/models/tool.py
index bcb49122b..7785e804e 100644
--- a/controlpanel/api/models/tool.py
+++ b/controlpanel/api/models/tool.py
@@ -30,13 +30,13 @@ class Tool(TimeStampedModel):
 
     class Meta(TimeStampedModel.Meta):
         db_table = "control_panel_api_tool"
-        ordering = ('name',)
+        ordering = ("name",)
 
     def __repr__(self):
-        return f'<Tool: {self.chart_name} {self.version}>'
+        return f"<Tool: {self.chart_name} {self.version}>"
 
     def url(self, user):
-        return f'https://{user.slug}-{self.chart_name}.{settings.TOOLS_DOMAIN}/'
+        return f"https://{user.slug}-{self.chart_name}.{settings.TOOLS_DOMAIN}/"
 
 
 class ToolDeploymentManager:
@@ -56,9 +56,7 @@ def filter(self, **kwargs):
         deployments = cluster.ToolDeployment.get_deployments(user, id_token)
         for deployment in deployments:
             chart_name, _ = deployment.metadata.labels["chart"].rsplit("-", 1)
-            filter = filter | (
-                Q(chart_name=chart_name)
-            )
+            filter = filter | (Q(chart_name=chart_name))
 
         tools = Tool.objects.filter(filter)
         results = []
@@ -85,7 +83,7 @@ def __init__(self, tool, user):
         self.user = user
 
     def __repr__(self):
-        return f'<ToolDeployment: {self.tool!r} {self.user!r}>'
+        return f"<ToolDeployment: {self.tool!r} {self.user!r}>"
 
     def get_installed_app_version(self, id_token):
         """
@@ -119,7 +117,6 @@ def get_installed_app_version(self, id_token):
 
         return None
 
-
     def outdated(self, id_token):
         """
         Returns true if the tool helm chart version is old
@@ -146,7 +143,7 @@ def delete(self, id_token):
 
     @property
     def host(self):
-        return f'{self.user.slug}-{self.tool.chart_name}.{settings.TOOLS_DOMAIN}'
+        return f"{self.user.slug}-{self.tool.chart_name}.{settings.TOOLS_DOMAIN}"
 
     def save(self, *args, **kwargs):
         """
diff --git a/controlpanel/frontend/consumers.py b/controlpanel/frontend/consumers.py
index dd5f7ae55..14e44b98a 100644
--- a/controlpanel/frontend/consumers.py
+++ b/controlpanel/frontend/consumers.py
@@ -44,20 +44,22 @@ async def handle(self, body):
         """
         self.streaming = True
 
-        user = self.scope.get('user')
+        user = self.scope.get("user")
         if not user or not user.is_authenticated:
-            await self.send_response(403, b'Forbidden')
+            await self.send_response(403, b"Forbidden")
             return
 
-        await self.send_headers(headers=[
-            (b"Cache-Control", b"no-cache"),
-            (b"Content-Type", b"text/event-stream"),
-            (b"Transfer-Encoding", b"chunked"),
-        ])
+        await self.send_headers(
+            headers=[
+                (b"Cache-Control", b"no-cache"),
+                (b"Content-Type", b"text/event-stream"),
+                (b"Transfer-Encoding", b"chunked"),
+            ]
+        )
 
         # headers are not sent until some part of the body is sent, so send an
         # empty string to force them
-        await self.send_body(b'', more_body=True)
+        await self.send_body(b"", more_body=True)
 
         # schedule a coroutine to send keepalive updates
         asyncio.get_event_loop().create_task(self.stream())
@@ -71,10 +73,9 @@ async def stream(self):
         Send a keepalive message every minute to prevent timeouts
         """
         while self.streaming:
-            await self.sse_event({
-                "event": "keepalive",
-                "data": datetime.now().isoformat(),
-            })
+            await self.sse_event(
+                {"event": "keepalive", "data": datetime.now().isoformat(),}
+            )
             await asyncio.sleep(60)
 
     async def sse_event(self, event):
@@ -85,8 +86,7 @@ async def sse_event(self, event):
         payload = "\n".join(f"{key}: {value}" for key, value in event.items())
 
         await self.send_body(
-            f"{payload}\n\n".encode("utf-8"),
-            more_body=self.streaming,
+            f"{payload}\n\n".encode("utf-8"), more_body=self.streaming,
         )
 
     async def disconnect(self):
@@ -101,7 +101,6 @@ async def disconnect(self):
 
 
 class BackgroundTaskConsumer(SyncConsumer):
-
     def tool_deploy(self, message):
         """
         Deploy the named tool for the specified user
@@ -159,12 +158,12 @@ def tool_restart(self, message):
             log.debug(f"Restarted {tool.name} for {user}")
 
     def get_tool_and_user(self, message):
-        tool_args = { "chart_name": message["tool_name"] }
+        tool_args = {"chart_name": message["tool_name"]}
         if "version" in message:
             tool_args["version"] = message["version"]
 
         tool = Tool.objects.get(**tool_args)
-        user = User.objects.get(auth0_id=message['user_id'])
+        user = User.objects.get(auth0_id=message["user_id"])
         return tool, user
 
 
@@ -173,11 +172,7 @@ def send_sse(user_id, event):
     Tell the SSEConsumer to send an event to the specified user
     """
     async_to_sync(channel_layer.group_send)(
-        sanitize_dns_label(user_id),
-        {
-            "type": "sse.event",
-            **event
-        },
+        sanitize_dns_label(user_id), {"type": "sse.event", **event},
     )
 
 
@@ -193,21 +188,15 @@ def update_tool_status(tool_deployment, id_token, status):
         "appVersion": app_version,
         "status": status,
     }
-    send_sse(user.auth0_id, {
-        "event": "toolStatus",
-        "data": json.dumps(payload),
-    })
+    send_sse(user.auth0_id, {"event": "toolStatus", "data": json.dumps(payload),})
 
 
 def start_background_task(task, message):
     async_to_sync(channel_layer.send)(
-        'background_tasks',
-        {
-            'type': task,
-            **message,
-        },
+        "background_tasks", {"type": task, **message,},
     )
 
+
 def wait_for_deployment(tool_deployment, id_token):
     status = TOOL_DEPLOYING
     while status == TOOL_DEPLOYING:
diff --git a/controlpanel/frontend/views/tool.py b/controlpanel/frontend/views/tool.py
index 3fe96315b..9882f9941 100644
--- a/controlpanel/frontend/views/tool.py
+++ b/controlpanel/frontend/views/tool.py
@@ -20,19 +20,16 @@
 
 
 class ToolList(LoginRequiredMixin, PermissionRequiredMixin, ListView):
-    context_object_name = 'tools'
+    context_object_name = "tools"
     model = Tool
-    permission_required = 'api.list_tool'
+    permission_required = "api.list_tool"
     template_name = "tool-list.html"
 
     def get_context_data(self, *args, **kwargs):
         user = self.request.user
         id_token = user.get_id_token()
 
-        tool_deployments = ToolDeployment.objects.filter(
-            user=user,
-            id_token=id_token,
-        )
+        tool_deployments = ToolDeployment.objects.filter(user=user, id_token=id_token,)
 
         context = super().get_context_data(*args, **kwargs)
         context["id_token"] = id_token
@@ -45,18 +42,21 @@ def get_context_data(self, *args, **kwargs):
 
 
 class DeployTool(LoginRequiredMixin, RedirectView):
-    http_method_names = ['post']
+    http_method_names = ["post"]
     url = reverse_lazy("list-tools")
 
     def get_redirect_url(self, *args, **kwargs):
-        name = kwargs['name']
-
-        start_background_task('tool.deploy', {
-            'tool_name': name,
-            'version': self.request.POST['version'],
-            'user_id': self.request.user.id,
-            'id_token': self.request.user.get_id_token(),
-        })
+        name = kwargs["name"]
+
+        start_background_task(
+            "tool.deploy",
+            {
+                "tool_name": name,
+                "version": self.request.POST["version"],
+                "user_id": self.request.user.id,
+                "id_token": self.request.user.get_id_token(),
+            },
+        )
 
         messages.success(
             self.request, f"Deploying {name}... this may take several minutes",
@@ -65,18 +65,21 @@ def get_redirect_url(self, *args, **kwargs):
 
 
 class UpgradeTool(LoginRequiredMixin, RedirectView):
-    http_method_names = ['post']
+    http_method_names = ["post"]
     url = reverse_lazy("list-tools")
 
     def get_redirect_url(self, *args, **kwargs):
-        name = kwargs['name']
-
-        start_background_task('tool.upgrade', {
-            'tool_name': name,
-            'version': self.request.POST['version'],
-            'user_id': self.request.user.id,
-            'id_token': self.request.user.get_id_token(),
-        })
+        name = kwargs["name"]
+
+        start_background_task(
+            "tool.upgrade",
+            {
+                "tool_name": name,
+                "version": self.request.POST["version"],
+                "user_id": self.request.user.id,
+                "id_token": self.request.user.get_id_token(),
+            },
+        )
 
         messages.success(
             self.request, f"Upgrading {name}... this may take several minutes",
@@ -89,13 +92,16 @@ class RestartTool(LoginRequiredMixin, RedirectView):
     url = reverse_lazy("list-tools")
 
     def get_redirect_url(self, *args, **kwargs):
-        name = self.kwargs['name']
-
-        start_background_task('tool.restart', {
-            'tool_name': name,
-            'user_id': self.request.user.id,
-            'id_token': self.request.user.get_id_token(),
-        })
+        name = self.kwargs["name"]
+
+        start_background_task(
+            "tool.restart",
+            {
+                "tool_name": name,
+                "user_id": self.request.user.id,
+                "id_token": self.request.user.get_id_token(),
+            },
+        )
 
         messages.success(
             self.request, f"Restarting {name}...",
diff --git a/tests/api/models/test_tool.py b/tests/api/models/test_tool.py
index 1e1b9f699..935322552 100644
--- a/tests/api/models/test_tool.py
+++ b/tests/api/models/test_tool.py
@@ -9,23 +9,20 @@
 
 @pytest.fixture
 def tool(db):
-    return mommy.make('api.Tool')
+    return mommy.make("api.Tool")
 
 
 @pytest.yield_fixture
 def token_hex():
-    with patch('controlpanel.api.models.tool.secrets') as secrets:
+    with patch("controlpanel.api.models.tool.secrets") as secrets:
         yield secrets.token_hex
 
 
 def test_deploy_for_generic(helm, token_hex, tool, users):
-    cookie_secret_proxy = 'cookie secret proxy'
-    cookie_secret_tool = 'cookie secret tool'
-    token_hex.side_effect = [
-        cookie_secret_proxy,
-        cookie_secret_tool
-    ]
-    user = users['normal_user']
+    cookie_secret_proxy = "cookie secret proxy"
+    cookie_secret_tool = "cookie secret tool"
+    token_hex.side_effect = [cookie_secret_proxy, cookie_secret_tool]
+    user = users["normal_user"]
 
     # simulate release with old naming scheme installed
     old_release_name = f"{user.username}-{tool.chart_name}"
@@ -39,14 +36,20 @@ def test_deploy_for_generic(helm, token_hex, tool, users):
 
     # install new release
     helm.upgrade_release.assert_called_with(
-        f'{tool.chart_name}-{user.slug}',
-        f'mojanalytics/{tool.chart_name}',
-        '--version', tool.version,
-        '--namespace', user.k8s_namespace,
-        '--set', f'username={user.username}',
-        '--set', f'Username={user.username}',
-        '--set', f'aws.iamRole={user.iam_role_name}',
-        '--set', f'toolsDomain={settings.TOOLS_DOMAIN}',
+        f"{tool.chart_name}-{user.slug}",
+        f"mojanalytics/{tool.chart_name}",
+        "--version",
+        tool.version,
+        "--namespace",
+        user.k8s_namespace,
+        "--set",
+        f"username={user.username}",
+        "--set",
+        f"Username={user.username}",
+        "--set",
+        f"aws.iamRole={user.iam_role_name}",
+        "--set",
+        f"toolsDomain={settings.TOOLS_DOMAIN}",
     )
 
 
@@ -58,16 +61,8 @@ def cluster():
 
 @pytest.mark.parametrize(
     "chart_version, expected_outdated",
-    [
-        (None, False),
-        ("0.0.1", True),
-        ("1.0.0", False),
-    ],
-    ids=[
-        "no-chart-version",
-        "old-chart-version",
-        "up-to-date-chart-version",
-    ],
+    [(None, False), ("0.0.1", True), ("1.0.0", False),],
+    ids=["no-chart-version", "old-chart-version", "up-to-date-chart-version",],
 )
 def test_tool_deployment_outdated(cluster, chart_version, expected_outdated):
     tool = Tool(chart_name="test-tool", version="1.0.0")
@@ -83,8 +78,6 @@ def test_tool_deployment_outdated(cluster, chart_version, expected_outdated):
     cluster_td.get_installed_chart_version.assert_called_with(id_token)
 
 
-
-
 @pytest.mark.parametrize(
     "chart_version, expected_app_version",
     [
@@ -92,13 +85,11 @@ def test_tool_deployment_outdated(cluster, chart_version, expected_outdated):
         ("1.0.0", None),
         ("2.2.5", "RStudio: 1.2.1335+conda, R: 3.5.1, Python: 3.7.1, patch: 10"),
     ],
-    ids=[
-        "no-chart-installed",
-        "old-chart-version",
-        "new-chart-version",
-    ],
+    ids=["no-chart-installed", "old-chart-version", "new-chart-version",],
 )
-def test_tool_deployment_get_installed_app_version(helm_repository_index, cluster, chart_version, expected_app_version):
+def test_tool_deployment_get_installed_app_version(
+    helm_repository_index, cluster, chart_version, expected_app_version
+):
     tool = Tool(chart_name="rstudio")
     user = User(username="test-user")
     td = ToolDeployment(tool, user)

From 5327586bb9f3630853d214a6f32c5ea6f901f5ef Mon Sep 17 00:00:00 2001
From: "Aldo \"xoen\" Giambelluca" <aldo.giambelluca@digital.justice.gov.uk>
Date: Fri, 26 Jun 2020 10:39:26 +0100
Subject: [PATCH 3/4] Show app version on tool Deploy/Upgrade

When a user click on the Deploy or Upgrade button the Control Panel
now shows them the version of the tool they're about to install or
upgrade to.

This version is actually the `appVersion` information coming from
the helm chart, this should contain useful information, for example
for RStudio would be something like

> RStudio: 1.2.1335+conda, R: 3.5.1, Python: 3.7.1, patch: 10

This will give the user enough details to take an informed decision
on whether to install or more importantly upgrade.

NOTE: As part of this change I've moved the logic to get a tool/chart
`appVersion` into the `HelmRepository` class as it's now used in more
than one place and it makes sense for this to be in the class used to
query the helm repository.

Ticket: https://trello.com/c/jHI3iCHq
---
 controlpanel/api/helm.py                    | 19 ++++++++++++++
 controlpanel/api/models/tool.py             | 25 ++++++++++++++----
 controlpanel/frontend/jinja2/tool-list.html |  4 +--
 tests/api/models/test_tool.py               | 15 +++++++++++
 tests/api/test_helm.py                      | 28 +++++++++++++++++++++
 5 files changed, 84 insertions(+), 7 deletions(-)

diff --git a/controlpanel/api/helm.py b/controlpanel/api/helm.py
index 74de216e0..0225195ce 100644
--- a/controlpanel/api/helm.py
+++ b/controlpanel/api/helm.py
@@ -231,6 +231,25 @@ def get_chart_info(cls, name):
             chart_info[chart.version] = chart
         return chart_info
 
+    @classmethod
+    def get_chart_app_version(cls, name, version):
+        """
+        Returns the "appVersion" metadata for the given
+        chart name/version.
+
+        It returns None if the chart or the chart version
+        are not found or if that version of a chart doesn't
+        have the "appVersion" field (e.g. the chart
+        preceed the introduction of this field)
+        """
+
+        chart_info = cls.get_chart_info(name)
+        version_info = chart_info.get(version, None)
+        if version_info:
+            return version_info.app_version
+
+        return None
+
     @classmethod
     def _outdated(cls):
         # helm update never called?
diff --git a/controlpanel/api/models/tool.py b/controlpanel/api/models/tool.py
index 7785e804e..dcba6650b 100644
--- a/controlpanel/api/models/tool.py
+++ b/controlpanel/api/models/tool.py
@@ -38,6 +38,23 @@ def __repr__(self):
     def url(self, user):
         return f"https://{user.slug}-{self.chart_name}.{settings.TOOLS_DOMAIN}/"
 
+    @property
+    def app_version(self):
+        """
+        Returns the "appVersion" for this tool.
+
+        This is metadata in the helm chart which we use to maintain details
+        of the actual tool version (e.g. "RStudio: 1.2.1335+conda, R: 3.5.1, ...")
+        as opposed to the chart version.
+
+        Returns None if this information is not available for this tool and
+        chart version (e.g. the chart was released before the `appVersion`
+        was introduced) or because the chart doesn't exist in the helm
+        reporitory.
+        """
+
+        return HelmRepository.get_chart_app_version(self.chart_name, self.version)
+
 
 class ToolDeploymentManager:
     """
@@ -109,11 +126,9 @@ def get_installed_app_version(self, id_token):
         td = cluster.ToolDeployment(self.user, self.tool)
         chart_version = td.get_installed_chart_version(id_token)
         if chart_version:
-            chart_info = HelmRepository.get_chart_info(self.tool.chart_name)
-
-            version_info = chart_info.get(chart_version, None)
-            if version_info:
-                return version_info.app_version
+            return HelmRepository.get_chart_app_version(
+                self.tool.chart_name, chart_version
+            )
 
         return None
 
diff --git a/controlpanel/frontend/jinja2/tool-list.html b/controlpanel/frontend/jinja2/tool-list.html
index f0cb96fa9..0d9d79c3d 100644
--- a/controlpanel/frontend/jinja2/tool-list.html
+++ b/controlpanel/frontend/jinja2/tool-list.html
@@ -57,7 +57,7 @@ <h1 class="govuk-heading-xl">Your tools</h1>
               method="post">
           {{ csrf_input }}
           <input type="hidden" value="{{ tool.version }}" name="version">
-          <button class="govuk-button govuk-button--secondary right">Deploy</button>
+          <button class="govuk-button govuk-button--secondary right js-confirm" data-confirm-message="This will install {{ tool.name }} ({% if tool.app_version %}{{ tool.app_version }}{% else %}version information not available{% endif %}), do you wish to continue?">Deploy</button>
         </form>
         {% if deployment and deployment.outdated(id_token) %}
           <form action="{{ url('upgrade-tool', kwargs={"name": tool.chart_name}) }}"
@@ -66,7 +66,7 @@ <h1 class="govuk-heading-xl">Your tools</h1>
                 method="post">
             {{ csrf_input }}
             <input type="hidden" value="{{ tool.version }}" name="version">
-            <button class="govuk-button govuk-button--secondary right">Upgrade</button>
+            <button class="govuk-button govuk-button--secondary right js-confirm" data-confirm-message="This will change your {{ tool.name }} instance {% if tool.app_version %}to {{ tool.app_version }}{% else %}(version information not available){% endif %}, do you wish to continue?">Upgrade</button>
           </form>
         {% endif %}
       </td>
diff --git a/tests/api/models/test_tool.py b/tests/api/models/test_tool.py
index 935322552..c675dff76 100644
--- a/tests/api/models/test_tool.py
+++ b/tests/api/models/test_tool.py
@@ -101,3 +101,18 @@ def test_tool_deployment_get_installed_app_version(
     assert td.get_installed_app_version(id_token) == expected_app_version
     cluster.ToolDeployment.assert_called_with(user, tool)
     cluster_td.get_installed_chart_version.assert_called_with(id_token)
+
+
+@pytest.mark.parametrize(
+    "chart_version, expected_app_version",
+    [
+        ("unknown-version", None),
+        ("1.0.0", None),
+        ("2.2.5", "RStudio: 1.2.1335+conda, R: 3.5.1, Python: 3.7.1, patch: 10"),
+    ],
+    ids=["unknown-version", "chart-with-no-appVersion", "chart-with-appVersion",],
+)
+def test_tool_app_version(helm_repository_index, chart_version, expected_app_version):
+    tool = Tool(chart_name="rstudio", version=chart_version)
+
+    assert tool.app_version == expected_app_version
diff --git a/tests/api/test_helm.py b/tests/api/test_helm.py
index 516e756cc..fa2782e6d 100644
--- a/tests/api/test_helm.py
+++ b/tests/api/test_helm.py
@@ -66,6 +66,34 @@ def test_helm_repository_chart_info_when_chart_found(helm_repository_index):
         assert rstudio_info["1.0.0"].app_version == None
 
 
+@pytest.mark.parametrize(
+    "chart_name, version, expected_app_version",
+    [
+        ("notfound", "v42", None),
+        ("rstudio", "unknown-version", None),
+        ("rstudio", "1.0.0", None),
+        (
+            "rstudio",
+            "2.2.5",
+            "RStudio: 1.2.1335+conda, R: 3.5.1, Python: 3.7.1, patch: 10",
+        ),
+    ],
+    ids=[
+        "unknown-chart",
+        "unknown-version",
+        "chart-with-no-appVersion",
+        "chart-with-appVersion",
+    ],
+)
+def test_helm_repository_get_chart_app_version(
+    helm_repository_index, chart_name, version, expected_app_version
+):
+    # See tests/api/fixtures/helm_mojanalytics_index.py
+    with patch("controlpanel.api.helm.open", helm_repository_index):
+        app_version = HelmRepository.get_chart_app_version(chart_name, version)
+        assert app_version == expected_app_version
+
+
 def test_helm_upgrade_release():
     helm.__class__.execute = MagicMock()
 

From 0228d735b62d8d5e396cfb4701ef393a86950303 Mon Sep 17 00:00:00 2001
From: "Aldo \"xoen\" Giambelluca" <aldo.giambelluca@digital.justice.gov.uk>
Date: Fri, 26 Jun 2020 10:48:45 +0100
Subject: [PATCH 4/4] Formatted some more Python code with black

We currently don't format all the python code automatically but we
think it may be a good idea to do it more and more.

Eventually we'll get to a point where all the code has the same coding
standards (although some of the formatting decisions may make the code
slightly less readable, e.g. see `test_tool.py` file in this commit)

NOTE: Keeping these formatting changes in separate commits for the
moment to make it explicit in Git history. Would help when investigating
bugs with with Git bisect/blame and reduce the time spent getting to the
real commit which may have caused it and the reasons why these changes
were made.

Keeping these formatting changes with other changes may confusing for
the person investigating.
---
 controlpanel/api/helm.py | 64 +++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 34 deletions(-)

diff --git a/controlpanel/api/helm.py b/controlpanel/api/helm.py
index 0225195ce..ab289a390 100644
--- a/controlpanel/api/helm.py
+++ b/controlpanel/api/helm.py
@@ -19,25 +19,24 @@ class HelmError(APIException):
 
 
 class Helm(object):
-
     @classmethod
     def execute(cls, *args, check=True, **kwargs):
         should_wait = False
-        if 'timeout' in kwargs:
+        if "timeout" in kwargs:
             should_wait = True
-            timeout = kwargs.pop('timeout')
+            timeout = kwargs.pop("timeout")
 
         try:
-            log.debug(' '.join(['helm', *args]))
+            log.debug(" ".join(["helm", *args]))
             env = os.environ.copy()
             # helm checks for existence of DEBUG env var
-            if 'DEBUG' in env:
-                del env['DEBUG']
+            if "DEBUG" in env:
+                del env["DEBUG"]
             proc = subprocess.Popen(
                 ["helm", *args],
                 stderr=subprocess.PIPE,
                 stdout=subprocess.PIPE,
-                encoding='utf8',
+                encoding="utf8",
                 env=env,
                 **kwargs,
             )
@@ -92,47 +91,46 @@ def parse_upgrade_output(output):
     section = None
     columns = None
     last_deployed = None
-    namespace = ''
+    namespace = ""
     resource_type = None
     resources = {}
     notes = []
 
-    for line in output.split('\n'):
+    for line in output.split("\n"):
 
-        if line.startswith('LAST DEPLOYED:'):
+        if line.startswith("LAST DEPLOYED:"):
             last_deployed = datetime.strptime(
-                line.split(':', 1)[1],
-                ' %a %b %d %H:%M:%S %Y',
+                line.split(":", 1)[1], " %a %b %d %H:%M:%S %Y",
             )
             continue
 
-        if line.startswith('NAMESPACE:'):
-            namespace = line.split(':', 1)[1].strip()
+        if line.startswith("NAMESPACE:"):
+            namespace = line.split(":", 1)[1].strip()
             continue
 
-        if line.startswith('==> ') and section == 'RESOURCES':
-            resource_type = line.split(' ', 1)[1].strip()
+        if line.startswith("==> ") and section == "RESOURCES":
+            resource_type = line.split(" ", 1)[1].strip()
             continue
 
-        if line.startswith('RESOURCES:'):
-            section = 'RESOURCES'
+        if line.startswith("RESOURCES:"):
+            section = "RESOURCES"
             continue
 
-        if line.startswith('NAME') and resource_type:
+        if line.startswith("NAME") and resource_type:
             columns = line.lower()
-            columns = re.split(r'\s+', columns)
+            columns = re.split(r"\s+", columns)
             continue
 
-        if section == 'NOTES':
+        if section == "NOTES":
             notes.append(line)
             continue
 
-        if line.startswith('NOTES:'):
-            section = 'NOTES'
+        if line.startswith("NOTES:"):
+            section = "NOTES"
             continue
 
         if section and line.strip():
-            row = re.split(r'\s+', line)
+            row = re.split(r"\s+", line)
             row = dict(zip(columns, row))
             resources[resource_type] = [
                 *resources.get(resource_type, []),
@@ -140,15 +138,14 @@ def parse_upgrade_output(output):
             ]
 
     return {
-        'last_deployed': last_deployed,
-        'namespace': namespace,
-        'resources': resources,
-        'notes': '\n'.join(notes),
+        "last_deployed": last_deployed,
+        "namespace": namespace,
+        "resources": resources,
+        "notes": "\n".join(notes),
     }
 
 
 class Chart(object):
-
     def __init__(self, name, description, version, app_version):
         self.name = name
         self.description = description
@@ -162,10 +159,7 @@ class HelmRepository(object):
 
     HELM_HOME = Helm.execute("home").stdout.read().strip()
     REPO_PATH = os.path.join(
-        HELM_HOME,
-        "repository",
-        "cache",
-        f"{settings.HELM_REPO}-index.yaml",
+        HELM_HOME, "repository", "cache", f"{settings.HELM_REPO}-index.yaml",
     )
 
     _updated_at = None
@@ -186,7 +180,9 @@ def _load(cls):
                 cls._repository = yaml.load(f, Loader=yaml.FullLoader)
         except Exception as err:
             wrapped_err = HelmError(err)
-            wrapped_err.detail = f"Error while opening/parsing helm repository cache: '{cls.REPO_PATH}'"
+            wrapped_err.detail = (
+                f"Error while opening/parsing helm repository cache: '{cls.REPO_PATH}'"
+            )
             raise HelmError(wrapped_err)
 
     @classmethod