From 4bc2c762b5d347acc282f494006df9a68884b06b Mon Sep 17 00:00:00 2001 From: Chad Woolley Date: Fri, 13 Dec 2024 22:29:02 +0530 Subject: [PATCH 1/2] Refactor workspace constants, constant usage, and related code/docs --- ee/lib/remote_development/README.md | 32 ++++++++++++++----- .../create/create_constants.rb | 29 +++++++++++++++++ .../workspace_operations/create/creator.rb | 3 +- .../create/main_component_updater.rb | 16 ++++++---- .../create/post_flatten_devfile_validator.rb | 14 +++++--- .../project_cloner_component_inserter.rb | 6 ++-- .../tools_injector_component_inserter.rb | 17 ++++------ .../create/volume_component_inserter.rb | 2 -- .../create/volume_definer.rb | 12 ++++--- .../create/workspace_variables.rb | 14 ++++---- .../workspace_operations/file_mounts.rb | 13 -------- .../reconcile/input/params_extractor.rb | 2 -- .../input/params_to_infos_converter.rb | 2 -- .../workspace_operations/reconcile/main.rb | 2 +- .../reconcile/output/devfile_parser.rb | 5 +-- .../output/response_payload_builder.rb | 1 - .../reconcile/reconcile_constants.rb | 20 ++++++++++++ .../workspace_operations_constants.rb | 23 +++++++++++++ .../remote_development/workspaces.rb | 5 ++- .../create/creator_spec.rb | 3 +- .../create/main_component_updater_spec.rb | 7 ++-- .../create/main_integration_spec.rb | 7 ++-- .../post_flatten_devfile_validator_spec.rb | 8 +++-- .../project_cloner_component_inserter_spec.rb | 1 - .../tools_injector_component_inserter_spec.rb | 7 ++-- .../create/volume_component_inserter_spec.rb | 3 +- .../create/volume_definer_spec.rb | 7 +++- .../create/workspace_creator_spec.rb | 5 +-- .../remote_development/integration_spec.rb | 3 +- .../remote_development_shared_contexts.rb | 8 +++-- locale/gitlab.pot | 4 +-- 31 files changed, 185 insertions(+), 96 deletions(-) create mode 100644 ee/lib/remote_development/workspace_operations/create/create_constants.rb delete mode 100644 ee/lib/remote_development/workspace_operations/file_mounts.rb create mode 100644 ee/lib/remote_development/workspace_operations/reconcile/reconcile_constants.rb create mode 100644 ee/lib/remote_development/workspace_operations/workspace_operations_constants.rb diff --git a/ee/lib/remote_development/README.md b/ee/lib/remote_development/README.md index 7c7ae0d5a376d1..b65734c0552886 100644 --- a/ee/lib/remote_development/README.md +++ b/ee/lib/remote_development/README.md @@ -21,13 +21,14 @@ - [Higher order functions](#higher-order-functions) - [Pure functions](#pure-functions) - [Concurrency and parallelism](#concurrency-and-parallelism) - - [Error Handling](#error-handling) + - [Error handling](#error-handling) - [Object-Oriented patterns](#object-oriented-patterns) - - [Value Objects](#value-objects) + - [Value objects](#value-objects) - [Code sharing patterns and DRYness](#code-sharing-patterns-and-dryness) -- [Other Patterns](#other-patterns) - - [Inversion of Control](#inversion-of-control) +- [Other patterns](#other-patterns) + - [Inversion of control](#inversion-of-control) - [Metaprogramming](#metaprogramming) + - [Constant declarations](#constant-declarations) - [Railway Oriented Programming and the Result class](#railway-oriented-programming-and-the-result-class) - [Result class](#result-class) - [Message class and Messages module](#message-class-and-messages-module) @@ -265,7 +266,7 @@ By using patterns such as immutable state and pure functions, we are able to sup This may be useful in the future for the Remote Development feature, as operations such as reconciliation of workspace state involve processing data for many independent workspaces in a single request. -### Error Handling +### Error handling The domain logic of the Remote Development feature is based on the ["Railway Oriented Programming"](https://fsharpforfunandprofit.com/rop/) pattern, through the usage of a standard [`Result` class](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/fp/result.rb) as found in many programming languages (ours is based on the [Rust implementation](https://doc.rust-lang.org/std/result/index.html)). @@ -276,7 +277,7 @@ This Railway Oriented Programming pattern allows us to keep the business logic d Although the functional patterns above are used when they provide benefits, we otherwise still try to adhere to standard OO/Ruby/Rails idioms and patterns, for example: -### Value Objects +### Value objects When we need to pass data around, we encapsulate it in objects. This may be a standard libary class such as `Hash` or `String`, or it may be a custom class which we create. @@ -298,9 +299,9 @@ In other cases, we do not use a mixin/module, but instead directly call a class/ However, we do use inheritance in the higher layers of the architecture where this is necessary confirm with existing patterns, e.g. in controllers or API classes. -## Other Patterns +## Other patterns -### Inversion of Control +### Inversion of control We use the pattern of "Inversion of Control" when applicable, to help achieve loose coupling between modules which implement business logic. @@ -316,6 +317,21 @@ See more details and examples of this in the sections below. We _currently_ do not make heavy use of metaprogramming. But, we may make use of it in the future in areas where it makes sense. +### Constant declarations + +- If a constant is only used in a single class or module, it are declared within that class/module +- If constants are used by multiple classes or modules, they are grouped in a single class at each Ruby module/namespace level corresponding to the domain use-case, at the lowest level at which they are used. + For example, constants used only within `RemoteDevelopment::WorkspaceOperations::Create` will all be declared in `RemoteDevelopment::WorkspaceOperations::Create::CreateConstants` class. + Likewise, constants shared across multiple sub-modules of `RemoteDevelopment::WorkspaceOperations` will be declared in `RemoteDevelopment::WorkspaceOperations::WorkspaceOperationConstants` + - We could split these up in more specific classes (e.g. `FileConstants`, `EnvVarConstants`, etc.), but this results in several more classes in the domain code files, without much benefit. + Grouping them in fewer files is easier, but scoping them by namespace still gives an indication of the scope where they are used. + Note that there may be some exceptions to this. + If we have many constants of a single category, we might decide to put them in their own file. + - Even though we could name all the classes just `Constants`, naming them uniquely after their corresponding namespace level makes it easier to find them, and understand the scope to which they are limited. + - Constants within a single file should be kept alphabetized. This is so no thought is required on how to group them - if grouping is desired, name them with the same prefix. + - Spec/factory code may refer to constants from different modules than their own if necessary. The scoping rules only apply to usages within production code. +- Do not declare string constants which are only interpolated into other string constants, and are not used anywhere else. Instead, declare a single constant as single string. + ## Railway Oriented Programming and the Result class The Domain Logic layer uses the "Railway Oriented Programming" pattern (AKA "ROP"), which is [explained here in a presentation and video](https://fsharpforfunandprofit.com/rop/) by Scott Wlaschin. The presentation slides on that page give an overview which explains the motivation and implementation of this pattern. diff --git a/ee/lib/remote_development/workspace_operations/create/create_constants.rb b/ee/lib/remote_development/workspace_operations/create/create_constants.rb new file mode 100644 index 00000000000000..0660e8f5720252 --- /dev/null +++ b/ee/lib/remote_development/workspace_operations/create/create_constants.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module WorkspaceOperations + module Create + # NOTE: Constants are scoped to the use-case namespace in which they are used in production + # code (but they may still be referenced by specs or fixtures or factories). + # For example, this RemoteDevelopment::WorkspaceOperations::Create::CreateConstants + # file contains constants which are only used by classes within that namespace. + # + # See documentation at ../../README.md#constant-declarations for more information. + module CreateConstants + include WorkspaceOperationsConstants + + # Please keep alphabetized + GIT_CREDENTIAL_STORE_FILE = "#{VARIABLES_FILE_DIR}/gl_git_credential_store.sh".freeze + MAIN_COMPONENT_INDICATOR_ATTRIBUTE = "gl/inject-editor" + NAMESPACE_PREFIX = "gl-rd-ns" + PROJECT_CLONER_COMPONENT_NAME = "gl-project-cloner" + TOKEN_FILE = "#{VARIABLES_FILE_DIR}/gl_token".freeze + TOOLS_DIR_NAME = ".gl-tools" + TOOLS_DIR_ENV_VAR = "GL_TOOLS_DIR" + TOOLS_INJECTOR_COMPONENT_NAME = "gl-tools-injector" + WORKSPACE_DATA_VOLUME_NAME = "gl-workspace-data" + WORKSPACE_DATA_VOLUME_PATH = "/projects" + end + end + end +end diff --git a/ee/lib/remote_development/workspace_operations/create/creator.rb b/ee/lib/remote_development/workspace_operations/create/creator.rb index ecc06d134f1932..5f65fd41783c9a 100644 --- a/ee/lib/remote_development/workspace_operations/create/creator.rb +++ b/ee/lib/remote_development/workspace_operations/create/creator.rb @@ -4,6 +4,7 @@ module RemoteDevelopment module WorkspaceOperations module Create class Creator + include CreateConstants include Messages RANDOM_STRING_LENGTH = 6 @@ -22,7 +23,7 @@ def self.create(context) # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/409774 # We can come maybe come up with a better/cooler way to get a unique name, for now this works context[:workspace_name] = "workspace-#{agent.id}-#{user.id}-#{random_string}" - context[:workspace_namespace] = "gl-rd-ns-#{agent.id}-#{user.id}-#{random_string}" + context[:workspace_namespace] = "#{NAMESPACE_PREFIX}-#{agent.id}-#{user.id}-#{random_string}" model_errors = nil updated_value = ApplicationRecord.transaction do diff --git a/ee/lib/remote_development/workspace_operations/create/main_component_updater.rb b/ee/lib/remote_development/workspace_operations/create/main_component_updater.rb index 7996961c61a44c..484765736d9d28 100644 --- a/ee/lib/remote_development/workspace_operations/create/main_component_updater.rb +++ b/ee/lib/remote_development/workspace_operations/create/main_component_updater.rb @@ -4,26 +4,26 @@ module RemoteDevelopment module WorkspaceOperations module Create class MainComponentUpdater - include Messages + include CreateConstants # @param [Hash] context # @return [Hash] def self.update(context) context => { processed_devfile: Hash => processed_devfile, - volume_mounts: Hash => volume_mounts, + tools_dir: String => tools_dir, vscode_extensions_gallery_metadata: Hash => vscode_extensions_gallery_metadata } - volume_mounts => { data_volume: Hash => data_volume } - data_volume => { path: String => volume_path } editor_port = WorkspaceCreator::WORKSPACE_PORT ssh_port = 60022 - tools_dir = "#{volume_path}/.gl-tools" # NOTE: We will always have exactly one main_component found, because we have already # validated this in post_flatten_devfile_validator.rb - main_component = processed_devfile.fetch(:components).find { |c| c.dig(:attributes, :'gl/inject-editor') } + main_component = + processed_devfile + .fetch(:components) + .find { |component| component.dig(:attributes, MAIN_COMPONENT_INDICATOR_ATTRIBUTE.to_sym) } container = main_component.fetch(:container) @@ -57,7 +57,9 @@ def self.update(context) def self.update_env_vars(container:, tools_dir:, editor_port:, ssh_port:, enable_marketplace:) (container[:env] ||= []).append( { - name: "GL_TOOLS_DIR", + # NOTE: Only "TOOLS_DIR" env var is extracted to a constant, because it is the only one referenced + # in multiple different classes. + name: TOOLS_DIR_ENV_VAR, value: tools_dir }, { diff --git a/ee/lib/remote_development/workspace_operations/create/post_flatten_devfile_validator.rb b/ee/lib/remote_development/workspace_operations/create/post_flatten_devfile_validator.rb index c1d3ab8fcc0d8f..f264cc3d2f6c6c 100644 --- a/ee/lib/remote_development/workspace_operations/create/post_flatten_devfile_validator.rb +++ b/ee/lib/remote_development/workspace_operations/create/post_flatten_devfile_validator.rb @@ -4,6 +4,7 @@ module RemoteDevelopment module WorkspaceOperations module Create class PostFlattenDevfileValidator + include CreateConstants include Messages # Since this is called after flattening the devfile, we can safely assume that it has valid syntax @@ -70,16 +71,21 @@ def self.validate_components(context) return err(_("No components present in devfile")) if components.blank? injected_main_components = components.select do |component| - component.dig(:attributes, :"gl/inject-editor") + component.dig(:attributes, MAIN_COMPONENT_INDICATOR_ATTRIBUTE.to_sym) end - return err(_("No component has 'gl/inject-editor' attribute")) if injected_main_components.empty? + if injected_main_components.empty? + return err( + format(_("No component has '%{attribute}' attribute"), attribute: MAIN_COMPONENT_INDICATOR_ATTRIBUTE) + ) + end if injected_main_components.length > 1 return err( format( - _("Multiple components '%{name}' have 'gl/inject-editor' attribute"), - name: injected_main_components.pluck(:name) # rubocop:disable CodeReuse/ActiveRecord -- this pluck isn't from ActiveRecord, it's from ActiveSupport + _("Multiple components '%{name}' have '%{attribute}' attribute"), + name: injected_main_components.pluck(:name), # rubocop:disable CodeReuse/ActiveRecord -- this pluck isn't from ActiveRecord, it's from ActiveSupport + attribute: MAIN_COMPONENT_INDICATOR_ATTRIBUTE ) ) end diff --git a/ee/lib/remote_development/workspace_operations/create/project_cloner_component_inserter.rb b/ee/lib/remote_development/workspace_operations/create/project_cloner_component_inserter.rb index 625eb1e86aa4ad..ae117a2de3c9f2 100644 --- a/ee/lib/remote_development/workspace_operations/create/project_cloner_component_inserter.rb +++ b/ee/lib/remote_development/workspace_operations/create/project_cloner_component_inserter.rb @@ -4,7 +4,7 @@ module RemoteDevelopment module WorkspaceOperations module Create class ProjectClonerComponentInserter - include Messages + include CreateConstants # @param [Hash] context # @return [Hash] @@ -70,7 +70,7 @@ def self.insert(context) # implement better error handling to allow cloner to be able to deal with different categories of errors # issue: https://gitlab.com/gitlab-org/gitlab/-/issues/408451 cloner_component = { - name: "gl-project-cloner", + name: PROJECT_CLONER_COMPONENT_NAME, container: { image: image, args: [container_args], @@ -94,7 +94,7 @@ def self.insert(context) # create a command that will invoke the cloner cloner_command = { - id: "gl-project-cloner-command", + id: "#{PROJECT_CLONER_COMPONENT_NAME}-command", apply: { component: cloner_component[:name] } diff --git a/ee/lib/remote_development/workspace_operations/create/tools_injector_component_inserter.rb b/ee/lib/remote_development/workspace_operations/create/tools_injector_component_inserter.rb index 316651e4bb81ed..5ce4795f7e6cea 100644 --- a/ee/lib/remote_development/workspace_operations/create/tools_injector_component_inserter.rb +++ b/ee/lib/remote_development/workspace_operations/create/tools_injector_component_inserter.rb @@ -4,21 +4,18 @@ module RemoteDevelopment module WorkspaceOperations module Create class ToolsInjectorComponentInserter - include Messages + include CreateConstants # @param [Hash] context # @return [Hash] def self.insert(context) context => { processed_devfile: Hash => processed_devfile, - volume_mounts: Hash => volume_mounts, + tools_dir: String => tools_dir, settings: Hash => settings, } - volume_mounts => { data_volume: Hash => data_volume } - data_volume => { path: String => volume_path } settings => { tools_injector_image: String => image_from_settings } - tools_dir = "#{volume_path}/.gl-tools" # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/409775 - choose image based on which editor is passed. insert_tools_injector_component( processed_devfile: processed_devfile, @@ -34,15 +31,13 @@ def self.insert(context) # @param [String] image # @return [void] def self.insert_tools_injector_component(processed_devfile:, tools_dir:, image:) - component_name = "gl-tools-injector" - processed_devfile[:components] << { - name: component_name, + name: TOOLS_INJECTOR_COMPONENT_NAME, container: { image: image, env: [ { - name: "GL_TOOLS_DIR", + name: TOOLS_DIR_ENV_VAR, value: tools_dir } ], @@ -53,11 +48,11 @@ def self.insert_tools_injector_component(processed_devfile:, tools_dir:, image:) } } - command_name = "#{component_name}-command" + command_name = "#{TOOLS_INJECTOR_COMPONENT_NAME}-command" processed_devfile[:commands] << { id: command_name, apply: { - component: component_name + component: TOOLS_INJECTOR_COMPONENT_NAME } } diff --git a/ee/lib/remote_development/workspace_operations/create/volume_component_inserter.rb b/ee/lib/remote_development/workspace_operations/create/volume_component_inserter.rb index a1b3635c1070a6..7128b763b9955c 100644 --- a/ee/lib/remote_development/workspace_operations/create/volume_component_inserter.rb +++ b/ee/lib/remote_development/workspace_operations/create/volume_component_inserter.rb @@ -4,8 +4,6 @@ module RemoteDevelopment module WorkspaceOperations module Create class VolumeComponentInserter - include Messages - # @param [Hash] context # @return [Hash] def self.insert(context) diff --git a/ee/lib/remote_development/workspace_operations/create/volume_definer.rb b/ee/lib/remote_development/workspace_operations/create/volume_definer.rb index b5c1ec2320f516..4187f62bd4c5b8 100644 --- a/ee/lib/remote_development/workspace_operations/create/volume_definer.rb +++ b/ee/lib/remote_development/workspace_operations/create/volume_definer.rb @@ -7,11 +7,11 @@ module Create # we may want to add more logic to this class in the future, possibly to allow users # control over the configuration of the volume mounts. class VolumeDefiner + include CreateConstants + # @param [Hash] context # @return [Hash] def self.define(context) - workspace_data_volume_name = "gl-workspace-data" - # workspace_root is set to /projects as devfile parser uses this value when setting env vars # PROJECTS_ROOT and PROJECT_SOURCE that are available within the spawned containers # hence, workspace_root will be used across containers/initContainers as the place for user data @@ -19,13 +19,15 @@ def self.define(context) # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/408450 # explore in depth implications of PROJECTS_ROOT and PROJECT_SOURCE env vars with devfile team # and update devfile processing to use them idiomatically / conform to devfile specifications - workspace_root = "/projects" + + tools_dir = "#{WORKSPACE_DATA_VOLUME_PATH}/#{TOOLS_DIR_NAME}" context.merge( + tools_dir: tools_dir, volume_mounts: { data_volume: { - name: workspace_data_volume_name, - path: workspace_root + name: WORKSPACE_DATA_VOLUME_NAME, + path: WORKSPACE_DATA_VOLUME_PATH } } ) diff --git a/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb b/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb index 3a4ade8e84986c..89fcc04d5510dc 100644 --- a/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb +++ b/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb @@ -4,6 +4,8 @@ module RemoteDevelopment module WorkspaceOperations module Create class WorkspaceVariables + include CreateConstants + GIT_CREDENTIAL_STORE_SCRIPT = <<~SH.chomp #!/bin/sh # This is a readonly store so we can exit cleanly when git attempts a store or erase action @@ -46,13 +48,13 @@ def self.variables( static_variables = [ { - key: File.basename(RemoteDevelopment::WorkspaceOperations::FileMounts::GITLAB_TOKEN_FILE), + key: File.basename(TOKEN_FILE), value: personal_access_token_value, variable_type: RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:file], workspace_id: workspace_id }, { - key: File.basename(RemoteDevelopment::WorkspaceOperations::FileMounts::GITLAB_GIT_CREDENTIAL_STORE_FILE), + key: File.basename(GIT_CREDENTIAL_STORE_FILE), value: GIT_CREDENTIAL_STORE_SCRIPT, variable_type: RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:file], workspace_id: workspace_id @@ -71,7 +73,7 @@ def self.variables( }, { key: 'GIT_CONFIG_VALUE_0', - value: RemoteDevelopment::WorkspaceOperations::FileMounts::GITLAB_GIT_CREDENTIAL_STORE_FILE, + value: GIT_CREDENTIAL_STORE_FILE, variable_type: RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:environment], workspace_id: workspace_id }, @@ -101,13 +103,13 @@ def self.variables( }, { key: 'GL_GIT_CREDENTIAL_STORE_FILE_PATH', - value: RemoteDevelopment::WorkspaceOperations::FileMounts::GITLAB_GIT_CREDENTIAL_STORE_FILE, + value: GIT_CREDENTIAL_STORE_FILE, variable_type: RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:environment], workspace_id: workspace_id }, { key: 'GL_TOKEN_FILE_PATH', - value: RemoteDevelopment::WorkspaceOperations::FileMounts::GITLAB_TOKEN_FILE, + value: TOKEN_FILE, variable_type: RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:environment], workspace_id: workspace_id }, @@ -144,7 +146,7 @@ def self.variables( }, { key: 'GITLAB_WORKFLOW_TOKEN_FILE', - value: RemoteDevelopment::WorkspaceOperations::FileMounts::GITLAB_TOKEN_FILE, + value: TOKEN_FILE, variable_type: RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:environment], workspace_id: workspace_id } diff --git a/ee/lib/remote_development/workspace_operations/file_mounts.rb b/ee/lib/remote_development/workspace_operations/file_mounts.rb deleted file mode 100644 index 596eb9bf05238c..00000000000000 --- a/ee/lib/remote_development/workspace_operations/file_mounts.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module RemoteDevelopment - module WorkspaceOperations - module FileMounts - ROOT_DIR = "/.workspace-data" - VARIABLES_DIR = "#{ROOT_DIR}/variables".freeze - VARIABLES_FILE_DIR = "#{VARIABLES_DIR}/file".freeze - GITLAB_GIT_CREDENTIAL_STORE_FILE = "#{VARIABLES_FILE_DIR}/gl_git_credential_store.sh".freeze - GITLAB_TOKEN_FILE = "#{VARIABLES_FILE_DIR}/gl_token".freeze - end - end -end diff --git a/ee/lib/remote_development/workspace_operations/reconcile/input/params_extractor.rb b/ee/lib/remote_development/workspace_operations/reconcile/input/params_extractor.rb index e1a3ee25166c54..52b94f26a8b7f8 100644 --- a/ee/lib/remote_development/workspace_operations/reconcile/input/params_extractor.rb +++ b/ee/lib/remote_development/workspace_operations/reconcile/input/params_extractor.rb @@ -5,8 +5,6 @@ module WorkspaceOperations module Reconcile module Input class ParamsExtractor - include Messages - # @param [Hash] context # @return [Hash] def self.extract(context) diff --git a/ee/lib/remote_development/workspace_operations/reconcile/input/params_to_infos_converter.rb b/ee/lib/remote_development/workspace_operations/reconcile/input/params_to_infos_converter.rb index aeba6c58840002..5fcd4a8226134d 100644 --- a/ee/lib/remote_development/workspace_operations/reconcile/input/params_to_infos_converter.rb +++ b/ee/lib/remote_development/workspace_operations/reconcile/input/params_to_infos_converter.rb @@ -5,8 +5,6 @@ module WorkspaceOperations module Reconcile module Input class ParamsToInfosConverter - include Messages - # @param [Hash] context # @return [Hash] def self.convert(context) diff --git a/ee/lib/remote_development/workspace_operations/reconcile/main.rb b/ee/lib/remote_development/workspace_operations/reconcile/main.rb index ce2bae1c4915d8..903b76a2a14555 100644 --- a/ee/lib/remote_development/workspace_operations/reconcile/main.rb +++ b/ee/lib/remote_development/workspace_operations/reconcile/main.rb @@ -29,7 +29,7 @@ def self.main(context) .map( # As the final step, return the response_payload content in a WorkspaceReconcileSuccessful message ->(context) do - RemoteDevelopment::Messages::WorkspaceReconcileSuccessful.new(context.fetch(:response_payload)) + WorkspaceReconcileSuccessful.new(context.fetch(:response_payload)) end ) diff --git a/ee/lib/remote_development/workspace_operations/reconcile/output/devfile_parser.rb b/ee/lib/remote_development/workspace_operations/reconcile/output/devfile_parser.rb index 1b18da2bd28acc..f77d3a3840f9f9 100644 --- a/ee/lib/remote_development/workspace_operations/reconcile/output/devfile_parser.rb +++ b/ee/lib/remote_development/workspace_operations/reconcile/output/devfile_parser.rb @@ -7,7 +7,8 @@ module WorkspaceOperations module Reconcile module Output class DevfileParser - RUN_AS_USER = 5001 + include WorkspaceOperationsConstants + include ReconcileConstants # @param [String] processed_devfile # @param [Hash] k8s_resources_params @@ -227,7 +228,7 @@ def self.inject_secrets(workspace_resources:, env_secret_names:, file_secret_nam volume_mounts = [ { 'name' => volume_name, - 'mountPath' => RemoteDevelopment::WorkspaceOperations::FileMounts::VARIABLES_FILE_DIR + 'mountPath' => VARIABLES_FILE_DIR } ] env_from = env_secret_names.map { |v| { 'secretRef' => { 'name' => v } } } diff --git a/ee/lib/remote_development/workspace_operations/reconcile/output/response_payload_builder.rb b/ee/lib/remote_development/workspace_operations/reconcile/output/response_payload_builder.rb index bb6858b81ebfac..85beca1669d56a 100644 --- a/ee/lib/remote_development/workspace_operations/reconcile/output/response_payload_builder.rb +++ b/ee/lib/remote_development/workspace_operations/reconcile/output/response_payload_builder.rb @@ -5,7 +5,6 @@ module WorkspaceOperations module Reconcile module Output class ResponsePayloadBuilder - include Messages include UpdateTypes ALL_RESOURCES_INCLUDED = :all_resources_included diff --git a/ee/lib/remote_development/workspace_operations/reconcile/reconcile_constants.rb b/ee/lib/remote_development/workspace_operations/reconcile/reconcile_constants.rb new file mode 100644 index 00000000000000..e31e2fc27c4c39 --- /dev/null +++ b/ee/lib/remote_development/workspace_operations/reconcile/reconcile_constants.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module WorkspaceOperations + module Reconcile + # NOTE: Constants are scoped to the use-case namespace in which they are used in production + # code (but they may still be referenced by specs or fixtures or factories). + # For example, this RemoteDevelopment::WorkspaceOperations::Create::CreateConstants + # file contains constants which are only used by classes within that namespace. + # + # See documentation at ../../README.md#constant-declarations for more information. + module ReconcileConstants + include WorkspaceOperationsConstants + + # Please keep alphabetized + RUN_AS_USER = 5001 + end + end + end +end diff --git a/ee/lib/remote_development/workspace_operations/workspace_operations_constants.rb b/ee/lib/remote_development/workspace_operations/workspace_operations_constants.rb new file mode 100644 index 00000000000000..56f0ffb6f6b47e --- /dev/null +++ b/ee/lib/remote_development/workspace_operations/workspace_operations_constants.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module WorkspaceOperations + # NOTE: Constants are scoped to the namespace in which they are used in production + # code (but they may still be referenced by specs or fixtures or factories). + # For example, this RemoteDevelopment::WorkspaceOperations::WorkspaceOperationsConstants + # file only contains constants which are used by multiple sub-namespaces + # of WorkspaceOperations, such as Create and Reconcile. + # Constants which are only used by a specific use-case sub-namespace + # like Create or Reconcile should be contained in the corresponding + # constants class such as CreateConstants or ReconcileConstants. + # + # Multiple related constants may be declared in their own dedicated + # namespace, such as RemoteDevelopment::WorkspaceOperations::States. + # + # See documentation at ../README.md#constant-declarations for more information. + module WorkspaceOperationsConstants + # Please keep alphabetized + VARIABLES_FILE_DIR = "/.workspace-data/variables/file" + end + end +end diff --git a/ee/spec/factories/remote_development/workspaces.rb b/ee/spec/factories/remote_development/workspaces.rb index 6b9842bb0c1ef8..16b3600487c8e9 100644 --- a/ee/spec/factories/remote_development/workspaces.rb +++ b/ee/spec/factories/remote_development/workspaces.rb @@ -12,7 +12,10 @@ name { "workspace-#{agent.id}-#{user.id}-#{random_string}" } force_include_all_resources { true } - add_attribute(:namespace) { "gl-rd-ns-#{agent.id}-#{user.id}-#{random_string}" } + add_attribute(:namespace) do + namespace_prefix = RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::NAMESPACE_PREFIX + "#{namespace_prefix}-#{agent.id}-#{user.id}-#{random_string}" + end desired_state { RemoteDevelopment::WorkspaceOperations::States::STOPPED } actual_state { RemoteDevelopment::WorkspaceOperations::States::STOPPED } diff --git a/ee/spec/lib/remote_development/workspace_operations/create/creator_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/creator_spec.rb index e6f5bc21aaf161..db99cf2be03431 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/creator_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/creator_spec.rb @@ -33,10 +33,11 @@ let(:workspace) { instance_double("RemoteDevelopment::Workspace") } # rubocop:disable RSpec/VerifiedDoubleReference -- We're using the quoted version so we can use fast_spec_helper let(:updated_value) do + namespace_prefix = RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::NAMESPACE_PREFIX initial_value.merge( { workspace_name: "workspace-#{agent.id}-#{user.id}-#{random_string}", - workspace_namespace: "gl-rd-ns-#{agent.id}-#{user.id}-#{random_string}" + workspace_namespace: "#{namespace_prefix}-#{agent.id}-#{user.id}-#{random_string}" } ) end diff --git a/ee/spec/lib/remote_development/workspace_operations/create/main_component_updater_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/main_component_updater_spec.rb index 18813d77b30af4..56b9f657042732 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/main_component_updater_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/main_component_updater_spec.rb @@ -17,11 +17,8 @@ let(:context) do { processed_devfile: input_processed_devfile, - volume_mounts: { - data_volume: { - path: "/projects" - } - }, + tools_dir: "#{RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::WORKSPACE_DATA_VOLUME_PATH}/" \ + "#{RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::TOOLS_DIR_NAME}", vscode_extensions_gallery_metadata: { enabled: vscode_extensions_gallery_metadata_enabled } } end diff --git a/ee/spec/lib/remote_development/workspace_operations/create/main_integration_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/main_integration_spec.rb index 23845dcaa82a0a..2b20cf1cd62ff6 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/main_integration_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/main_integration_spec.rb @@ -115,7 +115,8 @@ expect(workspace.desired_state_updated_at).to eq(Time.current) expect(workspace.actual_state).to eq(RemoteDevelopment::WorkspaceOperations::States::CREATION_REQUESTED) expect(workspace.name).to eq("workspace-#{agent.id}-#{user.id}-#{random_string}") - expect(workspace.namespace).to eq("gl-rd-ns-#{agent.id}-#{user.id}-#{random_string}") + namespace_prefix = RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::NAMESPACE_PREFIX + expect(workspace.namespace).to eq("#{namespace_prefix}-#{agent.id}-#{user.id}-#{random_string}") expect(workspace.workspaces_agent_config_version).to eq(expected_workspaces_agent_config_version) expect(workspace.url).to eq(URI::HTTPS.build({ host: "60001-#{workspace.name}.#{dns_zone}", @@ -225,11 +226,13 @@ let(:vscode_extensions_gallery_metadata_enabled) { false } it 'uses image override' do + tools_injector_component_name = + RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::TOOLS_INJECTOR_COMPONENT_NAME workspace = response.fetch(:payload).fetch(:workspace) processed_devfile = yaml_safe_load_symbolized(workspace.processed_devfile) image_from_processed_devfile = processed_devfile.fetch(:components) - .find { |component| component.fetch(:name) == "gl-tools-injector" } + .find { |component| component.fetch(:name) == tools_injector_component_name } .dig(:container, :image) expect(image_from_processed_devfile).to eq(tools_injector_image_from_settings) end diff --git a/ee/spec/lib/remote_development/workspace_operations/create/post_flatten_devfile_validator_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/post_flatten_devfile_validator_spec.rb index 261146ab3d5502..5893339c095110 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/post_flatten_devfile_validator_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/post_flatten_devfile_validator_spec.rb @@ -8,6 +8,10 @@ include_context 'with remote development shared fixtures' let(:flattened_devfile_name) { 'example.flattened-with-entries-devfile.yaml' } + let(:main_component_indicator_attribute) do + ::RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::MAIN_COMPONENT_INDICATOR_ATTRIBUTE + end + let(:processed_devfile) { yaml_safe_load_symbolized(read_devfile_yaml(flattened_devfile_name)) } let(:context) { { processed_devfile: processed_devfile } } @@ -48,8 +52,8 @@ 'example.invalid-components-entry-empty-devfile.yaml' | "No components present in devfile" 'example.invalid-components-entry-missing-devfile.yaml' | "No components present in devfile" 'example.invalid-component-missing-name.yaml' | "Components must have a 'name'" - 'example.invalid-attributes-tools-injector-absent-devfile.yaml' | "No component has 'gl/inject-editor' attribute" - 'example.invalid-attributes-tools-injector-multiple-devfile.yaml' | "Multiple components '[\"tooling-container\", \"tooling-container-2\"]' have 'gl/inject-editor' attribute" + 'example.invalid-attributes-tools-injector-absent-devfile.yaml' | "No component has '#{main_component_indicator_attribute}' attribute" + 'example.invalid-attributes-tools-injector-multiple-devfile.yaml' | "Multiple components '[\"tooling-container\", \"tooling-container-2\"]' have '#{main_component_indicator_attribute}' attribute" 'example.invalid-unsupported-component-container-dedicated-pod-devfile.yaml' | "Property 'dedicatedPod' of component 'example' is not yet supported" 'example.invalid-unsupported-starter-projects-devfile.yaml' | "'starterProjects' is not yet supported" 'example.invalid-unsupported-projects-devfile.yaml' | "'projects' is not yet supported" diff --git a/ee/spec/lib/remote_development/workspace_operations/create/project_cloner_component_inserter_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/project_cloner_component_inserter_spec.rb index 0604753d97bbaa..1864a03c0f7152 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/project_cloner_component_inserter_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/project_cloner_component_inserter_spec.rb @@ -17,7 +17,6 @@ let(:expected_processed_devfile_name) { "example.project-cloner-inserted-devfile.yaml" } let(:expected_processed_devfile) { yaml_safe_load_symbolized(read_devfile_yaml(expected_processed_devfile_name)) } - let(:component_name) { "gl-project-cloner" } let(:context) do { params: { diff --git a/ee/spec/lib/remote_development/workspace_operations/create/tools_injector_component_inserter_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/tools_injector_component_inserter_spec.rb index b6f957c9a680f9..9c7faf500e836a 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/tools_injector_component_inserter_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/tools_injector_component_inserter_spec.rb @@ -23,11 +23,8 @@ let(:context) do { processed_devfile: input_processed_devfile, - volume_mounts: { - data_volume: { - path: "/projects" - } - }, + tools_dir: "#{RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::WORKSPACE_DATA_VOLUME_PATH}/" \ + "#{RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::TOOLS_DIR_NAME}", settings: settings, vscode_extensions_gallery_metadata: { enabled: vscode_extensions_gallery_metadata_enabled } } diff --git a/ee/spec/lib/remote_development/workspace_operations/create/volume_component_inserter_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/volume_component_inserter_spec.rb index ddbcfc46b6a4c2..34e03e2df418db 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/volume_component_inserter_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/volume_component_inserter_spec.rb @@ -11,8 +11,7 @@ let(:expected_processed_devfile_name) { "example.processed-devfile.yaml" } let(:expected_processed_devfile) { yaml_safe_load_symbolized(read_devfile_yaml(expected_processed_devfile_name)) } - let(:component_name) { "gl-workspace-data" } - let(:volume_name) { "gl-workspace-data" } + let(:volume_name) { RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::WORKSPACE_DATA_VOLUME_NAME } let(:context) do { processed_devfile: input_processed_devfile, diff --git a/ee/spec/lib/remote_development/workspace_operations/create/volume_definer_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/volume_definer_spec.rb index dc584b4f0fae66..8e41762c99e649 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/volume_definer_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/volume_definer_spec.rb @@ -4,6 +4,10 @@ RSpec.describe RemoteDevelopment::WorkspaceOperations::Create::VolumeDefiner, feature_category: :workspaces do let(:context) { { params: 1 } } + let(:expected_tools_dir) do + "#{RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::WORKSPACE_DATA_VOLUME_PATH}/" \ + "#{RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::TOOLS_DIR_NAME}" + end subject(:returned_value) do described_class.define(context) @@ -13,9 +17,10 @@ expect(returned_value).to eq( { params: 1, + tools_dir: expected_tools_dir, volume_mounts: { data_volume: { - name: "gl-workspace-data", + name: RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::WORKSPACE_DATA_VOLUME_NAME, path: "/projects" } } diff --git a/ee/spec/lib/remote_development/workspace_operations/create/workspace_creator_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/workspace_creator_spec.rb index 730efa029d480b..ea8a9932a9e264 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/workspace_creator_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/workspace_creator_spec.rb @@ -31,6 +31,7 @@ end let(:context) do + namespace_prefix = RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::NAMESPACE_PREFIX { params: params, user: user, @@ -38,10 +39,10 @@ processed_devfile: processed_devfile, personal_access_token: personal_access_token, workspace_name: "workspace-#{agent.id}-#{user.id}-#{random_string}", - workspace_namespace: "gl-rd-ns-#{agent.id}-#{user.id}-#{random_string}", + workspace_namespace: "#{namespace_prefix}-#{agent.id}-#{user.id}-#{random_string}", volume_mounts: { data_volume: { - name: "gl-workspace-data", + name: RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::WORKSPACE_DATA_VOLUME_NAME, path: workspace_root } } diff --git a/ee/spec/requests/remote_development/integration_spec.rb b/ee/spec/requests/remote_development/integration_spec.rb index 184059b152b594..d3536e01dfc46c 100644 --- a/ee/spec/requests/remote_development/integration_spec.rb +++ b/ee/spec/requests/remote_development/integration_spec.rb @@ -223,7 +223,8 @@ def do_create_workspace # noinspection RubyResolve expect(workspace.desired_state_updated_at).to eq(Time.current) expect(workspace.name).to eq("workspace-#{agent.id}-#{user.id}-#{random_string}") - expect(workspace.namespace).to eq("gl-rd-ns-#{agent.id}-#{user.id}-#{random_string}") + namespace_prefix = RemoteDevelopment::WorkspaceOperations::Create::CreateConstants::NAMESPACE_PREFIX + expect(workspace.namespace).to eq("#{namespace_prefix}-#{agent.id}-#{user.id}-#{random_string}") expect(workspace.url).to eq(URI::HTTPS.build({ host: "60001-#{workspace.name}.#{dns_zone}", path: "/", diff --git a/ee/spec/support/shared_contexts/remote_development/remote_development_shared_contexts.rb b/ee/spec/support/shared_contexts/remote_development/remote_development_shared_contexts.rb index 4f2f69b589ff30..ec6c0e0e2387e0 100644 --- a/ee/spec/support/shared_contexts/remote_development/remote_development_shared_contexts.rb +++ b/ee/spec/support/shared_contexts/remote_development/remote_development_shared_contexts.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +# NOTE: These fixtures act somewhat as a "Golden Master" source of truth, so we do not use the constant values from +# RemoteDevelopment::WorkspaceOperations::Create::Constants, but instead hardcode the corresponding values here. RSpec.shared_context 'with remote development shared fixtures' do # rubocop:todo Metrics/ParameterLists, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity -- Cleanup as part of https://gitlab.com/gitlab-org/gitlab/-/issues/421687 def create_workspace_agent_info_hash( @@ -636,12 +638,12 @@ def workspace_deployment( default_runtime_class:, desired_config_generator_version: ) - variables_file_mount_path = RemoteDevelopment::WorkspaceOperations::FileMounts::VARIABLES_FILE_DIR + variables_file_mount_path = RemoteDevelopment::WorkspaceOperations::WorkspaceOperationsConstants::VARIABLES_FILE_DIR container_security_context = { 'allowPrivilegeEscalation' => allow_privilege_escalation, 'privileged' => false, 'runAsNonRoot' => true, - 'runAsUser' => 5001 + 'runAsUser' => RemoteDevelopment::WorkspaceOperations::Reconcile::ReconcileConstants::RUN_AS_USER } deployment = { @@ -940,7 +942,7 @@ def workspace_deployment( ], securityContext: { runAsNonRoot: true, - runAsUser: 5001, + runAsUser: RemoteDevelopment::WorkspaceOperations::Reconcile::ReconcileConstants::RUN_AS_USER, fsGroup: 0, fsGroupChangePolicy: "OnRootMismatch" } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 252d464baabd15..c08d4f8bc5de51 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -36633,7 +36633,7 @@ msgstr "" msgid "Multiple Prometheus integrations are not supported" msgstr "" -msgid "Multiple components '%{name}' have 'gl/inject-editor' attribute" +msgid "Multiple components '%{name}' have '%{attribute}' attribute" msgstr "" msgid "Multiple integrations of a single type are not supported for this project" @@ -37518,7 +37518,7 @@ msgstr "" msgid "No committers" msgstr "" -msgid "No component has 'gl/inject-editor' attribute" +msgid "No component has '%{attribute}' attribute" msgstr "" msgid "No components present in devfile" -- GitLab From dd9f901abb449986f5867a24ba8993402a4b8a91 Mon Sep 17 00:00:00 2001 From: Chad Woolley Date: Fri, 17 Jan 2025 00:18:58 -0800 Subject: [PATCH 2/2] Fix grammar typo --- ee/lib/remote_development/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/lib/remote_development/README.md b/ee/lib/remote_development/README.md index b65734c0552886..78ad14877fdb65 100644 --- a/ee/lib/remote_development/README.md +++ b/ee/lib/remote_development/README.md @@ -319,7 +319,7 @@ We _currently_ do not make heavy use of metaprogramming. But, we may make use of ### Constant declarations -- If a constant is only used in a single class or module, it are declared within that class/module +- If a constant is only used in a single class or module, it should be declared within that class/module. - If constants are used by multiple classes or modules, they are grouped in a single class at each Ruby module/namespace level corresponding to the domain use-case, at the lowest level at which they are used. For example, constants used only within `RemoteDevelopment::WorkspaceOperations::Create` will all be declared in `RemoteDevelopment::WorkspaceOperations::Create::CreateConstants` class. Likewise, constants shared across multiple sub-modules of `RemoteDevelopment::WorkspaceOperations` will be declared in `RemoteDevelopment::WorkspaceOperations::WorkspaceOperationConstants` -- GitLab