From cf1b85dd726c1947f9ff2af8d89aa240747f462d Mon Sep 17 00:00:00 2001 From: jerasmus Date: Tue, 19 Feb 2019 11:24:37 +0200 Subject: Add ability to edit Knative domain Added the functionality to edit the Knative domain --- app/assets/javascripts/clusters/clusters_bundle.js | 18 ++ .../clusters/components/application_row.vue | 23 +-- .../clusters/components/applications.vue | 184 +++++++++++++-------- .../clusters/services/clusters_service.js | 7 + .../javascripts/clusters/stores/clusters_store.js | 7 +- .../unreleased/56937-edit-knative-domain.yml | 5 + locale/gitlab.pot | 18 +- .../clusters/components/application_row_spec.js | 10 +- .../clusters/components/applications_spec.js | 169 ++++++++++--------- spec/javascripts/clusters/services/mock_data.js | 12 +- .../clusters/stores/clusters_store_spec.js | 1 + 11 files changed, 286 insertions(+), 168 deletions(-) create mode 100644 changelogs/unreleased/56937-edit-knative-domain.yml diff --git a/app/assets/javascripts/clusters/clusters_bundle.js b/app/assets/javascripts/clusters/clusters_bundle.js index 6ebd1ad109e..1667d027a84 100644 --- a/app/assets/javascripts/clusters/clusters_bundle.js +++ b/app/assets/javascripts/clusters/clusters_bundle.js @@ -36,6 +36,7 @@ export default class Clusters { installRunnerPath, installJupyterPath, installKnativePath, + updateKnativePath, installPrometheusPath, managePrometheusPath, hasRbac, @@ -62,6 +63,7 @@ export default class Clusters { installPrometheusEndpoint: installPrometheusPath, installJupyterEndpoint: installJupyterPath, installKnativeEndpoint: installKnativePath, + updateKnativeEndpoint: updateKnativePath, }); this.installApplication = this.installApplication.bind(this); @@ -129,6 +131,8 @@ export default class Clusters { eventHub.$on('upgradeApplication', data => this.upgradeApplication(data)); eventHub.$on('upgradeFailed', appId => this.upgradeFailed(appId)); eventHub.$on('dismissUpgradeSuccess', appId => this.dismissUpgradeSuccess(appId)); + eventHub.$on('saveKnativeDomain', data => this.saveKnativeDomain(data)); + eventHub.$on('setKnativeHostname', data => this.setKnativeHostname(data)); } removeListeners() { @@ -137,6 +141,8 @@ export default class Clusters { eventHub.$off('upgradeApplication', this.upgradeApplication); eventHub.$off('upgradeFailed', this.upgradeFailed); eventHub.$off('dismissUpgradeSuccess', this.dismissUpgradeSuccess); + eventHub.$off('saveKnativeDomain'); + eventHub.$off('setKnativeHostname'); } initPolling() { @@ -272,6 +278,18 @@ export default class Clusters { this.store.updateAppProperty(appId, 'requestStatus', null); } + saveKnativeDomain(data) { + const appId = data.id; + this.store.updateAppProperty(appId, 'status', APPLICATION_STATUS.UPDATING); + this.service.updateApplication(appId, data.params); + } + + setKnativeHostname(data) { + const appId = data.id; + this.store.updateAppProperty(appId, 'isEditingHostName', true); + this.store.updateAppProperty(appId, 'hostname', data.hostname); + } + destroy() { this.destroyed = true; diff --git a/app/assets/javascripts/clusters/components/application_row.vue b/app/assets/javascripts/clusters/components/application_row.vue index 5952e93b9a7..19e5ac1567d 100644 --- a/app/assets/javascripts/clusters/components/application_row.vue +++ b/app/assets/javascripts/clusters/components/application_row.vue @@ -191,14 +191,7 @@ export default { return this.status === APPLICATION_STATUS.UPDATE_ERRORED; }, upgradeFailureDescription() { - return sprintf( - s__( - 'ClusterIntegration|Something went wrong when upgrading %{title}. Please check the logs and try again.', - ), - { - title: this.title, - }, - ); + return s__('ClusterIntegration|Update failed. Please check the logs and try again.'); }, upgradeSuccessDescription() { return sprintf(s__('ClusterIntegration|%{title} upgraded successfully.'), { @@ -210,9 +203,9 @@ export default { if (this.upgradeAvailable && !this.upgradeFailed && !this.isUpgrading) { label = s__('ClusterIntegration|Upgrade'); } else if (this.isUpgrading) { - label = s__('ClusterIntegration|Upgrading'); + label = s__('ClusterIntegration|Updating'); } else if (this.upgradeFailed) { - label = s__('ClusterIntegration|Retry upgrade'); + label = s__('ClusterIntegration|Retry update'); } return label; @@ -224,6 +217,14 @@ export default { (this.upgradeRequested && !this.upgradeSuccessful) ); }, + shouldShowUpgradeDetails() { + // This method only returns true when; + // Upgrade was successful OR Upgrade failed + // AND new upgrade is unavailable AND version information is present. + return ( + (this.upgradeSuccessful || this.upgradeFailed) && !this.upgradeAvailable && this.version + ); + }, }, watch: { status() { @@ -303,7 +304,7 @@ export default {
{{ versionLabel }} diff --git a/app/assets/javascripts/clusters/components/applications.vue b/app/assets/javascripts/clusters/components/applications.vue index 0cf187d4189..f74cd71de04 100644 --- a/app/assets/javascripts/clusters/components/applications.vue +++ b/app/assets/javascripts/clusters/components/applications.vue @@ -15,11 +15,14 @@ import { s__, sprintf } from '../../locale'; import applicationRow from './application_row.vue'; import clipboardButton from '../../vue_shared/components/clipboard_button.vue'; import { CLUSTER_TYPE, APPLICATION_STATUS, INGRESS } from '../constants'; +import LoadingButton from '~/vue_shared/components/loading_button.vue'; +import eventHub from '~/clusters/event_hub'; export default { components: { applicationRow, clipboardButton, + LoadingButton, }, props: { type: { @@ -173,16 +176,55 @@ export default { jupyterHostname() { return this.applications.jupyter.hostname; }, + knative() { + return this.applications.knative; + }, knativeInstalled() { - return this.applications.knative.status === APPLICATION_STATUS.INSTALLED; + return ( + this.knative.status === APPLICATION_STATUS.INSTALLED || + this.knativeUpgrading || + this.knativeUpgradeFailed || + this.knative.status === APPLICATION_STATUS.UPDATED + ); + }, + knativeUpgrading() { + return ( + this.knative.status === APPLICATION_STATUS.UPDATING || + this.knative.status === APPLICATION_STATUS.SCHEDULED + ); + }, + knativeUpgradeFailed() { + return this.knative.status === APPLICATION_STATUS.UPDATE_ERRORED; }, knativeExternalIp() { - return this.applications.knative.externalIp; + return this.knative.externalIp; + }, + canUpdateKnativeEndpoint() { + return this.knativeExternalIp && !this.knativeUpgradeFailed && !this.knativeUpgrading; + }, + knativeHostname: { + get() { + return this.knative.hostname; + }, + set(hostname) { + eventHub.$emit('setKnativeHostname', { + id: 'knative', + hostname, + }); + }, }, }, created() { this.helmInstallIllustration = helmInstallIllustration; }, + methods: { + saveKnativeDomain() { + eventHub.$emit('saveKnativeDomain', { + id: 'knative', + params: { hostname: this.knative.hostname }, + }); + }, + }, }; @@ -471,76 +513,88 @@ export default { }}

- - - +
diff --git a/app/assets/javascripts/clusters/services/clusters_service.js b/app/assets/javascripts/clusters/services/clusters_service.js index 89dda4b7902..dea33ac44c5 100644 --- a/app/assets/javascripts/clusters/services/clusters_service.js +++ b/app/assets/javascripts/clusters/services/clusters_service.js @@ -12,6 +12,9 @@ export default class ClusterService { jupyter: this.options.installJupyterEndpoint, knative: this.options.installKnativeEndpoint, }; + this.appUpdateEndpointMap = { + knative: this.options.updateKnativeEndpoint, + }; } fetchData() { @@ -22,6 +25,10 @@ export default class ClusterService { return axios.post(this.appInstallEndpointMap[appId], params); } + updateApplication(appId, params) { + return axios.patch(this.appUpdateEndpointMap[appId], params); + } + static updateCluster(endpoint, data) { return axios.put(endpoint, data); } diff --git a/app/assets/javascripts/clusters/stores/clusters_store.js b/app/assets/javascripts/clusters/stores/clusters_store.js index d309678be27..3f03a8512fc 100644 --- a/app/assets/javascripts/clusters/stores/clusters_store.js +++ b/app/assets/javascripts/clusters/stores/clusters_store.js @@ -66,6 +66,7 @@ export default class ClusterStore { requestStatus: null, requestReason: null, hostname: null, + isEditingHostName: false, externalIp: null, }, }, @@ -129,8 +130,10 @@ export default class ClusterStore { ? `jupyter.${this.state.applications.ingress.externalIp}.nip.io` : ''); } else if (appId === KNATIVE) { - this.state.applications.knative.hostname = - serverAppEntry.hostname || this.state.applications.knative.hostname; + if (!this.state.applications.knative.isEditingHostName) { + this.state.applications.knative.hostname = + serverAppEntry.hostname || this.state.applications.knative.hostname; + } this.state.applications.knative.externalIp = serverAppEntry.external_ip || this.state.applications.knative.externalIp; } else if (appId === RUNNER) { diff --git a/changelogs/unreleased/56937-edit-knative-domain.yml b/changelogs/unreleased/56937-edit-knative-domain.yml new file mode 100644 index 00000000000..7147a4e06b1 --- /dev/null +++ b/changelogs/unreleased/56937-edit-knative-domain.yml @@ -0,0 +1,5 @@ +--- +title: Edit Knative domain after it has been deployed +merge_request: 25386 +author: +type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index cb599e4744c..ce29fc489be 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1683,7 +1683,7 @@ msgstr "" msgid "ClusterIntegration|Copy Jupyter Hostname to clipboard" msgstr "" -msgid "ClusterIntegration|Copy Knative IP Address to clipboard" +msgid "ClusterIntegration|Copy Knative Endpoint to clipboard" msgstr "" msgid "ClusterIntegration|Copy Kubernetes cluster name" @@ -1806,7 +1806,7 @@ msgstr "" msgid "ClusterIntegration|Knative Domain Name:" msgstr "" -msgid "ClusterIntegration|Knative IP Address:" +msgid "ClusterIntegration|Knative Endpoint:" msgstr "" msgid "ClusterIntegration|Knative extends Kubernetes to provide a set of middleware components that are essential to build modern, source-centric, and container-based applications that can run anywhere: on premises, in the cloud, or even in a third-party data center." @@ -1926,7 +1926,7 @@ msgstr "" msgid "ClusterIntegration|Request to begin installing failed" msgstr "" -msgid "ClusterIntegration|Retry upgrade" +msgid "ClusterIntegration|Retry update" msgstr "" msgid "ClusterIntegration|Save changes" @@ -1971,9 +1971,6 @@ msgstr "" msgid "ClusterIntegration|Something went wrong on our end." msgstr "" -msgid "ClusterIntegration|Something went wrong when upgrading %{title}. Please check the logs and try again." -msgstr "" - msgid "ClusterIntegration|Something went wrong while creating your Kubernetes cluster on Google Kubernetes Engine" msgstr "" @@ -1992,12 +1989,21 @@ msgstr "" msgid "ClusterIntegration|This option will allow you to install applications on RBAC clusters." msgstr "" +msgid "ClusterIntegration|To access your application after deployment, point a wildcard DNS to the Knative Endpoint." +msgstr "" + msgid "ClusterIntegration|Toggle Kubernetes cluster" msgstr "" msgid "ClusterIntegration|Token" msgstr "" +msgid "ClusterIntegration|Update failed. Please check the logs and try again." +msgstr "" + +msgid "ClusterIntegration|Updating" +msgstr "" + msgid "ClusterIntegration|Upgrade" msgstr "" diff --git a/spec/javascripts/clusters/components/application_row_spec.js b/spec/javascripts/clusters/components/application_row_spec.js index 8cb9713964e..a2dd4e93daf 100644 --- a/spec/javascripts/clusters/components/application_row_spec.js +++ b/spec/javascripts/clusters/components/application_row_spec.js @@ -230,7 +230,7 @@ describe('Application Row', () => { expect(upgradeBtn.innerHTML).toContain('Upgrade'); }); - it('has enabled "Retry upgrade" when APPLICATION_STATUS.UPDATE_ERRORED', () => { + it('has enabled "Retry update" when APPLICATION_STATUS.UPDATE_ERRORED', () => { vm = mountComponent(ApplicationRow, { ...DEFAULT_APPLICATION_STATE, status: APPLICATION_STATUS.UPDATE_ERRORED, @@ -239,10 +239,10 @@ describe('Application Row', () => { expect(upgradeBtn).not.toBe(null); expect(vm.upgradeFailed).toBe(true); - expect(upgradeBtn.innerHTML).toContain('Retry upgrade'); + expect(upgradeBtn.innerHTML).toContain('Retry update'); }); - it('has disabled "Retry upgrade" when APPLICATION_STATUS.UPDATING', () => { + it('has disabled "Updating" when APPLICATION_STATUS.UPDATING', () => { vm = mountComponent(ApplicationRow, { ...DEFAULT_APPLICATION_STATE, status: APPLICATION_STATUS.UPDATING, @@ -251,7 +251,7 @@ describe('Application Row', () => { expect(upgradeBtn).not.toBe(null); expect(vm.isUpgrading).toBe(true); - expect(upgradeBtn.innerHTML).toContain('Upgrading'); + expect(upgradeBtn.innerHTML).toContain('Updating'); }); it('clicking upgrade button emits event', () => { @@ -295,7 +295,7 @@ describe('Application Row', () => { expect(failureMessage).not.toBe(null); expect(failureMessage.innerHTML).toContain( - 'Something went wrong when upgrading GitLab Runner. Please check the logs and try again.', + 'Update failed. Please check the logs and try again.', ); }); }); diff --git a/spec/javascripts/clusters/components/applications_spec.js b/spec/javascripts/clusters/components/applications_spec.js index 14ef1193984..8daf0282184 100644 --- a/spec/javascripts/clusters/components/applications_spec.js +++ b/spec/javascripts/clusters/components/applications_spec.js @@ -1,7 +1,9 @@ import Vue from 'vue'; import applications from '~/clusters/components/applications.vue'; import { CLUSTER_TYPE } from '~/clusters/constants'; +import eventHub from '~/clusters/event_hub'; import mountComponent from 'spec/helpers/vue_mount_component_helper'; +import { APPLICATIONS_MOCK_STATE } from '../services/mock_data'; describe('Applications', () => { let vm; @@ -18,16 +20,8 @@ describe('Applications', () => { describe('Project cluster applications', () => { beforeEach(() => { vm = mountComponent(Applications, { + applications: APPLICATIONS_MOCK_STATE, type: CLUSTER_TYPE.PROJECT, - applications: { - helm: { title: 'Helm Tiller' }, - ingress: { title: 'Ingress' }, - cert_manager: { title: 'Cert-Manager' }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub' }, - knative: { title: 'Knative' }, - }, }); }); @@ -64,15 +58,7 @@ describe('Applications', () => { beforeEach(() => { vm = mountComponent(Applications, { type: CLUSTER_TYPE.GROUP, - applications: { - helm: { title: 'Helm Tiller' }, - ingress: { title: 'Ingress' }, - cert_manager: { title: 'Cert-Manager' }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub' }, - knative: { title: 'Knative' }, - }, + applications: APPLICATIONS_MOCK_STATE, }); }); @@ -111,17 +97,12 @@ describe('Applications', () => { it('renders ip address with a clipboard button', () => { vm = mountComponent(Applications, { applications: { + ...APPLICATIONS_MOCK_STATE, ingress: { title: 'Ingress', status: 'installed', externalIp: '0.0.0.0', }, - helm: { title: 'Helm Tiller' }, - cert_manager: { title: 'Cert-Manager' }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub', hostname: '' }, - knative: { title: 'Knative', hostname: '' }, }, }); @@ -137,16 +118,11 @@ describe('Applications', () => { it('renders an input text with a question mark and an alert text', () => { vm = mountComponent(Applications, { applications: { + ...APPLICATIONS_MOCK_STATE, ingress: { title: 'Ingress', status: 'installed', }, - helm: { title: 'Helm Tiller' }, - cert_manager: { title: 'Cert-Manager' }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub', hostname: '' }, - knative: { title: 'Knative', hostname: '' }, }, }); @@ -160,15 +136,7 @@ describe('Applications', () => { describe('before installing', () => { it('does not render the IP address', () => { vm = mountComponent(Applications, { - applications: { - helm: { title: 'Helm Tiller' }, - ingress: { title: 'Ingress' }, - cert_manager: { title: 'Cert-Manager' }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub', hostname: '' }, - knative: { title: 'Knative', hostname: '' }, - }, + applications: APPLICATIONS_MOCK_STATE, }); expect(vm.$el.textContent).not.toContain('Ingress IP Address'); @@ -181,17 +149,12 @@ describe('Applications', () => { it('renders email & allows editing', () => { vm = mountComponent(Applications, { applications: { - helm: { title: 'Helm Tiller', status: 'installed' }, - ingress: { title: 'Ingress', status: 'installed', externalIp: '1.1.1.1' }, + ...APPLICATIONS_MOCK_STATE, cert_manager: { title: 'Cert-Manager', email: 'before@example.com', status: 'installable', }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub', hostname: '', status: 'installable' }, - knative: { title: 'Knative', hostname: '', status: 'installable' }, }, }); @@ -204,17 +167,12 @@ describe('Applications', () => { it('renders email in readonly', () => { vm = mountComponent(Applications, { applications: { - helm: { title: 'Helm Tiller', status: 'installed' }, - ingress: { title: 'Ingress', status: 'installed', externalIp: '1.1.1.1' }, + ...APPLICATIONS_MOCK_STATE, cert_manager: { title: 'Cert-Manager', email: 'after@example.com', status: 'installed', }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub', hostname: '', status: 'installable' }, - knative: { title: 'Knative', hostname: '', status: 'installable' }, }, }); @@ -229,13 +187,12 @@ describe('Applications', () => { it('renders hostname active input', () => { vm = mountComponent(Applications, { applications: { - helm: { title: 'Helm Tiller', status: 'installed' }, - ingress: { title: 'Ingress', status: 'installed', externalIp: '1.1.1.1' }, - cert_manager: { title: 'Cert-Manager' }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub', hostname: '', status: 'installable' }, - knative: { title: 'Knative', hostname: '', status: 'installable' }, + ...APPLICATIONS_MOCK_STATE, + ingress: { + title: 'Ingress', + status: 'installed', + externalIp: '1.1.1.1', + }, }, }); @@ -247,13 +204,8 @@ describe('Applications', () => { it('does not render hostname input', () => { vm = mountComponent(Applications, { applications: { - helm: { title: 'Helm Tiller', status: 'installed' }, + ...APPLICATIONS_MOCK_STATE, ingress: { title: 'Ingress', status: 'installed' }, - cert_manager: { title: 'Cert-Manager' }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub', hostname: '', status: 'installable' }, - knative: { title: 'Knative', hostname: '', status: 'installable' }, }, }); @@ -265,13 +217,9 @@ describe('Applications', () => { it('renders readonly input', () => { vm = mountComponent(Applications, { applications: { - helm: { title: 'Helm Tiller', status: 'installed' }, + ...APPLICATIONS_MOCK_STATE, ingress: { title: 'Ingress', status: 'installed', externalIp: '1.1.1.1' }, - cert_manager: { title: 'Cert-Manager' }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, jupyter: { title: 'JupyterHub', status: 'installed', hostname: '' }, - knative: { title: 'Knative', status: 'installed', hostname: '' }, }, }); @@ -282,15 +230,7 @@ describe('Applications', () => { describe('without ingress installed', () => { beforeEach(() => { vm = mountComponent(Applications, { - applications: { - helm: { title: 'Helm Tiller' }, - ingress: { title: 'Ingress' }, - cert_manager: { title: 'Cert-Manager' }, - runner: { title: 'GitLab Runner' }, - prometheus: { title: 'Prometheus' }, - jupyter: { title: 'JupyterHub', status: 'not_installable' }, - knative: { title: 'Knative' }, - }, + applications: APPLICATIONS_MOCK_STATE, }); }); @@ -310,4 +250,77 @@ describe('Applications', () => { }); }); }); + + describe('Knative application', () => { + describe('when installed', () => { + describe('with ip address', () => { + const props = { + applications: { + ...APPLICATIONS_MOCK_STATE, + knative: { + title: 'Knative', + hostname: 'example.com', + status: 'installed', + externalIp: '1.1.1.1', + }, + }, + }; + it('renders ip address with a clipboard button', () => { + vm = mountComponent(Applications, props); + + expect(vm.$el.querySelector('.js-knative-ip-address').value).toEqual('1.1.1.1'); + + expect( + vm.$el + .querySelector('.js-knative-ip-clipboard-btn') + .getAttribute('data-clipboard-text'), + ).toEqual('1.1.1.1'); + }); + + it('renders domain & allows editing', () => { + expect(vm.$el.querySelector('.js-knative-domainname').value).toEqual('example.com'); + expect(vm.$el.querySelector('.js-knative-domainname').getAttribute('readonly')).toBe( + null, + ); + }); + + it('renders an update/save Knative domain button', () => { + expect(vm.$el.querySelector('.js-knative-save-domain-button')).not.toBe(null); + }); + + it('emits event when clicking Save changes button', () => { + spyOn(eventHub, '$emit'); + vm = mountComponent(Applications, props); + + const saveButton = vm.$el.querySelector('.js-knative-save-domain-button'); + + saveButton.click(); + + expect(eventHub.$emit).toHaveBeenCalledWith('saveKnativeDomain', { + id: 'knative', + params: { hostname: 'example.com' }, + }); + }); + }); + + describe('without ip address', () => { + it('renders an input text with a question mark and an alert text', () => { + vm = mountComponent(Applications, { + applications: { + ...APPLICATIONS_MOCK_STATE, + knative: { + title: 'Knative', + hostname: 'example.com', + status: 'installed', + }, + }, + }); + + expect(vm.$el.querySelector('.js-knative-ip-address').value).toEqual('?'); + + expect(vm.$el.querySelector('.js-no-knative-ip-message')).not.toBe(null); + }); + }); + }); + }); }); diff --git a/spec/javascripts/clusters/services/mock_data.js b/spec/javascripts/clusters/services/mock_data.js index 3c3d9977ffb..3ace19c6401 100644 --- a/spec/javascripts/clusters/services/mock_data.js +++ b/spec/javascripts/clusters/services/mock_data.js @@ -115,4 +115,14 @@ const DEFAULT_APPLICATION_STATE = { requestReason: null, }; -export { CLUSTERS_MOCK_DATA, DEFAULT_APPLICATION_STATE }; +const APPLICATIONS_MOCK_STATE = { + helm: { title: 'Helm Tiller', status: 'installable' }, + ingress: { title: 'Ingress', status: 'installable' }, + cert_manager: { title: 'Cert-Manager', status: 'installable' }, + runner: { title: 'GitLab Runner' }, + prometheus: { title: 'Prometheus' }, + jupyter: { title: 'JupyterHub', status: 'installable', hostname: '' }, + knative: { title: 'Knative ', status: 'installable', hostname: '' }, +}; + +export { CLUSTERS_MOCK_DATA, DEFAULT_APPLICATION_STATE, APPLICATIONS_MOCK_STATE }; diff --git a/spec/javascripts/clusters/stores/clusters_store_spec.js b/spec/javascripts/clusters/stores/clusters_store_spec.js index 37a4d6614f6..09bcdf91d91 100644 --- a/spec/javascripts/clusters/stores/clusters_store_spec.js +++ b/spec/javascripts/clusters/stores/clusters_store_spec.js @@ -111,6 +111,7 @@ describe('Clusters Store', () => { requestStatus: null, requestReason: null, hostname: null, + isEditingHostName: false, externalIp: null, }, cert_manager: { -- cgit v1.2.1 From f8234d9a086a43a95698da13d2734fe62ddb9ad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Mon, 18 Feb 2019 17:06:51 +0000 Subject: Creates Clusterss::ApplciationsController update endpoint - Creates new route - Creates new controller action - Creates call stack: Clusterss::ApplciationsController calls --> Clusters::Applications::UpdateService calls --> Clusters::Applications::ScheduleUpdateService calls --> ClusterUpdateAppWorker calls --> Clusters::Applications::PatchService --> ClusterWaitForAppInstallationWorker DRY req params Adds gcp_cluster:cluster_update_app queue Schedule_update_service is uneeded Extract common logic to a parent class (UpdateService will need it) Introduce new UpdateService Fix rescue class namespace Fix RuboCop offenses Adds BaseService for create and update services Remove request_handler code duplication Fixes update command Move update_command to ApplicationCore so all apps can use it Adds tests for Knative update_command Adds specs for PatchService Raise error if update receives an unistalled app Adds update_service spec Fix RuboCop offense Use subject in favor of go Adds update endpoint specs for project namespace Adds update endpoint specs for group namespace --- .../clusters/applications_controller.rb | 29 +++-- app/models/clusters/concerns/application_core.rb | 6 + .../clusters/applications/base_helm_service.rb | 4 + app/services/clusters/applications/base_service.rb | 76 ++++++++++++ .../clusters/applications/create_service.rb | 57 +-------- .../clusters/applications/patch_service.rb | 26 +++++ .../clusters/applications/update_service.rb | 34 ++++++ app/workers/all_queues.yml | 1 + app/workers/cluster_update_app_worker.rb | 13 +++ config/routes.rb | 1 + lib/gitlab/kubernetes/helm/install_command.rb | 3 +- .../clusters/applications_controller_spec.rb | 100 ++++++++++++---- .../clusters/applications_controller_spec.rb | 97 +++++++++++++--- spec/models/clusters/applications/knative_spec.rb | 26 ++++- .../clusters/applications/patch_service_spec.rb | 128 +++++++++++++++++++++ .../clusters/applications/update_service_spec.rb | 72 ++++++++++++ 16 files changed, 569 insertions(+), 104 deletions(-) create mode 100644 app/services/clusters/applications/base_service.rb create mode 100644 app/services/clusters/applications/patch_service.rb create mode 100644 app/services/clusters/applications/update_service.rb create mode 100644 app/workers/cluster_update_app_worker.rb create mode 100644 spec/services/clusters/applications/patch_service_spec.rb create mode 100644 spec/services/clusters/applications/update_service_spec.rb diff --git a/app/controllers/clusters/applications_controller.rb b/app/controllers/clusters/applications_controller.rb index c4e7fc950f9..73c744efeba 100644 --- a/app/controllers/clusters/applications_controller.rb +++ b/app/controllers/clusters/applications_controller.rb @@ -3,26 +3,41 @@ class Clusters::ApplicationsController < Clusters::BaseController before_action :cluster before_action :authorize_create_cluster!, only: [:create] + before_action :authorize_update_cluster!, only: [:update] def create - Clusters::Applications::CreateService - .new(@cluster, current_user, create_cluster_application_params) - .execute(request) + request_handler do + Clusters::Applications::CreateService + .new(@cluster, current_user, cluster_application_params) + .execute(request) + end + end + + def update + request_handler do + Clusters::Applications::UpdateService + .new(@cluster, current_user, cluster_application_params) + .execute(request) + end + end + + private + + def request_handler + yield head :no_content - rescue Clusters::Applications::CreateService::InvalidApplicationError + rescue Clusters::Applications::BaseService::InvalidApplicationError render_404 rescue StandardError head :bad_request end - private - def cluster @cluster ||= clusterable.clusters.find(params[:id]) || render_404 end - def create_cluster_application_params + def cluster_application_params params.permit(:application, :hostname, :email) end end diff --git a/app/models/clusters/concerns/application_core.rb b/app/models/clusters/concerns/application_core.rb index 683b45331f6..cdb42117281 100644 --- a/app/models/clusters/concerns/application_core.rb +++ b/app/models/clusters/concerns/application_core.rb @@ -30,6 +30,12 @@ module Clusters # Override if you need extra data synchronized # from K8s after installation end + + def update_command + command = install_command + command.version = version + command + end end end end diff --git a/app/services/clusters/applications/base_helm_service.rb b/app/services/clusters/applications/base_helm_service.rb index 8a71730d5ec..c38b2656260 100644 --- a/app/services/clusters/applications/base_helm_service.rb +++ b/app/services/clusters/applications/base_helm_service.rb @@ -46,6 +46,10 @@ module Clusters @install_command ||= app.install_command end + def update_command + @update_command ||= app.update_command + end + def upgrade_command(new_values = "") app.upgrade_command(new_values) end diff --git a/app/services/clusters/applications/base_service.rb b/app/services/clusters/applications/base_service.rb new file mode 100644 index 00000000000..cbd1cf03ae1 --- /dev/null +++ b/app/services/clusters/applications/base_service.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module Clusters + module Applications + class BaseService + InvalidApplicationError = Class.new(StandardError) + + attr_reader :cluster, :current_user, :params + + def initialize(cluster, user, params = {}) + @cluster = cluster + @current_user = user + @params = params.dup + end + + def execute(request) + instantiate_application.tap do |application| + if application.has_attribute?(:hostname) + application.hostname = params[:hostname] + end + + if application.has_attribute?(:email) + application.email = params[:email] + end + + if application.respond_to?(:oauth_application) + application.oauth_application = create_oauth_application(application, request) + end + + worker = worker_class(application) + + application.make_scheduled! + + worker.perform_async(application.name, application.id) + end + end + + protected + + def worker_class(application) + raise NotImplementedError + end + + def builders + raise NotImplementedError + end + + def project_builders + raise NotImplementedError + end + + def instantiate_application + builder.call(@cluster) || raise(InvalidApplicationError, "invalid application: #{application_name}") + end + + def builder + builders[application_name] || raise(InvalidApplicationError, "invalid application: #{application_name}") + end + + def application_name + params[:application] + end + + def create_oauth_application(application, request) + oauth_application_params = { + name: params[:application], + redirect_uri: application.callback_url, + scopes: 'api read_user openid', + owner: current_user + } + + ::Applications::CreateService.new(current_user, oauth_application_params).execute(request) + end + end + end +end diff --git a/app/services/clusters/applications/create_service.rb b/app/services/clusters/applications/create_service.rb index 12f8c849d41..bd7c31bb981 100644 --- a/app/services/clusters/applications/create_service.rb +++ b/app/services/clusters/applications/create_service.rb @@ -2,47 +2,11 @@ module Clusters module Applications - class CreateService - InvalidApplicationError = Class.new(StandardError) - - attr_reader :cluster, :current_user, :params - - def initialize(cluster, user, params = {}) - @cluster = cluster - @current_user = user - @params = params.dup - end - - def execute(request) - create_application.tap do |application| - if application.has_attribute?(:hostname) - application.hostname = params[:hostname] - end - - if application.has_attribute?(:email) - application.email = params[:email] - end - - if application.respond_to?(:oauth_application) - application.oauth_application = create_oauth_application(application, request) - end - - worker = application.updateable? ? ClusterUpgradeAppWorker : ClusterInstallAppWorker - - application.make_scheduled! - - worker.perform_async(application.name, application.id) - end - end - + class CreateService < Clusters::Applications::BaseService private - def create_application - builder.call(@cluster) - end - - def builder - builders[application_name] || raise(InvalidApplicationError, "invalid application: #{application_name}") + def worker_class(application) + application.updateable? ? ClusterUpgradeAppWorker : ClusterInstallAppWorker end def builders @@ -65,21 +29,6 @@ module Clusters "knative" => -> (cluster) { cluster.application_knative || cluster.build_application_knative } } end - - def application_name - params[:application] - end - - def create_oauth_application(application, request) - oauth_application_params = { - name: params[:application], - redirect_uri: application.callback_url, - scopes: 'api read_user openid', - owner: current_user - } - - ::Applications::CreateService.new(current_user, oauth_application_params).execute(request) - end end end end diff --git a/app/services/clusters/applications/patch_service.rb b/app/services/clusters/applications/patch_service.rb new file mode 100644 index 00000000000..67fc8fd9800 --- /dev/null +++ b/app/services/clusters/applications/patch_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Clusters + module Applications + class PatchService < BaseHelmService + def execute + return unless app.scheduled? + + begin + app.make_updating! + + helm_api.update(update_command) + + ClusterWaitForAppInstallationWorker.perform_in( + ClusterWaitForAppInstallationWorker::INTERVAL, app.name, app.id) + rescue Kubeclient::HttpError => e + log_error(e) + app.make_update_errored!("Kubernetes error: #{e.error_code}") + rescue StandardError => e + log_error(e) + app.make_update_errored!("Can't start update process.") + end + end + end + end +end diff --git a/app/services/clusters/applications/update_service.rb b/app/services/clusters/applications/update_service.rb new file mode 100644 index 00000000000..979d870cc94 --- /dev/null +++ b/app/services/clusters/applications/update_service.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Clusters + module Applications + class UpdateService < Clusters::Applications::BaseService + private + + def worker_class(application) + ClusterUpdateAppWorker + end + + def builders + { + "helm" => -> (cluster) { cluster.application_helm }, + "ingress" => -> (cluster) { cluster.application_ingress }, + "cert_manager" => -> (cluster) { cluster.application_cert_manager } + }.tap do |hash| + hash.merge!(project_builders) if cluster.project_type? + end + end + + # These applications will need extra configuration to enable them to work + # with groups of projects + def project_builders + { + "prometheus" => -> (cluster) { cluster.application_prometheus }, + "runner" => -> (cluster) { cluster.application_runner }, + "jupyter" => -> (cluster) { cluster.application_jupyter }, + "knative" => -> (cluster) { cluster.application_knative } + } + end + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index d86f654dd44..bc7db5669e1 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -23,6 +23,7 @@ - cronjob:prune_web_hook_logs - gcp_cluster:cluster_install_app +- gcp_cluster:cluster_update_app - gcp_cluster:cluster_upgrade_app - gcp_cluster:cluster_provision - gcp_cluster:cluster_wait_for_app_installation diff --git a/app/workers/cluster_update_app_worker.rb b/app/workers/cluster_update_app_worker.rb new file mode 100644 index 00000000000..bec422c34a9 --- /dev/null +++ b/app/workers/cluster_update_app_worker.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class ClusterUpdateAppWorker + include ApplicationWorker + include ClusterQueue + include ClusterApplications + + def perform(app_name, app_id) + find_application(app_name, app_id) do |app| + Clusters::Applications::PatchService.new(app).execute + end + end +end diff --git a/config/routes.rb b/config/routes.rb index 484e05114be..53c6225eff1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -101,6 +101,7 @@ Rails.application.routes.draw do member do scope :applications do post '/:application', to: 'clusters/applications#create', as: :install_applications + patch '/:application', to: 'clusters/applications#update', as: :update_applications end get :cluster_status, format: :json diff --git a/lib/gitlab/kubernetes/helm/install_command.rb b/lib/gitlab/kubernetes/helm/install_command.rb index f931248b747..e33ba9305ce 100644 --- a/lib/gitlab/kubernetes/helm/install_command.rb +++ b/lib/gitlab/kubernetes/helm/install_command.rb @@ -7,7 +7,8 @@ module Gitlab include BaseCommand include ClientCommand - attr_reader :name, :files, :chart, :version, :repository, :preinstall, :postinstall + attr_reader :name, :files, :chart, :repository, :preinstall, :postinstall + attr_accessor :version def initialize(name:, chart:, files:, rbac:, version: nil, repository: nil, preinstall: nil, postinstall: nil) @name = name diff --git a/spec/controllers/groups/clusters/applications_controller_spec.rb b/spec/controllers/groups/clusters/applications_controller_spec.rb index dd5263b077c..84da43fe2ef 100644 --- a/spec/controllers/groups/clusters/applications_controller_spec.rb +++ b/spec/controllers/groups/clusters/applications_controller_spec.rb @@ -9,9 +9,25 @@ describe Groups::Clusters::ApplicationsController do Clusters::Cluster::APPLICATIONS[application] end + shared_examples 'a secure endpoint' do + it { expect { subject }.to be_allowed_for(:admin) } + it { expect { subject }.to be_allowed_for(:owner).of(group) } + it { expect { subject }.to be_allowed_for(:maintainer).of(group) } + it { expect { subject }.to be_denied_for(:developer).of(group) } + it { expect { subject }.to be_denied_for(:reporter).of(group) } + it { expect { subject }.to be_denied_for(:guest).of(group) } + it { expect { subject }.to be_denied_for(:user) } + it { expect { subject }.to be_denied_for(:external) } + end + + let(:cluster) { create(:cluster, :group, :provided_by_gcp) } + let(:group) { cluster.group } + describe 'POST create' do - let(:cluster) { create(:cluster, :group, :provided_by_gcp) } - let(:group) { cluster.group } + subject do + post :create, params: params.merge(group_id: group) + end + let(:application) { 'helm' } let(:params) { { application: application, id: cluster.id } } @@ -26,7 +42,7 @@ describe Groups::Clusters::ApplicationsController do it 'schedule an application installation' do expect(ClusterInstallAppWorker).to receive(:perform_async).with(application, anything).once - expect { go }.to change { current_application.count } + expect { subject }.to change { current_application.count } expect(response).to have_http_status(:no_content) expect(cluster.application_helm).to be_scheduled end @@ -37,7 +53,7 @@ describe Groups::Clusters::ApplicationsController do end it 'return 404' do - expect { go }.not_to change { current_application.count } + expect { subject }.not_to change { current_application.count } expect(response).to have_http_status(:not_found) end end @@ -46,9 +62,7 @@ describe Groups::Clusters::ApplicationsController do let(:application) { 'unkwnown-app' } it 'return 404' do - go - - expect(response).to have_http_status(:not_found) + is_expected.to have_http_status(:not_found) end end @@ -58,9 +72,7 @@ describe Groups::Clusters::ApplicationsController do end it 'returns 400' do - go - - expect(response).to have_http_status(:bad_request) + is_expected.to have_http_status(:bad_request) end end end @@ -70,18 +82,66 @@ describe Groups::Clusters::ApplicationsController do allow(ClusterInstallAppWorker).to receive(:perform_async) end - it { expect { go }.to be_allowed_for(:admin) } - it { expect { go }.to be_allowed_for(:owner).of(group) } - it { expect { go }.to be_allowed_for(:maintainer).of(group) } - it { expect { go }.to be_denied_for(:developer).of(group) } - it { expect { go }.to be_denied_for(:reporter).of(group) } - it { expect { go }.to be_denied_for(:guest).of(group) } - it { expect { go }.to be_denied_for(:user) } - it { expect { go }.to be_denied_for(:external) } + it_behaves_like 'a secure endpoint' end + end - def go - post :create, params: params.merge(group_id: group) + describe 'PATCH update' do + subject do + patch :update, params: params.merge(group_id: group) + end + + let!(:application) { create(:clusters_applications_cert_managers, :installed, cluster: cluster) } + let(:application_name) { application.name } + let(:params) { { application: application_name, id: cluster.id, email: "new-email@example.com" } } + + describe 'functionality' do + let(:user) { create(:user) } + + before do + group.add_maintainer(user) + sign_in(user) + end + + context "when cluster and app exists" do + it "schedules an application update" do + expect(ClusterUpdateAppWorker).to receive(:perform_async).with(application.name, anything).once + + is_expected.to have_http_status(:no_content) + + expect(cluster.application_cert_manager).to be_scheduled + end + end + + context 'when cluster do not exists' do + before do + cluster.destroy! + end + + it { is_expected.to have_http_status(:not_found) } + end + + context 'when application is unknown' do + let(:application_name) { 'unkwnown-app' } + + it { is_expected.to have_http_status(:not_found) } + end + + context 'when application is already scheduled' do + before do + application.make_scheduled! + end + + it { is_expected.to have_http_status(:bad_request) } + end + end + + describe 'security' do + before do + allow(ClusterUpdateAppWorker).to receive(:perform_async) + end + + it_behaves_like 'a secure endpoint' end end end diff --git a/spec/controllers/projects/clusters/applications_controller_spec.rb b/spec/controllers/projects/clusters/applications_controller_spec.rb index cb558259225..247adf3f8c7 100644 --- a/spec/controllers/projects/clusters/applications_controller_spec.rb +++ b/spec/controllers/projects/clusters/applications_controller_spec.rb @@ -9,7 +9,22 @@ describe Projects::Clusters::ApplicationsController do Clusters::Cluster::APPLICATIONS[application] end + shared_examples 'a secure endpoint' do + it { expect { subject }.to be_allowed_for(:admin) } + it { expect { subject }.to be_allowed_for(:owner).of(project) } + it { expect { subject }.to be_allowed_for(:maintainer).of(project) } + it { expect { subject }.to be_denied_for(:developer).of(project) } + it { expect { subject }.to be_denied_for(:reporter).of(project) } + it { expect { subject }.to be_denied_for(:guest).of(project) } + it { expect { subject }.to be_denied_for(:user) } + it { expect { subject }.to be_denied_for(:external) } + end + describe 'POST create' do + subject do + post :create, params: params.merge(namespace_id: project.namespace, project_id: project) + end + let(:cluster) { create(:cluster, :project, :provided_by_gcp) } let(:project) { cluster.project } let(:application) { 'helm' } @@ -26,7 +41,7 @@ describe Projects::Clusters::ApplicationsController do it 'schedule an application installation' do expect(ClusterInstallAppWorker).to receive(:perform_async).with(application, anything).once - expect { go }.to change { current_application.count } + expect { subject }.to change { current_application.count } expect(response).to have_http_status(:no_content) expect(cluster.application_helm).to be_scheduled end @@ -37,7 +52,7 @@ describe Projects::Clusters::ApplicationsController do end it 'return 404' do - expect { go }.not_to change { current_application.count } + expect { subject }.not_to change { current_application.count } expect(response).to have_http_status(:not_found) end end @@ -46,9 +61,7 @@ describe Projects::Clusters::ApplicationsController do let(:application) { 'unkwnown-app' } it 'return 404' do - go - - expect(response).to have_http_status(:not_found) + is_expected.to have_http_status(:not_found) end end @@ -58,9 +71,7 @@ describe Projects::Clusters::ApplicationsController do end it 'returns 400' do - go - - expect(response).to have_http_status(:bad_request) + is_expected.to have_http_status(:bad_request) end end end @@ -70,18 +81,68 @@ describe Projects::Clusters::ApplicationsController do allow(ClusterInstallAppWorker).to receive(:perform_async) end - it { expect { go }.to be_allowed_for(:admin) } - it { expect { go }.to be_allowed_for(:owner).of(project) } - it { expect { go }.to be_allowed_for(:maintainer).of(project) } - it { expect { go }.to be_denied_for(:developer).of(project) } - it { expect { go }.to be_denied_for(:reporter).of(project) } - it { expect { go }.to be_denied_for(:guest).of(project) } - it { expect { go }.to be_denied_for(:user) } - it { expect { go }.to be_denied_for(:external) } + it_behaves_like 'a secure endpoint' end + end - def go - post :create, params: params.merge(namespace_id: project.namespace, project_id: project) + describe 'PATCH update' do + subject do + patch :update, params: params.merge(namespace_id: project.namespace, project_id: project) + end + + let(:cluster) { create(:cluster, :project, :provided_by_gcp) } + let(:project) { cluster.project } + let!(:application) { create(:clusters_applications_knative, :installed, cluster: cluster) } + let(:application_name) { application.name } + let(:params) { { application: application_name, id: cluster.id, hostname: "new.example.com" } } + + describe 'functionality' do + let(:user) { create(:user) } + + before do + project.add_maintainer(user) + sign_in(user) + end + + context "when cluster and app exists" do + it "schedules an application update" do + expect(ClusterUpdateAppWorker).to receive(:perform_async).with(application.name, anything).once + + is_expected.to have_http_status(:no_content) + + expect(cluster.application_knative).to be_scheduled + end + end + + context 'when cluster do not exists' do + before do + cluster.destroy! + end + + it { is_expected.to have_http_status(:not_found) } + end + + context 'when application is unknown' do + let(:application_name) { 'unkwnown-app' } + + it { is_expected.to have_http_status(:not_found) } + end + + context 'when application is already scheduled' do + before do + application.make_scheduled! + end + + it { is_expected.to have_http_status(:bad_request) } + end + end + + describe 'security' do + before do + allow(ClusterUpdateAppWorker).to receive(:perform_async) + end + + it_behaves_like 'a secure endpoint' end end end diff --git a/spec/models/clusters/applications/knative_spec.rb b/spec/models/clusters/applications/knative_spec.rb index 006b922ab27..4884a5927fb 100644 --- a/spec/models/clusters/applications/knative_spec.rb +++ b/spec/models/clusters/applications/knative_spec.rb @@ -66,9 +66,7 @@ describe Clusters::Applications::Knative do end end - describe '#install_command' do - subject { knative.install_command } - + shared_examples 'a command' do it 'should be an instance of Helm::InstallCommand' do expect(subject).to be_an_instance_of(Gitlab::Kubernetes::Helm::InstallCommand) end @@ -76,7 +74,6 @@ describe Clusters::Applications::Knative do it 'should be initialized with knative arguments' do expect(subject.name).to eq('knative') expect(subject.chart).to eq('knative/knative') - expect(subject.version).to eq('0.2.2') expect(subject.files).to eq(knative.files) end @@ -98,6 +95,27 @@ describe Clusters::Applications::Knative do end end + describe '#install_command' do + subject { knative.install_command } + + it 'should be initialized with latest version' do + expect(subject.version).to eq('0.2.2') + end + + it_behaves_like 'a command' + end + + describe '#update_command' do + let!(:current_installed_version) { knative.version = '0.1.0' } + subject { knative.update_command } + + it 'should be initialized with current version' do + expect(subject.version).to eq(current_installed_version) + end + + it_behaves_like 'a command' + end + describe '#files' do let(:application) { knative } let(:values) { subject[:'values.yaml'] } diff --git a/spec/services/clusters/applications/patch_service_spec.rb b/spec/services/clusters/applications/patch_service_spec.rb new file mode 100644 index 00000000000..d4ee3243b84 --- /dev/null +++ b/spec/services/clusters/applications/patch_service_spec.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Clusters::Applications::PatchService do + describe '#execute' do + let(:application) { create(:clusters_applications_knative, :scheduled) } + let!(:update_command) { application.update_command } + let(:service) { described_class.new(application) } + let(:helm_client) { instance_double(Gitlab::Kubernetes::Helm::Api) } + + before do + allow(service).to receive(:update_command).and_return(update_command) + allow(service).to receive(:helm_api).and_return(helm_client) + end + + context 'when there are no errors' do + before do + expect(helm_client).to receive(:update).with(update_command) + allow(ClusterWaitForAppInstallationWorker).to receive(:perform_in).and_return(nil) + end + + it 'make the application updating' do + expect(application.cluster).not_to be_nil + service.execute + + expect(application).to be_updating + end + + it 'schedule async installation status check' do + expect(ClusterWaitForAppInstallationWorker).to receive(:perform_in).once + + service.execute + end + end + + context 'when kubernetes cluster communication fails' do + let(:error) { Kubeclient::HttpError.new(500, 'system failure', nil) } + + before do + expect(helm_client).to receive(:update).with(update_command).and_raise(error) + end + + it 'make the application errored' do + service.execute + + expect(application).to be_update_errored + expect(application.status_reason).to match('Kubernetes error: 500') + end + + it 'logs errors' do + expect(service.send(:logger)).to receive(:error).with( + { + exception: 'Kubeclient::HttpError', + message: 'system failure', + service: 'Clusters::Applications::PatchService', + app_id: application.id, + project_ids: application.cluster.project_ids, + group_ids: [], + error_code: 500 + } + ) + + expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( + error, + extra: { + exception: 'Kubeclient::HttpError', + message: 'system failure', + service: 'Clusters::Applications::PatchService', + app_id: application.id, + project_ids: application.cluster.project_ids, + group_ids: [], + error_code: 500 + } + ) + + service.execute + end + end + + context 'a non kubernetes error happens' do + let(:application) { create(:clusters_applications_knative, :scheduled) } + let(:error) { StandardError.new('something bad happened') } + + before do + expect(application).to receive(:make_updating!).once.and_raise(error) + end + + it 'make the application errored' do + expect(helm_client).not_to receive(:update) + + service.execute + + expect(application).to be_update_errored + expect(application.status_reason).to eq("Can't start update process.") + end + + it 'logs errors' do + expect(service.send(:logger)).to receive(:error).with( + { + exception: 'StandardError', + error_code: nil, + message: 'something bad happened', + service: 'Clusters::Applications::PatchService', + app_id: application.id, + project_ids: application.cluster.projects.pluck(:id), + group_ids: [] + } + ) + + expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( + error, + extra: { + exception: 'StandardError', + error_code: nil, + message: 'something bad happened', + service: 'Clusters::Applications::PatchService', + app_id: application.id, + project_ids: application.cluster.projects.pluck(:id), + group_ids: [] + } + ) + + service.execute + end + end + end +end diff --git a/spec/services/clusters/applications/update_service_spec.rb b/spec/services/clusters/applications/update_service_spec.rb new file mode 100644 index 00000000000..22ad698f77d --- /dev/null +++ b/spec/services/clusters/applications/update_service_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Clusters::Applications::UpdateService do + include TestRequestHelpers + + let(:cluster) { create(:cluster, :project, :provided_by_gcp) } + let(:user) { create(:user) } + let(:params) { { application: 'knative', hostname: 'udpate.example.com' } } + let(:service) { described_class.new(cluster, user, params) } + + subject { service.execute(test_request) } + + describe '#execute' do + before do + allow(ClusterUpdateAppWorker).to receive(:perform_async) + end + + context 'application is not installed' do + it 'raises Clusters::Applications::BaseService::InvalidApplicationError' do + expect(ClusterUpdateAppWorker).not_to receive(:perform_async) + + expect { subject } + .to raise_exception { Clusters::Applications::BaseService::InvalidApplicationError } + .and not_change { Clusters::Applications::Knative.count } + .and not_change { Clusters::Applications::Knative.with_status(:scheduled).count } + end + end + + context 'application is installed' do + context 'application is schedulable' do + let!(:application) do + create(:clusters_applications_knative, status: 3, cluster: cluster) + end + + it 'updates the application data' do + expect do + subject + end.to change { application.reload.hostname }.to(params[:hostname]) + end + + it 'makes application scheduled!' do + subject + + expect(application.reload).to be_scheduled + end + + it 'schedules ClusterUpdateAppWorker' do + expect(ClusterUpdateAppWorker).to receive(:perform_async) + + subject + end + end + + context 'application is not schedulable' do + let!(:application) do + create(:clusters_applications_knative, status: 4, cluster: cluster) + end + + it 'raises StateMachines::InvalidTransition' do + expect(ClusterUpdateAppWorker).not_to receive(:perform_async) + + expect { subject } + .to raise_exception { StateMachines::InvalidTransition } + .and not_change { application.reload.hostname } + .and not_change { Clusters::Applications::Knative.with_status(:scheduled).count } + end + end + end + end +end -- cgit v1.2.1 From 1186a6fd5408737f1995ac16ffc18f6aaf431cce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Mon, 4 Mar 2019 10:08:53 +0000 Subject: Sends update route to the client - extends presenters to include update endpoint path - sends path to the client on clusters clusters show view. --- app/presenters/clusterable_presenter.rb | 4 ++++ app/presenters/group_clusterable_presenter.rb | 5 +++++ app/presenters/project_clusterable_presenter.rb | 5 +++++ app/views/clusters/clusters/show.html.haml | 1 + spec/presenters/group_clusterable_presenter_spec.rb | 8 ++++++++ spec/presenters/project_clusterable_presenter_spec.rb | 8 ++++++++ 6 files changed, 31 insertions(+) diff --git a/app/presenters/clusterable_presenter.rb b/app/presenters/clusterable_presenter.rb index d94d9118eee..34bdf156623 100644 --- a/app/presenters/clusterable_presenter.rb +++ b/app/presenters/clusterable_presenter.rb @@ -44,6 +44,10 @@ class ClusterablePresenter < Gitlab::View::Presenter::Delegated raise NotImplementedError end + def update_applications_cluster_path(cluster, application) + raise NotImplementedError + end + def cluster_path(cluster, params = {}) raise NotImplementedError end diff --git a/app/presenters/group_clusterable_presenter.rb b/app/presenters/group_clusterable_presenter.rb index ef6bbc0d109..f5b0bb64487 100644 --- a/app/presenters/group_clusterable_presenter.rb +++ b/app/presenters/group_clusterable_presenter.rb @@ -14,6 +14,11 @@ class GroupClusterablePresenter < ClusterablePresenter install_applications_group_cluster_path(clusterable, cluster, application) end + override :update_applications_cluster_path + def update_applications_cluster_path(cluster, application) + update_applications_group_cluster_path(clusterable, cluster, application) + end + override :cluster_path def cluster_path(cluster, params = {}) group_cluster_path(clusterable, cluster, params) diff --git a/app/presenters/project_clusterable_presenter.rb b/app/presenters/project_clusterable_presenter.rb index 63e69b91b11..8661ee02b68 100644 --- a/app/presenters/project_clusterable_presenter.rb +++ b/app/presenters/project_clusterable_presenter.rb @@ -14,6 +14,11 @@ class ProjectClusterablePresenter < ClusterablePresenter install_applications_project_cluster_path(clusterable, cluster, application) end + override :update_applications_cluster_path + def update_applications_cluster_path(cluster, application) + update_applications_project_cluster_path(clusterable, cluster, application) + end + override :cluster_path def cluster_path(cluster, params = {}) project_cluster_path(clusterable, cluster, params) diff --git a/app/views/clusters/clusters/show.html.haml b/app/views/clusters/clusters/show.html.haml index 1ef76ef801e..7d381c6d4a6 100644 --- a/app/views/clusters/clusters/show.html.haml +++ b/app/views/clusters/clusters/show.html.haml @@ -15,6 +15,7 @@ install_runner_path: clusterable.install_applications_cluster_path(@cluster, :runner), install_jupyter_path: clusterable.install_applications_cluster_path(@cluster, :jupyter), install_knative_path: clusterable.install_applications_cluster_path(@cluster, :knative), + update_knative_path: clusterable.update_applications_cluster_path(@cluster, :knative), toggle_status: @cluster.enabled? ? 'true': 'false', has_rbac: @cluster.platform_kubernetes_rbac? ? 'true': 'false', cluster_type: @cluster.cluster_type, diff --git a/spec/presenters/group_clusterable_presenter_spec.rb b/spec/presenters/group_clusterable_presenter_spec.rb index 205160742bf..fa77273f6aa 100644 --- a/spec/presenters/group_clusterable_presenter_spec.rb +++ b/spec/presenters/group_clusterable_presenter_spec.rb @@ -69,6 +69,14 @@ describe GroupClusterablePresenter do it { is_expected.to eq(install_applications_group_cluster_path(group, cluster, application)) } end + describe '#update_applications_cluster_path' do + let(:application) { :helm } + + subject { presenter.update_applications_cluster_path(cluster, application) } + + it { is_expected.to eq(update_applications_group_cluster_path(group, cluster, application)) } + end + describe '#cluster_path' do subject { presenter.cluster_path(cluster) } diff --git a/spec/presenters/project_clusterable_presenter_spec.rb b/spec/presenters/project_clusterable_presenter_spec.rb index c50d90ae1e8..6786a84243f 100644 --- a/spec/presenters/project_clusterable_presenter_spec.rb +++ b/spec/presenters/project_clusterable_presenter_spec.rb @@ -69,6 +69,14 @@ describe ProjectClusterablePresenter do it { is_expected.to eq(install_applications_project_cluster_path(project, cluster, application)) } end + describe '#update_applications_cluster_path' do + let(:application) { :helm } + + subject { presenter.update_applications_cluster_path(cluster, application) } + + it { is_expected.to eq(update_applications_project_cluster_path(project, cluster, application)) } + end + describe '#cluster_path' do subject { presenter.cluster_path(cluster) } -- cgit v1.2.1 From f933e6125c81877b9118ea753f3218f404bbaff7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Tue, 5 Mar 2019 13:33:09 +0000 Subject: Adds Knative udpate feature specs - specs for clicking Install button - specs for clicking Save changes button --- .../projects/clusters/applications_spec.rb | 81 +++++++++++++++++++--- 1 file changed, 73 insertions(+), 8 deletions(-) diff --git a/spec/features/projects/clusters/applications_spec.rb b/spec/features/projects/clusters/applications_spec.rb index 2c8d014c36d..d97bf246ce3 100644 --- a/spec/features/projects/clusters/applications_spec.rb +++ b/spec/features/projects/clusters/applications_spec.rb @@ -17,7 +17,7 @@ describe 'Clusters Applications', :js do end context 'when cluster is being created' do - let(:cluster) { create(:cluster, :providing_by_gcp, projects: [project])} + let(:cluster) { create(:cluster, :providing_by_gcp, projects: [project]) } it 'user is unable to install applications' do page.within('.js-cluster-application-row-helm') do @@ -28,7 +28,7 @@ describe 'Clusters Applications', :js do end context 'when cluster is created' do - let(:cluster) { create(:cluster, :provided_by_gcp, projects: [project])} + let(:cluster) { create(:cluster, :provided_by_gcp, projects: [project]) } it 'user can install applications' do page.within('.js-cluster-application-row-helm') do @@ -52,7 +52,7 @@ describe 'Clusters Applications', :js do expect(page.find(:css, '.js-cluster-application-install-button')['disabled']).to eq('true') expect(page).to have_css('.js-cluster-application-install-button', exact_text: 'Installing') - wait_until_helm_created! + wait_until_app_created!('helm') Clusters::Cluster.last.application_helm.make_installing! @@ -76,7 +76,7 @@ describe 'Clusters Applications', :js do end context 'on an abac cluster' do - let(:cluster) { create(:cluster, :provided_by_gcp, :rbac_disabled, projects: [project])} + let(:cluster) { create(:cluster, :provided_by_gcp, :rbac_disabled, projects: [project]) } it 'should show info block and not be installable' do page.within('.js-cluster-application-row-knative') do @@ -87,7 +87,7 @@ describe 'Clusters Applications', :js do end context 'on an rbac cluster' do - let(:cluster) { create(:cluster, :provided_by_gcp, projects: [project])} + let(:cluster) { create(:cluster, :provided_by_gcp, projects: [project]) } it 'should not show callout block and be installable' do page.within('.js-cluster-application-row-knative') do @@ -95,6 +95,60 @@ describe 'Clusters Applications', :js do expect(page).to have_css('.js-cluster-application-install-button:not([disabled])') end end + + describe 'when user clicks install button' do + def domainname_form_value + page.find('.js-knative-domainname').value + end + + before do + allow(ClusterInstallAppWorker).to receive(:perform_async) + allow(ClusterWaitForIngressIpAddressWorker).to receive(:perform_in) + allow(ClusterWaitForIngressIpAddressWorker).to receive(:perform_async) + + page.within('.js-cluster-application-row-knative') do + expect(page).to have_css('.js-cluster-application-install-button:not([disabled])') + + page.find('.js-knative-domainname').set("domain.example.org") + + click_button 'Install' + + wait_until_app_created!('knative') + + expect(page).to have_css('.js-cluster-application-install-button', exact_text: 'Installing') + + Clusters::Cluster.last.application_knative.make_installing! + Clusters::Cluster.last.application_knative.make_installed! + Clusters::Cluster.last.application_knative.update_attribute(:external_ip, '127.0.0.1') + end + end + + it 'shows status transition' do + page.within('.js-cluster-application-row-knative') do + expect(domainname_form_value).to eq('domain.example.org') + expect(page).to have_css('.js-cluster-application-install-button', exact_text: 'Installed') + end + + expect(page).to have_content('Knative was successfully installed on your Kubernetes cluster') + expect(page).to have_css('.js-knative-save-domain-button'), exact_text: 'Save changes' + end + + it 'can then update the domain' do + page.within('.js-cluster-application-row-knative') do + expect(ClusterUpdateAppWorker).to receive(:perform_async) + + expect(domainname_form_value).to eq('domain.example.org') + + page.find('.js-knative-domainname').set("new.domain.example.org") + + click_button 'Save changes' + + wait_until_app_updated!(cluster.application_knative) + + expect(domainname_form_value).to eq('new.domain.example.org') + end + end + end end end @@ -185,11 +239,22 @@ describe 'Clusters Applications', :js do end end - def wait_until_helm_created! + def wait_until_app_created!(app) + retries = 0 + + while Clusters::Cluster.last.send("application_#{app}").nil? + raise "Timed out waiting for #{ app } application to be created in DB" if (retries += 1) > 3 + + sleep(1) + end + end + + def wait_until_app_updated!(app) retries = 0 + updated_at = app.updated_at - while Clusters::Cluster.last.application_helm.nil? - raise "Timed out waiting for helm application to be created in DB" if (retries += 1) > 3 + while updated_at == app.reload.updated_at + raise "Timed out waiting for #{ app } application to be created in DB" if (retries += 1) > 3 sleep(1) end -- cgit v1.2.1 From 5e6e1dd54ae80f6725d75da01eb024d40aa47167 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Tue, 5 Mar 2019 15:14:29 +0000 Subject: Fix rubocop offenses --- spec/features/projects/clusters/applications_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/projects/clusters/applications_spec.rb b/spec/features/projects/clusters/applications_spec.rb index d97bf246ce3..7eabcda286a 100644 --- a/spec/features/projects/clusters/applications_spec.rb +++ b/spec/features/projects/clusters/applications_spec.rb @@ -243,7 +243,7 @@ describe 'Clusters Applications', :js do retries = 0 while Clusters::Cluster.last.send("application_#{app}").nil? - raise "Timed out waiting for #{ app } application to be created in DB" if (retries += 1) > 3 + raise "Timed out waiting for #{app} application to be created in DB" if (retries += 1) > 3 sleep(1) end @@ -254,7 +254,7 @@ describe 'Clusters Applications', :js do updated_at = app.updated_at while updated_at == app.reload.updated_at - raise "Timed out waiting for #{ app } application to be created in DB" if (retries += 1) > 3 + raise "Timed out waiting for #{app} application to be created in DB" if (retries += 1) > 3 sleep(1) end -- cgit v1.2.1 From 3bdff7aadfc68222086518a365496fb22357cb9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Tue, 5 Mar 2019 15:21:29 +0000 Subject: Rename ClusterUpdateAppWorker to ClusterPatchAppWorker - This is to avoid colision with EE ClusterUpdateAppWorker --- app/services/clusters/applications/update_service.rb | 2 +- app/workers/all_queues.yml | 2 +- app/workers/cluster_patch_app_worker.rb | 13 +++++++++++++ app/workers/cluster_update_app_worker.rb | 13 ------------- .../groups/clusters/applications_controller_spec.rb | 4 ++-- .../projects/clusters/applications_controller_spec.rb | 4 ++-- spec/features/projects/clusters/applications_spec.rb | 2 +- spec/services/clusters/applications/update_service_spec.rb | 10 +++++----- 8 files changed, 25 insertions(+), 25 deletions(-) create mode 100644 app/workers/cluster_patch_app_worker.rb delete mode 100644 app/workers/cluster_update_app_worker.rb diff --git a/app/services/clusters/applications/update_service.rb b/app/services/clusters/applications/update_service.rb index 979d870cc94..a9d4e609992 100644 --- a/app/services/clusters/applications/update_service.rb +++ b/app/services/clusters/applications/update_service.rb @@ -6,7 +6,7 @@ module Clusters private def worker_class(application) - ClusterUpdateAppWorker + ClusterPatchAppWorker end def builders diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index bc7db5669e1..b2d88567e0e 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -23,7 +23,7 @@ - cronjob:prune_web_hook_logs - gcp_cluster:cluster_install_app -- gcp_cluster:cluster_update_app +- gcp_cluster:cluster_patch_app - gcp_cluster:cluster_upgrade_app - gcp_cluster:cluster_provision - gcp_cluster:cluster_wait_for_app_installation diff --git a/app/workers/cluster_patch_app_worker.rb b/app/workers/cluster_patch_app_worker.rb new file mode 100644 index 00000000000..0549e81ed05 --- /dev/null +++ b/app/workers/cluster_patch_app_worker.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class ClusterPatchAppWorker + include ApplicationWorker + include ClusterQueue + include ClusterApplications + + def perform(app_name, app_id) + find_application(app_name, app_id) do |app| + Clusters::Applications::PatchService.new(app).execute + end + end +end diff --git a/app/workers/cluster_update_app_worker.rb b/app/workers/cluster_update_app_worker.rb deleted file mode 100644 index bec422c34a9..00000000000 --- a/app/workers/cluster_update_app_worker.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -class ClusterUpdateAppWorker - include ApplicationWorker - include ClusterQueue - include ClusterApplications - - def perform(app_name, app_id) - find_application(app_name, app_id) do |app| - Clusters::Applications::PatchService.new(app).execute - end - end -end diff --git a/spec/controllers/groups/clusters/applications_controller_spec.rb b/spec/controllers/groups/clusters/applications_controller_spec.rb index 84da43fe2ef..16a63536ea6 100644 --- a/spec/controllers/groups/clusters/applications_controller_spec.rb +++ b/spec/controllers/groups/clusters/applications_controller_spec.rb @@ -105,7 +105,7 @@ describe Groups::Clusters::ApplicationsController do context "when cluster and app exists" do it "schedules an application update" do - expect(ClusterUpdateAppWorker).to receive(:perform_async).with(application.name, anything).once + expect(ClusterPatchAppWorker).to receive(:perform_async).with(application.name, anything).once is_expected.to have_http_status(:no_content) @@ -138,7 +138,7 @@ describe Groups::Clusters::ApplicationsController do describe 'security' do before do - allow(ClusterUpdateAppWorker).to receive(:perform_async) + allow(ClusterPatchAppWorker).to receive(:perform_async) end it_behaves_like 'a secure endpoint' diff --git a/spec/controllers/projects/clusters/applications_controller_spec.rb b/spec/controllers/projects/clusters/applications_controller_spec.rb index 247adf3f8c7..cd1a01f8acc 100644 --- a/spec/controllers/projects/clusters/applications_controller_spec.rb +++ b/spec/controllers/projects/clusters/applications_controller_spec.rb @@ -106,7 +106,7 @@ describe Projects::Clusters::ApplicationsController do context "when cluster and app exists" do it "schedules an application update" do - expect(ClusterUpdateAppWorker).to receive(:perform_async).with(application.name, anything).once + expect(ClusterPatchAppWorker).to receive(:perform_async).with(application.name, anything).once is_expected.to have_http_status(:no_content) @@ -139,7 +139,7 @@ describe Projects::Clusters::ApplicationsController do describe 'security' do before do - allow(ClusterUpdateAppWorker).to receive(:perform_async) + allow(ClusterPatchAppWorker).to receive(:perform_async) end it_behaves_like 'a secure endpoint' diff --git a/spec/features/projects/clusters/applications_spec.rb b/spec/features/projects/clusters/applications_spec.rb index 7eabcda286a..3784203a6b4 100644 --- a/spec/features/projects/clusters/applications_spec.rb +++ b/spec/features/projects/clusters/applications_spec.rb @@ -135,7 +135,7 @@ describe 'Clusters Applications', :js do it 'can then update the domain' do page.within('.js-cluster-application-row-knative') do - expect(ClusterUpdateAppWorker).to receive(:perform_async) + expect(ClusterPatchAppWorker).to receive(:perform_async) expect(domainname_form_value).to eq('domain.example.org') diff --git a/spec/services/clusters/applications/update_service_spec.rb b/spec/services/clusters/applications/update_service_spec.rb index 22ad698f77d..2d299882af0 100644 --- a/spec/services/clusters/applications/update_service_spec.rb +++ b/spec/services/clusters/applications/update_service_spec.rb @@ -14,12 +14,12 @@ describe Clusters::Applications::UpdateService do describe '#execute' do before do - allow(ClusterUpdateAppWorker).to receive(:perform_async) + allow(ClusterPatchAppWorker).to receive(:perform_async) end context 'application is not installed' do it 'raises Clusters::Applications::BaseService::InvalidApplicationError' do - expect(ClusterUpdateAppWorker).not_to receive(:perform_async) + expect(ClusterPatchAppWorker).not_to receive(:perform_async) expect { subject } .to raise_exception { Clusters::Applications::BaseService::InvalidApplicationError } @@ -46,8 +46,8 @@ describe Clusters::Applications::UpdateService do expect(application.reload).to be_scheduled end - it 'schedules ClusterUpdateAppWorker' do - expect(ClusterUpdateAppWorker).to receive(:perform_async) + it 'schedules ClusterPatchAppWorker' do + expect(ClusterPatchAppWorker).to receive(:perform_async) subject end @@ -59,7 +59,7 @@ describe Clusters::Applications::UpdateService do end it 'raises StateMachines::InvalidTransition' do - expect(ClusterUpdateAppWorker).not_to receive(:perform_async) + expect(ClusterPatchAppWorker).not_to receive(:perform_async) expect { subject } .to raise_exception { StateMachines::InvalidTransition } -- cgit v1.2.1 From 70e443014ae3577d6ab17d87e4346cab75d217bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Wed, 6 Mar 2019 11:45:27 +0000 Subject: Use tap command for better readability --- app/models/clusters/concerns/application_core.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/clusters/concerns/application_core.rb b/app/models/clusters/concerns/application_core.rb index cdb42117281..ee964fb7c93 100644 --- a/app/models/clusters/concerns/application_core.rb +++ b/app/models/clusters/concerns/application_core.rb @@ -32,9 +32,9 @@ module Clusters end def update_command - command = install_command - command.version = version - command + install_command.tap do |command| + command.version = version + end end end end -- cgit v1.2.1 From 34d837b575350b98a8d3809556a07473ba7d0d54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Wed, 6 Mar 2019 11:50:39 +0000 Subject: Remove unecessary begin..end block --- .../clusters/applications/install_service.rb | 22 ++++++++++------------ .../clusters/applications/patch_service.rb | 22 ++++++++++------------ 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/app/services/clusters/applications/install_service.rb b/app/services/clusters/applications/install_service.rb index 5a65dc4ef59..5bd3623a558 100644 --- a/app/services/clusters/applications/install_service.rb +++ b/app/services/clusters/applications/install_service.rb @@ -6,19 +6,17 @@ module Clusters def execute return unless app.scheduled? - begin - app.make_installing! - helm_api.install(install_command) + app.make_installing! + helm_api.install(install_command) - ClusterWaitForAppInstallationWorker.perform_in( - ClusterWaitForAppInstallationWorker::INTERVAL, app.name, app.id) - rescue Kubeclient::HttpError => e - log_error(e) - app.make_errored!("Kubernetes error: #{e.error_code}") - rescue StandardError => e - log_error(e) - app.make_errored!("Can't start installation process.") - end + ClusterWaitForAppInstallationWorker.perform_in( + ClusterWaitForAppInstallationWorker::INTERVAL, app.name, app.id) + rescue Kubeclient::HttpError => e + log_error(e) + app.make_errored!("Kubernetes error: #{e.error_code}") + rescue StandardError => e + log_error(e) + app.make_errored!("Can't start installation process.") end end end diff --git a/app/services/clusters/applications/patch_service.rb b/app/services/clusters/applications/patch_service.rb index 67fc8fd9800..20c739af7a2 100644 --- a/app/services/clusters/applications/patch_service.rb +++ b/app/services/clusters/applications/patch_service.rb @@ -6,20 +6,18 @@ module Clusters def execute return unless app.scheduled? - begin - app.make_updating! + app.make_updating! - helm_api.update(update_command) + helm_api.update(update_command) - ClusterWaitForAppInstallationWorker.perform_in( - ClusterWaitForAppInstallationWorker::INTERVAL, app.name, app.id) - rescue Kubeclient::HttpError => e - log_error(e) - app.make_update_errored!("Kubernetes error: #{e.error_code}") - rescue StandardError => e - log_error(e) - app.make_update_errored!("Can't start update process.") - end + ClusterWaitForAppInstallationWorker.perform_in( + ClusterWaitForAppInstallationWorker::INTERVAL, app.name, app.id) + rescue Kubeclient::HttpError => e + log_error(e) + app.make_update_errored!("Kubernetes error: #{e.error_code}") + rescue StandardError => e + log_error(e) + app.make_update_errored!("Can't start update process.") end end end -- cgit v1.2.1 From c08beb5051224dbee52716b0fa9c4dd9fedad5ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Cunha?= Date: Wed, 6 Mar 2019 12:48:45 +0000 Subject: Don't use sleep in feature specs --- .../projects/clusters/applications_spec.rb | 33 ++++++---------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/spec/features/projects/clusters/applications_spec.rb b/spec/features/projects/clusters/applications_spec.rb index 3784203a6b4..713e25cdcb2 100644 --- a/spec/features/projects/clusters/applications_spec.rb +++ b/spec/features/projects/clusters/applications_spec.rb @@ -31,6 +31,8 @@ describe 'Clusters Applications', :js do let(:cluster) { create(:cluster, :provided_by_gcp, projects: [project]) } it 'user can install applications' do + wait_for_requests + page.within('.js-cluster-application-row-helm') do expect(page.find(:css, '.js-cluster-application-install-button')['disabled']).to be_nil expect(page).to have_css('.js-cluster-application-install-button', exact_text: 'Install') @@ -44,6 +46,8 @@ describe 'Clusters Applications', :js do page.within('.js-cluster-application-row-helm') do page.find(:css, '.js-cluster-application-install-button').click end + + wait_for_requests end it 'they see status transition' do @@ -52,8 +56,6 @@ describe 'Clusters Applications', :js do expect(page.find(:css, '.js-cluster-application-install-button')['disabled']).to eq('true') expect(page).to have_css('.js-cluster-application-install-button', exact_text: 'Installing') - wait_until_app_created!('helm') - Clusters::Cluster.last.application_helm.make_installing! # FE starts polling and update the buttons to "Installing" @@ -113,7 +115,7 @@ describe 'Clusters Applications', :js do click_button 'Install' - wait_until_app_created!('knative') + wait_for_requests expect(page).to have_css('.js-cluster-application-install-button', exact_text: 'Installing') @@ -143,7 +145,7 @@ describe 'Clusters Applications', :js do click_button 'Save changes' - wait_until_app_updated!(cluster.application_knative) + wait_for_requests expect(domainname_form_value).to eq('new.domain.example.org') end @@ -202,6 +204,8 @@ describe 'Clusters Applications', :js do page.within('.js-cluster-application-row-ingress') do expect(page).to have_css('.js-cluster-application-install-button:not([disabled])') page.find(:css, '.js-cluster-application-install-button').click + + wait_for_requests end end @@ -238,25 +242,4 @@ describe 'Clusters Applications', :js do end end end - - def wait_until_app_created!(app) - retries = 0 - - while Clusters::Cluster.last.send("application_#{app}").nil? - raise "Timed out waiting for #{app} application to be created in DB" if (retries += 1) > 3 - - sleep(1) - end - end - - def wait_until_app_updated!(app) - retries = 0 - updated_at = app.updated_at - - while updated_at == app.reload.updated_at - raise "Timed out waiting for #{app} application to be created in DB" if (retries += 1) > 3 - - sleep(1) - end - end end -- cgit v1.2.1