From b1f0acbcc7df2c703721a2862cb2db70d601aea9 Mon Sep 17 00:00:00 2001 From: Taka Nishida Date: Wed, 18 Dec 2024 16:27:18 +0900 Subject: [PATCH 1/6] Allow configuring "auto_stop_setting" through graphql This MR allows configuring "auto_stop_setting" field through GraphQL. Changelog: added --- app/graphql/mutations/environments/create.rb | 5 +++++ app/graphql/mutations/environments/update.rb | 5 +++++ app/graphql/types/environment_type.rb | 3 +++ .../types/environments/auto_stop_setting_enum.rb | 14 ++++++++++++++ app/services/environments/create_service.rb | 2 +- app/services/environments/update_service.rb | 2 +- doc/api/graphql/reference/index.md | 12 ++++++++++++ .../environments/auto_stop_setting_enum_spec.rb | 9 +++++++++ spec/services/environments/create_service_spec.rb | 3 ++- spec/services/environments/update_service_spec.rb | 6 +++++- 10 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 app/graphql/types/environments/auto_stop_setting_enum.rb create mode 100644 spec/graphql/types/environments/auto_stop_setting_enum_spec.rb diff --git a/app/graphql/mutations/environments/create.rb b/app/graphql/mutations/environments/create.rb index cb81095129d51c..1a942365d21a3a 100644 --- a/app/graphql/mutations/environments/create.rb +++ b/app/graphql/mutations/environments/create.rb @@ -50,6 +50,11 @@ class Create < ::Mutations::BaseMutation required: false, description: 'Flux resource path of the environment.' + argument :auto_stop_setting, + Types::Environments::AutoStopSettingEnum, + required: false, + description: 'Auto stop setting of the environment.' + field :environment, Types::EnvironmentType, null: true, diff --git a/app/graphql/mutations/environments/update.rb b/app/graphql/mutations/environments/update.rb index 81f0900eacf74c..ef2e98edd84d65 100644 --- a/app/graphql/mutations/environments/update.rb +++ b/app/graphql/mutations/environments/update.rb @@ -43,6 +43,11 @@ class Update < ::Mutations::BaseMutation required: false, description: 'Flux resource path of the environment.' + argument :auto_stop_setting, + Types::Environments::AutoStopSettingEnum, + required: false, + description: 'Auto stop setting of the environment.' + field :environment, Types::EnvironmentType, null: true, diff --git a/app/graphql/types/environment_type.rb b/app/graphql/types/environment_type.rb index a766a3bb0395b9..16dbce061a7a88 100644 --- a/app/graphql/types/environment_type.rb +++ b/app/graphql/types/environment_type.rb @@ -94,6 +94,9 @@ class EnvironmentType < BaseObject extension ::Gitlab::Graphql::Limit::FieldCallCount, limit: 1 end + field :auto_stop_setting, Types::Environments::AutoStopSettingEnum, + description: 'Auto stop setting of the environment.' + markdown_field :description_html, null: true def tier diff --git a/app/graphql/types/environments/auto_stop_setting_enum.rb b/app/graphql/types/environments/auto_stop_setting_enum.rb new file mode 100644 index 00000000000000..30430d7cdf66ab --- /dev/null +++ b/app/graphql/types/environments/auto_stop_setting_enum.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Types + module Environments + class AutoStopSettingEnum < BaseEnum + graphql_name 'AutoStopSetting' + description 'Auto stop setting.' + + ::Environment.auto_stop_settings.each_key do |key| + value key.upcase, value: key, description: key.titleize + end + end + end +end diff --git a/app/services/environments/create_service.rb b/app/services/environments/create_service.rb index f4da8eabfd1494..b58930483fd62f 100644 --- a/app/services/environments/create_service.rb +++ b/app/services/environments/create_service.rb @@ -3,7 +3,7 @@ module Environments class CreateService < BaseService ALLOWED_ATTRIBUTES = %i[name description external_url tier cluster_agent kubernetes_namespace - flux_resource_path].freeze + flux_resource_path auto_stop_setting].freeze def execute unless can?(current_user, :create_environment, project) diff --git a/app/services/environments/update_service.rb b/app/services/environments/update_service.rb index 7b34fce41d0b5b..beebb0c65c2964 100644 --- a/app/services/environments/update_service.rb +++ b/app/services/environments/update_service.rb @@ -3,7 +3,7 @@ module Environments class UpdateService < BaseService ALLOWED_ATTRIBUTES = %i[description external_url tier cluster_agent kubernetes_namespace - flux_resource_path].freeze + flux_resource_path auto_stop_setting].freeze def execute(environment) unless can?(current_user, :update_environment, environment) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index f8633332bdb89f..8b958ec56f547b 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -5174,6 +5174,7 @@ Input type: `EnvironmentCreateInput` | Name | Type | Description | | ---- | ---- | ----------- | +| `autoStopSetting` | [`AutoStopSetting`](#autostopsetting) | Auto stop setting of the environment. | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `clusterAgentId` | [`ClustersAgentID`](#clustersagentid) | Cluster agent of the environment. | | `description` | [`String`](#string) | Description of the environment. | @@ -5244,6 +5245,7 @@ Input type: `EnvironmentUpdateInput` | Name | Type | Description | | ---- | ---- | ----------- | +| `autoStopSetting` | [`AutoStopSetting`](#autostopsetting) | Auto stop setting of the environment. | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `clusterAgentId` | [`ClustersAgentID`](#clustersagentid) | Cluster agent of the environment. | | `description` | [`String`](#string) | Description of the environment. | @@ -23736,6 +23738,7 @@ Describes where code is deployed for a project. | ---- | ---- | ----------- | | `autoDeleteAt` | [`Time`](#time) | When the environment is going to be deleted automatically. | | `autoStopAt` | [`Time`](#time) | When the environment is going to be stopped automatically. | +| `autoStopSetting` | [`AutoStopSetting`](#autostopsetting) | Auto stop setting of the environment. | | `clusterAgent` | [`ClusterAgent`](#clusteragent) | Cluster agent of the environment. | | `createdAt` | [`Time`](#time) | When the environment was created. | | `deployFreezes` | [`[CiFreezePeriod!]`](#cifreezeperiod) | Deployment freeze periods of the environment. | @@ -38718,6 +38721,15 @@ Assignee ID wildcard values. | `ANY` | An assignee is assigned. | | `NONE` | No assignee is assigned. | +### `AutoStopSetting` + +Auto stop setting. + +| Value | Description | +| ----- | ----------- | +| `ALWAYS` | Always. | +| `WITH_ACTION` | With Action. | + ### `AvailabilityEnum` User availability status. diff --git a/spec/graphql/types/environments/auto_stop_setting_enum_spec.rb b/spec/graphql/types/environments/auto_stop_setting_enum_spec.rb new file mode 100644 index 00000000000000..912d62b321f32f --- /dev/null +++ b/spec/graphql/types/environments/auto_stop_setting_enum_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Types::Environments::AutoStopSettingEnum, feature_category: :environment_management do + it 'exposes all auto stop settings' do + expect(described_class.values.keys).to include(*%w[ALWAYS WITH_ACTION]) + end +end diff --git a/spec/services/environments/create_service_spec.rb b/spec/services/environments/create_service_spec.rb index 96dc1c75c57319..7210ed3524626a 100644 --- a/spec/services/environments/create_service_spec.rb +++ b/spec/services/environments/create_service_spec.rb @@ -14,7 +14,7 @@ describe '#execute' do subject { service.execute } - let(:params) { { name: 'production', description: 'description', external_url: 'https://gitlab.com', tier: :production } } + let(:params) { { name: 'production', description: 'description', external_url: 'https://gitlab.com', tier: :production, auto_stop_setting: :always } } it 'creates an environment' do expect { subject }.to change { ::Environment.count }.by(1) @@ -28,6 +28,7 @@ expect(response.payload[:environment].description).to eq('description') expect(response.payload[:environment].external_url).to eq('https://gitlab.com') expect(response.payload[:environment].tier).to eq('production') + expect(response.payload[:environment].auto_stop_setting).to eq('always') end context 'with a cluster agent' do diff --git a/spec/services/environments/update_service_spec.rb b/spec/services/environments/update_service_spec.rb index b7c9c3bcd74f8c..6ccbbf50fa8215 100644 --- a/spec/services/environments/update_service_spec.rb +++ b/spec/services/environments/update_service_spec.rb @@ -15,7 +15,7 @@ describe '#execute' do subject { service.execute(environment) } - let(:params) { { external_url: 'https://gitlab.com/', description: 'description' } } + let(:params) { { external_url: 'https://gitlab.com/', description: 'description', auto_stop_setting: :with_action } } it 'updates the external URL' do expect { subject }.to change { environment.reload.external_url }.to('https://gitlab.com/') @@ -25,6 +25,10 @@ expect { subject }.to change { environment.reload.description }.to('description') end + it 'updates the auto stop setting' do + expect { subject }.to change { environment.reload.auto_stop_setting }.to('with_action') + end + it 'returns successful response' do response = subject -- GitLab From 0ebbca80968c946a5fae55fb6dcfd9c9091dcf64 Mon Sep 17 00:00:00 2001 From: Taka Nishida Date: Thu, 19 Dec 2024 13:02:56 +0900 Subject: [PATCH 2/6] Fix argument error when invalid auto_stop_setting is given --- app/services/environments/create_service.rb | 9 ++++----- app/services/environments/update_service.rb | 7 ++++--- .../environments/create_service_spec.rb | 18 +++++++++++++++++- .../environments/update_service_spec.rb | 14 +++++++++++++- 4 files changed, 38 insertions(+), 10 deletions(-) diff --git a/app/services/environments/create_service.rb b/app/services/environments/create_service.rb index b58930483fd62f..ba27b228b65389 100644 --- a/app/services/environments/create_service.rb +++ b/app/services/environments/create_service.rb @@ -19,13 +19,12 @@ def execute payload: { environment: nil }) end - environment = project.environments.create(**params.slice(*ALLOWED_ATTRIBUTES)) - - if environment.persisted? + begin + environment = project.environments.create!(**params.slice(*ALLOWED_ATTRIBUTES)) ServiceResponse.success(payload: { environment: environment }) - else + rescue StandardError => err ServiceResponse.error( - message: environment.errors.full_messages, + message: [err.message], payload: { environment: nil } ) end diff --git a/app/services/environments/update_service.rb b/app/services/environments/update_service.rb index beebb0c65c2964..fc16047e3b43c5 100644 --- a/app/services/environments/update_service.rb +++ b/app/services/environments/update_service.rb @@ -19,11 +19,12 @@ def execute(environment) payload: { environment: environment }) end - if environment.update(**params.slice(*ALLOWED_ATTRIBUTES)) + begin + environment.update!(**params.slice(*ALLOWED_ATTRIBUTES)) ServiceResponse.success(payload: { environment: environment }) - else + rescue StandardError => err ServiceResponse.error( - message: environment.errors.full_messages, + message: [err.message], payload: { environment: environment } ) end diff --git a/spec/services/environments/create_service_spec.rb b/spec/services/environments/create_service_spec.rb index 7210ed3524626a..ea24a9b5cb973d 100644 --- a/spec/services/environments/create_service_spec.rb +++ b/spec/services/environments/create_service_spec.rb @@ -78,7 +78,23 @@ response = subject expect(response).to be_error - expect(response.message).to match_array("External url URI is invalid") + expect(response.message).to match_array("Validation failed: External url URI is invalid") + expect(response.payload[:environment]).to be_nil + end + end + + context 'when params contain invalid auto_stop_setting' do + let(:params) { { name: 'production', auto_stop_setting: :invalid } } + + it 'does not create an environment' do + expect { subject }.not_to change { ::Environment.count } + end + + it 'returns an error' do + response = subject + + expect(response).to be_error + expect(response.message).to match_array("'invalid' is not a valid auto_stop_setting") expect(response.payload[:environment]).to be_nil end end diff --git a/spec/services/environments/update_service_spec.rb b/spec/services/environments/update_service_spec.rb index 6ccbbf50fa8215..d3c08c1c4627f1 100644 --- a/spec/services/environments/update_service_spec.rb +++ b/spec/services/environments/update_service_spec.rb @@ -95,7 +95,19 @@ response = subject expect(response).to be_error - expect(response.message).to match_array("External url URI is invalid") + expect(response.message).to match_array("Validation failed: External url URI is invalid") + expect(response.payload[:environment]).to eq(environment) + end + end + + context 'when params contain invalid auto_stop_setting' do + let(:params) { { auto_stop_setting: :invalid } } + + it 'returns an error' do + response = subject + + expect(response).to be_error + expect(response.message).to match_array("'invalid' is not a valid auto_stop_setting") expect(response.payload[:environment]).to eq(environment) end end -- GitLab From ad840e17576cc0b79a77dbd5c94f4265ddc923b2 Mon Sep 17 00:00:00 2001 From: Taka Nishida Date: Thu, 19 Dec 2024 14:17:23 +0900 Subject: [PATCH 3/6] Update tests error message --- spec/graphql/mutations/environments/create_spec.rb | 2 +- spec/graphql/mutations/environments/update_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/graphql/mutations/environments/create_spec.rb b/spec/graphql/mutations/environments/create_spec.rb index 128fa5502f2781..0ddc7eebda005a 100644 --- a/spec/graphql/mutations/environments/create_spec.rb +++ b/spec/graphql/mutations/environments/create_spec.rb @@ -35,7 +35,7 @@ expect(subject) .to eq({ environment: nil, - errors: ['External url URI is invalid'] + errors: ['Validation failed: External url URI is invalid'] }) end end diff --git a/spec/graphql/mutations/environments/update_spec.rb b/spec/graphql/mutations/environments/update_spec.rb index 7c60b5f6da0881..de0f78dee3a97f 100644 --- a/spec/graphql/mutations/environments/update_spec.rb +++ b/spec/graphql/mutations/environments/update_spec.rb @@ -36,7 +36,7 @@ expect(subject) .to eq({ environment: environment, - errors: ['External url URI is invalid'] + errors: ['Validation failed: External url URI is invalid'] }) end end -- GitLab From 449e02ca3c4d5513e96fe23e2ba062d0407be4dc Mon Sep 17 00:00:00 2001 From: Taka Nishida Date: Fri, 20 Dec 2024 13:07:34 +0900 Subject: [PATCH 4/6] Specify the error tyeps that should be rescued --- app/services/environments/create_service.rb | 2 +- app/services/environments/update_service.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/environments/create_service.rb b/app/services/environments/create_service.rb index ba27b228b65389..209508f17449f9 100644 --- a/app/services/environments/create_service.rb +++ b/app/services/environments/create_service.rb @@ -22,7 +22,7 @@ def execute begin environment = project.environments.create!(**params.slice(*ALLOWED_ATTRIBUTES)) ServiceResponse.success(payload: { environment: environment }) - rescue StandardError => err + rescue ActiveRecord::RecordInvalid, ArgumentError => err ServiceResponse.error( message: [err.message], payload: { environment: nil } diff --git a/app/services/environments/update_service.rb b/app/services/environments/update_service.rb index fc16047e3b43c5..26567a25ee9106 100644 --- a/app/services/environments/update_service.rb +++ b/app/services/environments/update_service.rb @@ -22,7 +22,7 @@ def execute(environment) begin environment.update!(**params.slice(*ALLOWED_ATTRIBUTES)) ServiceResponse.success(payload: { environment: environment }) - rescue StandardError => err + rescue ActiveRecord::RecordInvalid, ArgumentError => err ServiceResponse.error( message: [err.message], payload: { environment: environment } -- GitLab From 96b6c80f573408843b0873d122e6e662eca3ef25 Mon Sep 17 00:00:00 2001 From: Taka Nishida Date: Fri, 10 Jan 2025 19:11:12 +0900 Subject: [PATCH 5/6] Fix: Return multiple error messages when RecordInvalid --- app/services/environments/create_service.rb | 9 ++++----- spec/services/environments/create_service_spec.rb | 7 ++++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/services/environments/create_service.rb b/app/services/environments/create_service.rb index 209508f17449f9..8a576c923287e7 100644 --- a/app/services/environments/create_service.rb +++ b/app/services/environments/create_service.rb @@ -22,11 +22,10 @@ def execute begin environment = project.environments.create!(**params.slice(*ALLOWED_ATTRIBUTES)) ServiceResponse.success(payload: { environment: environment }) - rescue ActiveRecord::RecordInvalid, ArgumentError => err - ServiceResponse.error( - message: [err.message], - payload: { environment: nil } - ) + rescue ActiveRecord::RecordInvalid => err + ServiceResponse.error(message: err.record.errors.full_messages, payload: { environment: nil }) + rescue ArgumentError => err + ServiceResponse.error(message: [err.message], payload: { environment: nil }) end end diff --git a/spec/services/environments/create_service_spec.rb b/spec/services/environments/create_service_spec.rb index ea24a9b5cb973d..5e57311e9ec3b0 100644 --- a/spec/services/environments/create_service_spec.rb +++ b/spec/services/environments/create_service_spec.rb @@ -67,8 +67,8 @@ end end - context 'when params contain invalid value' do - let(:params) { { name: 'production', external_url: 'http://${URL}' } } + context 'when params contain invalid values' do + let(:params) { { name: 'production', external_url: 'http://${URL}', kubernetes_namespace: "invalid" } } it 'does not create an environment' do expect { subject }.not_to change { ::Environment.count } @@ -78,7 +78,8 @@ response = subject expect(response).to be_error - expect(response.message).to match_array("Validation failed: External url URI is invalid") + expect(response.message).to match_array(["External url URI is invalid", + "Kubernetes namespace cannot be set without a cluster agent"]) expect(response.payload[:environment]).to be_nil end end -- GitLab From ff5dcae0a3a1f8ad1aff3e8d3133515959acc510 Mon Sep 17 00:00:00 2001 From: Taka Nishida Date: Fri, 10 Jan 2025 19:51:34 +0900 Subject: [PATCH 6/6] Fix: Return all error messages when updating the environment --- app/services/environments/update_service.rb | 9 ++++----- spec/graphql/mutations/environments/create_spec.rb | 2 +- spec/graphql/mutations/environments/update_spec.rb | 2 +- spec/services/environments/update_service_spec.rb | 7 ++++--- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/services/environments/update_service.rb b/app/services/environments/update_service.rb index 26567a25ee9106..eceb7ab46697ca 100644 --- a/app/services/environments/update_service.rb +++ b/app/services/environments/update_service.rb @@ -22,11 +22,10 @@ def execute(environment) begin environment.update!(**params.slice(*ALLOWED_ATTRIBUTES)) ServiceResponse.success(payload: { environment: environment }) - rescue ActiveRecord::RecordInvalid, ArgumentError => err - ServiceResponse.error( - message: [err.message], - payload: { environment: environment } - ) + rescue ActiveRecord::RecordInvalid => err + ServiceResponse.error(message: err.record.errors.full_messages, payload: { environment: environment }) + rescue ArgumentError => err + ServiceResponse.error(message: [err.message], payload: { environment: environment }) end end diff --git a/spec/graphql/mutations/environments/create_spec.rb b/spec/graphql/mutations/environments/create_spec.rb index 0ddc7eebda005a..128fa5502f2781 100644 --- a/spec/graphql/mutations/environments/create_spec.rb +++ b/spec/graphql/mutations/environments/create_spec.rb @@ -35,7 +35,7 @@ expect(subject) .to eq({ environment: nil, - errors: ['Validation failed: External url URI is invalid'] + errors: ['External url URI is invalid'] }) end end diff --git a/spec/graphql/mutations/environments/update_spec.rb b/spec/graphql/mutations/environments/update_spec.rb index de0f78dee3a97f..7c60b5f6da0881 100644 --- a/spec/graphql/mutations/environments/update_spec.rb +++ b/spec/graphql/mutations/environments/update_spec.rb @@ -36,7 +36,7 @@ expect(subject) .to eq({ environment: environment, - errors: ['Validation failed: External url URI is invalid'] + errors: ['External url URI is invalid'] }) end end diff --git a/spec/services/environments/update_service_spec.rb b/spec/services/environments/update_service_spec.rb index d3c08c1c4627f1..45dbf837b224a0 100644 --- a/spec/services/environments/update_service_spec.rb +++ b/spec/services/environments/update_service_spec.rb @@ -88,14 +88,15 @@ end end - context 'when params contain invalid value' do - let(:params) { { external_url: 'http://${URL}' } } + context 'when params contain invalid values' do + let(:params) { { external_url: 'http://${URL}', kubernetes_namespace: "invalid" } } it 'returns an error' do response = subject expect(response).to be_error - expect(response.message).to match_array("Validation failed: External url URI is invalid") + expect(response.message).to match_array(["External url URI is invalid", + "Kubernetes namespace cannot be set without a cluster agent"]) expect(response.payload[:environment]).to eq(environment) end end -- GitLab