diff --git a/ee/lib/remote_development/README.md b/ee/lib/remote_development/README.md index 7c7ae0d5a376d120a5f3dcf045083b242f1b6e01..78ad14877fdb6513d18c5076a2316a26ad6d75cb 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 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` + - 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 0000000000000000000000000000000000000000..0660e8f5720252179891686a019d68b58d9e887a --- /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 ecc06d134f19327f88cf32eb091bdcc31b94212e..5f65fd41783c9a05a3fbc12c1cfb7994565ea48b 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 7996961c61a44cea0f8ec4d1f715940b79119464..484765736d9d2885cc2193eace9798cd7221af62 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 c1d3ab8fcc0d8f12f651da7cd7049d2aca243cb4..f264cc3d2f6c6c27d6ff0635f553eb1526f9d064 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 625eb1e86aa4ad3448debb763c5effc1f173f77e..ae117a2de3c9f21ca3cbacbeeb8169da47336737 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 316651e4bb81ed40cbf2ca6965dcb57613dcd9f2..5ce4795f7e6cea53192c9c02d799c3ef8b140dfb 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 a1b3635c1070a6c025d66e305de3d228fa1726d7..7128b763b9955c92e7843964ce885eda0b68fd5f 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 b5c1ec2320f516326e723415ad3413d4e84b50b6..4187f62bd4c5b8e0ed37130a2d559d69efdbf01d 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 3a4ade8e84986ca53bb285d9c74f4bebd1999e77..89fcc04d5510dc2fae8df22adc28d4708b13d136 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 596eb9bf05238c9b58ec9f02a9335e60af5419a5..0000000000000000000000000000000000000000 --- 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 e1a3ee25166c54cac76c4f03dfd9fdd34bb49e57..52b94f26a8b7f88ec357c795297b780d20869b54 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 aeba6c588400022902f40cc0a9fc3006dc9a8bef..5fcd4a8226134dad07c28d6f3832ac2af5f6c2f9 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 ce2bae1c4915d8cf87655db2460992659ba32374..903b76a2a145552820308a733cc28b8b118f3c00 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 1b18da2bd28acc2933a9f447772804192bf05b9a..f77d3a3840f9f966f66a0d6a33c51aeaf370bd2e 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 bb6858b81ebfac1e1605aba1eee6a130d2be882e..85beca1669d56aa6689df4c5fff135f8dc0533b3 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 0000000000000000000000000000000000000000..e31e2fc27c4c39bf07bd06a133b4275bcb4009d4 --- /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 0000000000000000000000000000000000000000..56f0ffb6f6b47e5101a694fd65f845cdf2256e12 --- /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 6b9842bb0c1ef81799acb530b6c507bd0983003f..16b3600487c8e9f9ad07f229421fcdb6dcd19d51 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 e6f5bc21aaf161639bbd717f0d5a309dba5377b5..db99cf2be03431bcb5429a5046e687343de3ea6e 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 18813d77b30af4c773bdefb09fc26bc981f59dea..56b9f657042732c54467cab6816761e4a03fe01d 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 23845dcaa82a0a65efac8982725d317845f48ecb..2b20cf1cd62ff643176a4dacdc8efa61bea6b5b8 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 261146ab3d550282b0666074cbb42a539003db2f..5893339c095110820cb64bcd18c61f50e10fa0bb 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 0604753d97bbaa13f420dc0840975c3419db1747..1864a03c0f71520a7941b1a1a43fdea26acc12b8 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 b6f957c9a680f99f48addcefb9ee41c04b1386ae..9c7faf500e836a2542ce4a8882ebedd2a1a431ee 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 ddbcfc46b6a4c205348655f28290ebc374061e83..34e03e2df418db343f02531950ce19f2dbd315de 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 dc584b4f0fae66e5308c17ed7a29b6f399651914..8e41762c99e649c053a23f01613b8d2aac5b7274 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 730efa029d480bd02e21302c7d728e5def01da08..ea8a9932a9e264f815f05bae6a0ecbc7eb810ea3 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 184059b152b59441970248f5461c82f9d0f22b08..d3536e01dfc46c7359824ccdfe95f27d9bdf1342 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 4f2f69b589ff30006f1efad771151c17db2fa15c..ec6c0e0e2387e0ef111c8ad33c791bcdfb537bc1 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 252d464baabd15c7e21b997dd702a64798dd6dd8..c08d4f8bc5de51b792941a898fcd124ee41f417b 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"