From b58949dc8dd43fbfecc3da2ce0489eeeca1c1c9b Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Tue, 22 Apr 2025 19:05:46 -0400 Subject: [PATCH 1/5] Update variable args in workspaceCreate mutation - Deprecate the variables arg - Add new worskpace_variables arg - Update graphql docs Changelog: changed EE: true --- doc/api/graphql/reference/_index.md | 3 ++- .../remote_development/workspace_operations/create.rb | 11 ++++++++++- .../workspace_operations/create_spec.rb | 7 ------- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/doc/api/graphql/reference/_index.md b/doc/api/graphql/reference/_index.md index 9a8d024e821027..d4a975f51c66e9 100644 --- a/doc/api/graphql/reference/_index.md +++ b/doc/api/graphql/reference/_index.md @@ -12958,7 +12958,8 @@ Input type: `WorkspaceCreateInput` | `maxHoursBeforeTermination` {{< icon name="warning-solid" >}} | [`Int`](#int) | **Deprecated:** Field is not used. Deprecated in GitLab 17.9. | | `projectId` | [`ProjectID!`](#projectid) | ID of the project that will provide the Devfile for the created workspace. | | `projectRef` | [`String`](#string) | Project repo git ref. | -| `variables` | [`[WorkspaceVariableInput!]`](#workspacevariableinput) | Variables to inject into the workspace. | +| `variables` {{< icon name="warning-solid" >}} | [`[WorkspaceVariableInput!]`](#workspacevariableinput) | **Deprecated:** Argument is renamed to workspace_variables. Deprecated in GitLab 18.0. | +| `workspaceVariables` {{< icon name="warning-solid" >}} | [`[WorkspaceVariableInput!]`](#workspacevariableinput) | **Deprecated:** **Status**: Experiment. Introduced in GitLab 18.0. | #### Fields diff --git a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb index 71b89876c58a02..82cf340787b0ea 100644 --- a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb +++ b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb @@ -67,6 +67,14 @@ class Create < BaseMutation required: false, default_value: [], replace_null_with_default: true, + description: 'Variables to inject into the workspace.', + deprecated: { reason: 'Argument is renamed to workspace_variables', milestone: '18.0' } + + argument :workspace_variables, [::Types::RemoteDevelopment::WorkspaceVariableInput], + required: false, + default_value: [], + replace_null_with_default: true, + experiment: { milestone: '18.0' }, description: 'Variables to inject into the workspace.' # @param [Hash] args @@ -123,7 +131,8 @@ def resolve(args) # noinspection RubyNilAnalysis - This is because the superclass #current_user uses #[], which can return nil track_usage_event(:users_creating_workspaces, current_user.id) - variables = args.fetch(:variables, []).map(&:to_h) + workspace_variables = args.delete(:workspace_variables) + variables = (args[:variables] || workspace_variables || []).map(&:to_h) params = args.merge( agent: agent, user: current_user, diff --git a/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb b/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb index 647485ba47d674..f5ba195fcafd14 100644 --- a/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb @@ -39,13 +39,6 @@ let(:desired_state) { RemoteDevelopment::WorkspaceOperations::States::RUNNING } let(:devfile_path) { '.devfile.yaml' } - let(:mutation_expected_varaiables) do - [ - { key: 'VAR1', value: 'value 1', type: 'ENVIRONMENT', variable_type: 'ENVIRONMENT' }, - { key: 'VAR2', value: 'value 2', type: 'ENVIRONMENT', variable_type: 'ENVIRONMENT' } - ] - end - let(:service_class_expected_variables) do [ { key: 'VAR1', value: 'value 1', type: 'ENVIRONMENT', variable_type: 0 }, -- GitLab From ce4eb09381ca70268d452a8a7155ddbae5c272a1 Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Wed, 23 Apr 2025 14:57:51 -0400 Subject: [PATCH 2/5] Refactor logic for setting variables in resolve method --- .../remote_development/workspace_operations/create.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb index 82cf340787b0ea..23106a89c90dcd 100644 --- a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb +++ b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb @@ -132,7 +132,9 @@ def resolve(args) track_usage_event(:users_creating_workspaces, current_user.id) workspace_variables = args.delete(:workspace_variables) - variables = (args[:variables] || workspace_variables || []).map(&:to_h) + variables_array = workspace_variables.presence || args.fetch(:variables, []) + + variables = variables_array.map(&:to_h) params = args.merge( agent: agent, user: current_user, -- GitLab From cd69860a837262d894b902408f8f573efc0443e2 Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Thu, 24 Apr 2025 23:21:35 -0400 Subject: [PATCH 3/5] Add test cases in create_spec.rb --- .../workspace_operations/create.rb | 3 +- .../workspace_operations/create_spec.rb | 65 ++++++++++++++++--- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb index 23106a89c90dcd..00c70692dfe259 100644 --- a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb +++ b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb @@ -131,8 +131,7 @@ def resolve(args) # noinspection RubyNilAnalysis - This is because the superclass #current_user uses #[], which can return nil track_usage_event(:users_creating_workspaces, current_user.id) - workspace_variables = args.delete(:workspace_variables) - variables_array = workspace_variables.presence || args.fetch(:variables, []) + variables_array = args[:workspace_variables].presence || args.fetch(:variables, []) variables = variables_array.map(&:to_h) params = args.merge( diff --git a/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb b/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb index f5ba195fcafd14..cab87d88c9a175 100644 --- a/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb @@ -38,14 +38,24 @@ let(:desired_state) { RemoteDevelopment::WorkspaceOperations::States::RUNNING } let(:devfile_path) { '.devfile.yaml' } + let(:mutation_variables_arg) do + [ + { key: 'MV1', value: 'value 1', type: 'ENVIRONMENT' }, + { key: 'MV2', value: 'value 2', type: 'ENVIRONMENT' } + ] + end - let(:service_class_expected_variables) do + let(:mutation_workspace_variables_arg) do [ - { key: 'VAR1', value: 'value 1', type: 'ENVIRONMENT', variable_type: 0 }, - { key: 'VAR2', value: 'value 2', type: 'ENVIRONMENT', variable_type: 0 } + { key: 'MWV1', value: 'value 1', type: 'ENVIRONMENT' }, + { key: 'MWV2', value: 'value 2', type: 'ENVIRONMENT' } ] end + let(:service_class_expected_variables) do + [] + end + let(:all_mutation_args) do { desired_state: desired_state, @@ -53,11 +63,7 @@ cluster_agent_id: agent.to_global_id.to_s, project_id: workspace_project.to_global_id.to_s, project_ref: 'main', - devfile_path: devfile_path, - variables: [ - { key: 'VAR1', value: 'value 1', type: 'ENVIRONMENT' }, - { key: 'VAR2', value: 'value 2', type: 'ENVIRONMENT' } - ] + devfile_path: devfile_path } end @@ -98,6 +104,13 @@ def mutation_response graphql_mutation_response(:workspace_create) end + # @param [Array] variables + # @param [Integer] type + # @return [Array] + def add_variable_type(variables, type = 0) + variables.map { |var| var.dup.merge(variable_type: type) } + end + before do stub_licensed_features(remote_development: true) @@ -172,6 +185,42 @@ def mutation_response it_behaves_like 'successful create' end + # TODO: Once the deprectated `variables` argument is removed, refactor this and only check for workspace_variables + context 'when variables is not present and workspace_variables is present' do + let(:mutation_args) { all_mutation_args.merge(workspace_variables: mutation_workspace_variables_arg) } + let(:service_class_expected_variables) do + add_variable_type(mutation_workspace_variables_arg) + end + + it_behaves_like 'successful create' + end + + # TODO: Once the deprectated `variables` argument is removed, delete this context + context 'when workspace_variables is not present and variables is present' do + let(:mutation_args) { all_mutation_args.merge(variables: mutation_variables_arg) } + let(:service_class_expected_variables) do + add_variable_type(mutation_variables_arg) + end + + it_behaves_like 'successful create' + end + + # TODO: Once the deprectated `variables` argument is removed, delete this context + context 'when workspace_variables and variables are both present' do + let(:mutation_args) do + all_mutation_args.merge( + workspace_variables: mutation_workspace_variables_arg, + variables: mutation_variables_arg + ) + end + + let(:service_class_expected_variables) do + add_variable_type(mutation_workspace_variables_arg) + end + + it_behaves_like 'successful create' + end + context "when the agent project no longer exists under the namespace it is mapped to" do before do agent_project.project_namespace.update!(parent: root_namespace) -- GitLab From 47d7b47c3b80b64c5e5b7cf52a97595c5c6b9c5f Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Fri, 25 Apr 2025 11:43:26 -0400 Subject: [PATCH 4/5] Delete workspace_variables arg in mutation resolve method --- .../remote_development/workspace_operations/create.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb index 00c70692dfe259..23106a89c90dcd 100644 --- a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb +++ b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb @@ -131,7 +131,8 @@ def resolve(args) # noinspection RubyNilAnalysis - This is because the superclass #current_user uses #[], which can return nil track_usage_event(:users_creating_workspaces, current_user.id) - variables_array = args[:workspace_variables].presence || args.fetch(:variables, []) + workspace_variables = args.delete(:workspace_variables) + variables_array = workspace_variables.presence || args.fetch(:variables, []) variables = variables_array.map(&:to_h) params = args.merge( -- GitLab From d6a2601c8c1d8b2520bbec2af39cfe7df97d58fc Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Tue, 29 Apr 2025 15:06:41 -0400 Subject: [PATCH 5/5] Apply review patch --- .../workspace_operations/create.rb | 10 +- .../workspace_operations/create_spec.rb | 150 ++++++++---------- 2 files changed, 73 insertions(+), 87 deletions(-) diff --git a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb index 23106a89c90dcd..6491e0a9cc0601 100644 --- a/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb +++ b/ee/app/graphql/mutations/remote_development/workspace_operations/create.rb @@ -101,6 +101,12 @@ def resolve(args) devfile_ref = args.delete(:devfile_ref) args[:project_ref] = devfile_ref if args[:project_ref].nil? + # If 'workspace_variables' is not specified, use 'variables' arg for backward compatibility. + workspace_variables = args.delete(:workspace_variables) + variables_array = workspace_variables.presence || args.fetch(:variables, []) + + variables = variables_array.map(&:to_h) + # NOTE: What the following line actually does - the agent is delegating to the project to check that the user # has the :create_workspace ability on the _agent's_ project, which will be true if the user is a developer # on the agent's project. @@ -131,10 +137,6 @@ def resolve(args) # noinspection RubyNilAnalysis - This is because the superclass #current_user uses #[], which can return nil track_usage_event(:users_creating_workspaces, current_user.id) - workspace_variables = args.delete(:workspace_variables) - variables_array = workspace_variables.presence || args.fetch(:variables, []) - - variables = variables_array.map(&:to_h) params = args.merge( agent: agent, user: current_user, diff --git a/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb b/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb index cab87d88c9a175..db6a6dc576f365 100644 --- a/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb @@ -38,43 +38,42 @@ let(:desired_state) { RemoteDevelopment::WorkspaceOperations::States::RUNNING } let(:devfile_path) { '.devfile.yaml' } - let(:mutation_variables_arg) do - [ - { key: 'MV1', value: 'value 1', type: 'ENVIRONMENT' }, - { key: 'MV2', value: 'value 2', type: 'ENVIRONMENT' } - ] - end - let(:mutation_workspace_variables_arg) do + let(:variables) do [ - { key: 'MWV1', value: 'value 1', type: 'ENVIRONMENT' }, - { key: 'MWV2', value: 'value 2', type: 'ENVIRONMENT' } + { key: 'VAR1', value: 'value 1', type: 'ENVIRONMENT', variable_type: 'ENVIRONMENT' }, + { key: 'VAR2', value: 'value 2', type: 'ENVIRONMENT', variable_type: 'ENVIRONMENT' } ] end let(:service_class_expected_variables) do - [] + [ + { key: 'VAR1', value: 'value 1', type: 'ENVIRONMENT', variable_type: 0 }, + { key: 'VAR2', value: 'value 2', type: 'ENVIRONMENT', variable_type: 0 } + ] end - let(:all_mutation_args) do + let(:base_mutation_args) do { desired_state: desired_state, editor: 'webide', cluster_agent_id: agent.to_global_id.to_s, project_id: workspace_project.to_global_id.to_s, project_ref: 'main', - devfile_path: devfile_path + devfile_path: devfile_path, + workspace_variables: variables } end - let(:mutation_args) { all_mutation_args } - - let(:mutation) do - graphql_mutation(:workspace_create, mutation_args) - end + let(:expected_service_params) do + params = { + desired_state: desired_state, + editor: 'webide', + project_ref: 'main', + devfile_path: devfile_path, + variables: variables + } - let(:expected_service_args) do - params = all_mutation_args.except(:cluster_agent_id, :project_id) # noinspection RubyMismatchedArgumentType - RubyMine is misinterpreting types for Hash values params[:variables] = service_class_expected_variables # noinspection RubyMismatchedArgumentType - RubyMine is misinterpreting types for Hash values @@ -83,11 +82,21 @@ # noinspection RubyMismatchedArgumentType - RubyMine is misinterpreting types for Hash values params[:project] = workspace_project + params + end + + let(:mutation_args) { base_mutation_args } + + let(:mutation) do + graphql_mutation(:workspace_create, mutation_args) + end + + let(:expected_service_args) do { domain_main_class: ::RemoteDevelopment::WorkspaceOperations::Create::Main, domain_main_class_args: { user: current_user, - params: params, + params: expected_service_params, vscode_extension_marketplace_metadata: { enabled: true }, vscode_extension_marketplace: { some_setting: "some-value" } } @@ -104,13 +113,6 @@ def mutation_response graphql_mutation_response(:workspace_create) end - # @param [Array] variables - # @param [Integer] type - # @return [Array] - def add_variable_type(variables, type = 0) - variables.map { |var| var.dup.merge(variable_type: type) } - end - before do stub_licensed_features(remote_development: true) @@ -160,65 +162,19 @@ def add_variable_type(variables, type = 0) it_behaves_like 'successful create' end - context 'when devfile_path is nil' do - let(:devfile_path) { nil } - - it_behaves_like 'successful create' - end - - context 'when devfile_path is not present' do - let(:devfile_path) { nil } - let(:mutation_args) { all_mutation_args.except(:devfile_path) } - - it_behaves_like 'successful create' - end - - context 'when project_ref is not present and devfile_ref is present' do - let(:mutation_args) { all_mutation_args.except(:project_ref).merge(devfile_ref: 'main') } + describe 'devfile_path behavior' do + context 'when devfile_path is nil' do + let(:devfile_path) { nil } - it_behaves_like 'successful create' - end - - context 'when project_ref and devfile_ref are both present' do - let(:mutation_args) { all_mutation_args.merge(devfile_ref: 'main1') } - - it_behaves_like 'successful create' - end - - # TODO: Once the deprectated `variables` argument is removed, refactor this and only check for workspace_variables - context 'when variables is not present and workspace_variables is present' do - let(:mutation_args) { all_mutation_args.merge(workspace_variables: mutation_workspace_variables_arg) } - let(:service_class_expected_variables) do - add_variable_type(mutation_workspace_variables_arg) - end - - it_behaves_like 'successful create' - end - - # TODO: Once the deprectated `variables` argument is removed, delete this context - context 'when workspace_variables is not present and variables is present' do - let(:mutation_args) { all_mutation_args.merge(variables: mutation_variables_arg) } - let(:service_class_expected_variables) do - add_variable_type(mutation_variables_arg) + it_behaves_like 'successful create' end - it_behaves_like 'successful create' - end - - # TODO: Once the deprectated `variables` argument is removed, delete this context - context 'when workspace_variables and variables are both present' do - let(:mutation_args) do - all_mutation_args.merge( - workspace_variables: mutation_workspace_variables_arg, - variables: mutation_variables_arg - ) - end + context 'when devfile_path is not present' do + let(:devfile_path) { nil } + let(:mutation_args) { base_mutation_args.except(:devfile_path) } - let(:service_class_expected_variables) do - add_variable_type(mutation_workspace_variables_arg) + it_behaves_like 'successful create' end - - it_behaves_like 'successful create' end context "when the agent project no longer exists under the namespace it is mapped to" do @@ -244,6 +200,34 @@ def add_variable_type(variables, type = 0) it_behaves_like 'a mutation that returns errors in the response', errors: ['some error'] end + + describe 'deprecated fields behavior' do + context 'when project_ref is not present and devfile_ref is present' do + let(:mutation_args) do + base_mutation_args.except(:project_ref).merge(devfile_ref: 'main') + end + + it_behaves_like 'successful create' + end + + context 'when project_ref and devfile_ref are both present' do + let(:mutation_args) { base_mutation_args.merge(devfile_ref: 'main1') } + + it_behaves_like 'successful create' + end + + context 'when workspace_variables is not present and variables is present' do + let(:mutation_args) { base_mutation_args.except(:workspace_variables).merge(variables: variables) } + + it_behaves_like 'successful create' + end + + context 'when workspace_variables and variables are both present' do + let(:mutation_args) { base_mutation_args.merge(variables: variables) } + + it_behaves_like 'successful create' + end + end end context 'when workspace project and agent project are NOT in the same root namespace' do @@ -267,7 +251,7 @@ def add_variable_type(variables, type = 0) context 'when required arguments are missing' do context 'when validates against GraphQL not allow null behaviour' do - let(:mutation_args) { all_mutation_args.except(:desired_state) } + let(:mutation_args) { base_mutation_args.except(:desired_state) } it 'returns error about required argument' do post_graphql_mutation(mutation, current_user: user) @@ -277,7 +261,7 @@ def add_variable_type(variables, type = 0) end context 'when both project_ref and devfile_ref not present' do - let(:mutation_args) { all_mutation_args.except(:project_ref, :devfile_ref) } + let(:mutation_args) { base_mutation_args.except(:project_ref, :devfile_ref) } it 'returns error about required argument' do post_graphql_mutation(mutation, current_user: user) -- GitLab