From c925e9991aab7ac7bb293a5cbce52a0e2afd5142 Mon Sep 17 00:00:00 2001 From: ymao2 Date: Fri, 2 Dec 2022 18:23:02 +0000 Subject: [PATCH] Solved the issues after merge --- controlpanel/api/cluster.py | 15 ---- controlpanel/api/helm.py | 68 +++++-------------- controlpanel/api/models/tool.py | 40 ----------- controlpanel/api/serializers.py | 5 +- controlpanel/api/views/tool_deployments.py | 3 +- controlpanel/frontend/consumers.py | 1 + controlpanel/frontend/jinja2/base.html | 2 +- .../frontend/jinja2/release-create.html | 3 +- .../frontend/jinja2/release-detail.html | 3 +- controlpanel/frontend/jinja2/tool-list.html | 18 +++-- .../static/javascripts/modules/tool-status.js | 6 +- controlpanel/frontend/views/tool.py | 38 +++++------ tests/api/cluster/test_tool_deployment.py | 21 ------ tests/api/models/test_tool.py | 24 ------- tests/api/test_helm.py | 21 ++---- tests/api/views/test_tool_deployments.py | 2 +- tests/frontend/test_consumers.py | 1 + 17 files changed, 70 insertions(+), 201 deletions(-) diff --git a/controlpanel/api/cluster.py b/controlpanel/api/cluster.py index d8b62782c..131a58a38 100644 --- a/controlpanel/api/cluster.py +++ b/controlpanel/api/cluster.py @@ -744,21 +744,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 - - This is extracted from the `chart` label in the corresponding - `Deployment`. - """ - - try: - deployment = self.get_deployment(id_token) - _, chart_version = deployment.metadata.labels["chart"].rsplit("-", 1) - return chart_version - except ObjectDoesNotExist: - return None - def get_status(self, id_token, deployment=None): try: if deployment is None: diff --git a/controlpanel/api/helm.py b/controlpanel/api/helm.py index 8e338c746..167071ad5 100644 --- a/controlpanel/api/helm.py +++ b/controlpanel/api/helm.py @@ -142,8 +142,20 @@ def get_default_image_tag_from_helm_chart(chart_url, chart_name): def get_helm_entries(): - # Update and grab repository metadata. - repository = update_helm_repository() + # Update repository metadata. + update_helm_repository() + # Grab repository metadata. + repo_path = get_repo_path() + try: + with open(repo_path) as f: + repository = yaml.load(f, Loader=yaml.FullLoader) + except Exception as ex: + error = HelmError(ex) + error.detail = ( + f"Error while opening/parsing helm repository cache: '{repo_path}'" + ) + raise HelmError(error) + if not repository: # Metadata not available, so bail with empty dictionary. return {} @@ -216,54 +228,6 @@ def delete(namespace, *args, dry_run=False): log.info(stdout) -def get_chart_info(chart_name): - """ - Get information about the given chart. - - Returns a dictionary with the chart versions as keys and instances of the - HelmChart class as associated values. - - Returns an empty dictionary when the chart is not in the helm repository - index. - """ - chart_info = {} # The end result. - # Update - update_helm_repository() - # Grab repository metadata. - repo_path = get_repo_path() - try: - with open(repo_path) as f: - repository = yaml.load(f, Loader=yaml.FullLoader) - except Exception as ex: - error = HelmError(ex) - error.detail = ( - f"Error while opening/parsing helm repository cache: '{repo_path}'" - ) - raise HelmError(error) - - if not repository: - # Metadata not available, so bail with empty dictionary. - return chart_info - entries = repository.get("entries") - if entries: - versions = entries.get(chart_name) - if versions: - # There are versions for the referenced chart. Add them to the - # result as HelmChart instances. - for version in versions: - chart = HelmChart( - version["name"], - version["description"], - version["version"], - # appVersion is relatively new so some old charts don't - # have it. - version.get("appVersion"), - version["urls"][0], - ) - chart_info[chart.version] = chart - return chart_info - - def get_chart_app_version(chart_name, chart_version): """ Returns the "appVersion" metadata for the helm chart with the referenced @@ -273,8 +237,8 @@ def get_chart_app_version(chart_name, chart_version): "appVersion" metadata. """ - chart = get_chart_info(chart_name) - chart_at_version = chart.get(chart_version) + entries = get_helm_entries() + chart_at_version = get_chart_version_info(entries, chart_name, chart_version) if chart_at_version: return chart_at_version.app_version else: diff --git a/controlpanel/api/models/tool.py b/controlpanel/api/models/tool.py index 04021f74f..8a3d26763 100644 --- a/controlpanel/api/models/tool.py +++ b/controlpanel/api/models/tool.py @@ -102,9 +102,6 @@ def image_tag(self): return values.get("{}.tag".format(chart_image_key_name)) or \ values.get("{}.image.tag".format(chart_image_key_name)) - def tool_release_tag(self, image_tag=None): - return "{}-{}-{}".format(self.chart_name, self.version, image_tag or self.image_tag) - class ToolDeploymentManager: """ @@ -137,43 +134,6 @@ def __init__(self, tool, user, old_chart_name=None): def __repr__(self): return f"" - def get_installed_chart_version(self, id_token): - """ - Returns the installed chart version for this tool - - Returns None if the chart is not installed for the user - """ - td = cluster.ToolDeployment(self.user, self.tool) - return td.get_installed_chart_version(id_token) - - def get_installed_app_version(self, id_token, chart_version=None): - """ - Returns the version of the deployed tool - - NOTE: This is the version coming from the helm - chart `appVersion` field, **not** the version - of the chart released in the user namespace. - - e.g. if user has `rstudio-2.2.5` (chart version) - installed in his namespace, this would return - "RStudio: 1.2.1335+conda, R: 3.5.1, Python: 3.7.1, patch: 10" - **not** "2.2.5". - - Also bear in mind that Helm added this `appVersion` - field only "recently" so if a user has an old - version of a tool chart installed this would return - `None` as we can't determine the tool version - as this information is simply not available - in the helm repository index. - """ - if not chart_version: - chart_version = self.get_installed_chart_version(id_token) - if chart_version: - return helm.get_chart_app_version( - self.tool.chart_name, chart_version - ) - return None - def delete(self, id_token): """ Remove the release from the cluster diff --git a/controlpanel/api/serializers.py b/controlpanel/api/serializers.py index b1c261153..468e590bf 100644 --- a/controlpanel/api/serializers.py +++ b/controlpanel/api/serializers.py @@ -274,9 +274,10 @@ class ToolDeploymentSerializer(serializers.Serializer): def validate_version(self, value): try: - _, _ = value.split("__") + _, _, _ = value.split("__") except ValueError: - raise serializers.ValidationError('This field include chart name and version and they are joined by "__".') + raise serializers.ValidationError('This field include chart name, version and tool.id,' + ' they are joined by "__".') class ESBucketHitsSerializer(serializers.BaseSerializer): diff --git a/controlpanel/api/views/tool_deployments.py b/controlpanel/api/views/tool_deployments.py index fcc8b6612..970d8dc8d 100644 --- a/controlpanel/api/views/tool_deployments.py +++ b/controlpanel/api/views/tool_deployments.py @@ -30,7 +30,7 @@ def _deploy(self, chart_name, data): # attribute and then split on "__" to extract them. Why? Because we # need both pieces of information to kick off the background helm # deploy. - tool_name, tool_version = chart_info.split("__") + tool_name, tool_version, tool_id = chart_info.split("__") # Kick off the helm chart as a background task. start_background_task( @@ -38,6 +38,7 @@ def _deploy(self, chart_name, data): { "tool_name": chart_name, "version": tool_version, + "tool_id": tool_id, "user_id": self.request.user.id, "id_token": self.request.user.get_id_token(), "old_chart_name": old_chart_name, diff --git a/controlpanel/frontend/consumers.py b/controlpanel/frontend/consumers.py index 45b032bfd..21900abda 100644 --- a/controlpanel/frontend/consumers.py +++ b/controlpanel/frontend/consumers.py @@ -197,6 +197,7 @@ def update_tool_status(tool_deployment, id_token, status): payload = { "toolName": tool.chart_name, "version": tool.version, + "tool_id": tool.id, "status": status, } send_sse(user.auth0_id, {"event": "toolStatus", "data": json.dumps(payload),}) diff --git a/controlpanel/frontend/jinja2/base.html b/controlpanel/frontend/jinja2/base.html index bc5e6348b..805087c65 100644 --- a/controlpanel/frontend/jinja2/base.html +++ b/controlpanel/frontend/jinja2/base.html @@ -196,7 +196,7 @@ - + diff --git a/controlpanel/frontend/jinja2/release-create.html b/controlpanel/frontend/jinja2/release-create.html index 848f7a2f7..64e97256a 100644 --- a/controlpanel/frontend/jinja2/release-create.html +++ b/controlpanel/frontend/jinja2/release-create.html @@ -66,8 +66,7 @@

{{ page_title }}

}, "name": "description", "attributes": { - "pattern": "[a-z0-9.-]{1,60}", - "maxlength": "60", + "maxlength": "100", }, "value": form.description.value(), "errorMessage": { "html": form.description.errors|join(". ") } if form.description.errors else {} diff --git a/controlpanel/frontend/jinja2/release-detail.html b/controlpanel/frontend/jinja2/release-detail.html index 89104b767..b6f9fea79 100644 --- a/controlpanel/frontend/jinja2/release-detail.html +++ b/controlpanel/frontend/jinja2/release-detail.html @@ -69,8 +69,7 @@

{{ page_title }}

}, "name": "description", "attributes": { - "pattern": "[a-z0-9.-]{1,60}", - "maxlength": "60", + "maxlength": "100", }, "value": form.description.value(), "errorMessage": { "html": form.description.errors|join(". ") } if form.description.errors else {} diff --git a/controlpanel/frontend/jinja2/tool-list.html b/controlpanel/frontend/jinja2/tool-list.html index 6ffc68553..9fa9d3283 100644 --- a/controlpanel/frontend/jinja2/tool-list.html +++ b/controlpanel/frontend/jinja2/tool-list.html @@ -38,9 +38,9 @@

{{ tool_info.name }}

id="tools-{{ chart_name }}" name="version"> {% set installed_chart_version = None %} {% set installed_release_version = None %} - {% if deployment %} - {% set installed_chart_version = deployment.app_version %} - {% set installed_release_version = deployment.tool_release_tag %} + {% if deployment and deployment.tool_id > -1 %} + {% set installed_chart_version = deployment.description %} + {% set installed_release_version = deployment.tool_id %} @@ -100,12 +100,22 @@

{{ tool_info.name }}

{{ csrf_input }} + +{% if deployment and deployment.tool_id == -1 %} +
+

+ Looks like your current deployment ({{ deployment.chart_name}}-{{ deployment.chart_version }}: {{ deployment.image_tag }}) + was not recognised within current maintained tool release versions, No worry you can still open it and have a look, + then feel free to switch one of versions from above dropdown list. +

+
+{% endif %}
{% endfor %} diff --git a/controlpanel/frontend/static/javascripts/modules/tool-status.js b/controlpanel/frontend/static/javascripts/modules/tool-status.js index 3c3f930a9..2121c4b35 100644 --- a/controlpanel/frontend/static/javascripts/modules/tool-status.js +++ b/controlpanel/frontend/static/javascripts/modules/tool-status.js @@ -39,7 +39,7 @@ moj.Modules.toolStatus = { const toolstatus = this; return event => { const data = JSON.parse(event.data); - if (data.toolName != listener.dataset.toolName) { + if (data.toolName.startsWith(listener.dataset.toolName) === false ) { return; } listener.querySelector(toolstatus.statusLabelClass).innerText = data.status; @@ -89,14 +89,14 @@ moj.Modules.toolStatus = { } // 3. add "(installed)" suffix and class to new version - let newValue = newVersionData.toolName + "__" + newVersionData.version; + let newValue = newVersionData.toolName + "__" + newVersionData.version + "__" + newVersionData.tool_id; let newVersionOption = listener.querySelector(this.versionSelector + " option[value='" + newValue + "']"); if (newVersionOption) { newVersionOption.innerText = newVersionOption.innerText + this.installedSuffix; newVersionOption.classList.add(this.versionInstalledClass) // set the new version as the current chosen item - const dropDownToolId = "tools-" + newVersionData.toolName; + const dropDownToolId = "tools-" + listener.dataset.toolName; document.getElementById(dropDownToolId).selectedIndex = newVersionOption.index; } } diff --git a/controlpanel/frontend/views/tool.py b/controlpanel/frontend/views/tool.py index 49d84edfd..99493adf8 100644 --- a/controlpanel/frontend/views/tool.py +++ b/controlpanel/frontend/views/tool.py @@ -76,10 +76,9 @@ def _add_new_item_to_tool_box(self, user, tool_box, tool, tools_info, charts_inf } image_tag = tool.image_tag if not image_tag: - image_tag = charts_info.get(tool.version, {}).get('image_tag') or 'unknown' - tool_release_tag = tool.tool_release_tag(image_tag=image_tag) - if tool_release_tag not in tools_info[tool_box]["releases"]: - tools_info[tool_box]["releases"][tool_release_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, "chart_name": tool.chart_name, "description": tool.description, @@ -104,15 +103,14 @@ def _add_deployed_charts_info(self, tools_info, user, id_token, charts_info): tool = self._find_related_tool_record(chart_name, chart_version, image_tag) if not tool: log.warn("this chart({}-{}) has not available from DB. ".format(chart_name, chart_version)) - continue - self._add_new_item_to_tool_box(user, tool_box, tool, tools_info, charts_info) + else: + self._add_new_item_to_tool_box(user, tool_box, tool, tools_info, charts_info) tools_info[tool_box]["deployment"] = { - "tool_id": tool.id, + "tool_id": tool.id if tool else -1, "chart_name": chart_name, "chart_version": chart_version, "image_tag": image_tag, "description": tool.description if tool else 'Not available', - "tool_release_tag": tool.tool_release_tag(image_tag=image_tag), "status": ToolDeployment(tool, user).get_status(id_token, deployment=deployment) } @@ -127,22 +125,25 @@ def _retrieve_detail_tool_info(self, user, tools, charts_info): return tools_info def _get_charts_info(self, tool_list): - # Each version now needs to display the chart_name and the - # "app_version" metadata from helm. TODO: Stop using helm. + # We may need the default image_tag from helm chart + # unless we configure it specifically in parameters of tool release charts_info = {} - chart_entries = get_helm_entries() + chart_entries = None for tool in tool_list: if tool.version in charts_info: continue - chart_app_version = get_chart_version_info(chart_entries, tool.chart_name, tool.version) image_tag = tool.image_tag - if chart_app_version and not 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] ={ - "app_version": chart_app_version.app_version if chart_app_version else 'Unknown', - "image_tag": image_tag - } + charts_info[tool.version] = image_tag return charts_info def get_context_data(self, *args, **kwargs): @@ -169,10 +170,9 @@ def get_context_data(self, *args, **kwargs): "chart_version": , "tool_id": , "status": }, "releases": { - "rstudio-2.2.5-": { + "": { "chart_name": "rstudio", "description": "RStudio: 1.2.1335+conda, R: 3.5.1, Python: 3.7.1, patch: 10", "image_tag": "4.0.5", diff --git a/tests/api/cluster/test_tool_deployment.py b/tests/api/cluster/test_tool_deployment.py index 7865ee354..68c9a32ae 100644 --- a/tests/api/cluster/test_tool_deployment.py +++ b/tests/api/cluster/test_tool_deployment.py @@ -5,27 +5,6 @@ from controlpanel.api.models import Tool, User -def test_get_installed_chart_version(): - user = User(username="test-user") - tool = Tool(chart_name="test-chart") - id_token = "dummy" - - installed_chart_version = "1.2.3" - - td = ToolDeployment(user, tool) - - deploy_metadata = Mock("k8s Deployment - metadata") - deploy_metadata.labels = { - "chart": f"{tool.chart_name}-{installed_chart_version}" - } - deploy = Mock("k8s Deployment", metadata=deploy_metadata) - - with patch("controlpanel.api.cluster.ToolDeployment.get_deployment") as get_deployment: - get_deployment.return_value = deploy - assert td.get_installed_chart_version(id_token) == installed_chart_version - get_deployment.assert_called_with(id_token) - - def test_url(): """ Ensures the URL for the tool release is properly constructed either from diff --git a/tests/api/models/test_tool.py b/tests/api/models/test_tool.py index 06dab1e33..d282880b0 100644 --- a/tests/api/models/test_tool.py +++ b/tests/api/models/test_tool.py @@ -50,30 +50,6 @@ def cluster(): yield cluster -@pytest.mark.parametrize( - "chart_version, expected_app_version", - [ - (None, None), - ("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",], -) -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) - id_token = "dummy" - - cluster_td = cluster.ToolDeployment.return_value - cluster_td.get_installed_chart_version.return_value = chart_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.django_db @pytest.mark.parametrize( "chart_version, expected_description", diff --git a/tests/api/test_helm.py b/tests/api/test_helm.py index a36d5a60d..2ac7ba6bb 100644 --- a/tests/api/test_helm.py +++ b/tests/api/test_helm.py @@ -23,32 +23,25 @@ def test_chart_app_version(): assert chart.app_version == app_version -def test_helm_repository_chart_info_when_chart_not_found( - helm_repository_index, -): - with patch("builtins.open", helm_repository_index): - info = helm.get_chart_info("notfound") - assert info == {} - - -def test_helm_repository_chart_info_when_chart_found(helm_repository_index): +def test_helm_repository(helm_repository_index): with patch("builtins.open", helm_repository_index): # See tests/api/fixtures/helm_mojanalytics_index.py - rstudio_info = helm.get_chart_info("rstudio") + entries = helm.get_helm_entries() + rstudio_info = entries.get('rstudio') rstudio_2_2_5_app_version = ( "RStudio: 1.2.1335+conda, R: 3.5.1, Python: 3.7.1, patch: 10" ) assert len(rstudio_info) == 2 - assert "2.2.5" in rstudio_info - assert "1.0.0" in rstudio_info + assert "2.2.5" in rstudio_info[0]["version"] + assert "1.0.0" in rstudio_info[1]["version"] - assert rstudio_info["2.2.5"].app_version == rstudio_2_2_5_app_version + assert rstudio_info[0].get("appVersion") == rstudio_2_2_5_app_version # Helm added `appVersion` field in metadata only # "recently" so for testing that for old chart # version this returns `None` - assert rstudio_info["1.0.0"].app_version == None + assert rstudio_info[1].get("appVersion") == None @pytest.mark.parametrize( diff --git a/tests/api/views/test_tool_deployments.py b/tests/api/views/test_tool_deployments.py index 9b035ecba..f3a3a5e5f 100644 --- a/tests/api/views/test_tool_deployments.py +++ b/tests/api/views/test_tool_deployments.py @@ -20,6 +20,6 @@ def test_post_not_supported_action(client): def test_post(client): - data = {'version': "rstudio__v1.0.0"} + data = {'version': "rstudio__v1.0.0__1"} response = client.post(reverse('tool-deployments', ('rstudio', 'deploy')), data) assert response.status_code == status.HTTP_200_OK diff --git a/tests/frontend/test_consumers.py b/tests/frontend/test_consumers.py index f39b6eec7..d2e10f555 100644 --- a/tests/frontend/test_consumers.py +++ b/tests/frontend/test_consumers.py @@ -211,6 +211,7 @@ def test_update_tool_status(): { "toolName": tool.chart_name, "version": tool.version, + "tool_id": tool.id, "status": status, } ),