From 556042c01281cd84407f9075e6fc93212f82fd54 Mon Sep 17 00:00:00 2001 From: Chad Woolley Date: Thu, 8 Feb 2024 18:33:28 -0400 Subject: [PATCH 1/3] Introduce Remote Dev Settings module - Add Settings module for Remote Development. See ee/lib/remote_development/README.md for details. --- .../remote_development/constants.js | 1 - .../remote_development/init_workspaces_app.js | 11 +- .../remote_development/pages/create.vue | 4 +- ee/app/models/remote_development/workspace.rb | 10 +- .../remote_development/workspace_variable.rb | 2 +- .../agent_config/update_service.rb | 6 +- .../workspaces/create_service.rb | 6 +- .../workspaces/reconcile_service.rb | 3 +- .../workspaces/update_service.rb | 7 +- .../workspaces/index.html.haml | 7 +- ee/lib/remote_development/README.md | 126 +++++++++++++++--- ee/lib/remote_development/messages.rb | 7 + ee/lib/remote_development/settings.rb | 7 + .../settings/current_settings_reader.rb | 39 ++++++ .../settings/defaults_initializer.rb | 60 +++++++++ .../settings/env_var_reader.rb | 61 +++++++++ ee/lib/remote_development/settings/main.rb | 48 +++++++ .../remote_development/settings/public_api.rb | 36 +++++ .../project_cloner_component_injector.rb | 9 +- .../init_workspaces_app_spec.js | 41 ++++-- .../remote_development/pages/create_spec.js | 4 + .../remote_development/router/index_spec.js | 2 + .../settings/current_settings_reader_spec.rb | 62 +++++++++ .../settings/defaults_initializer_spec.rb | 70 ++++++++++ .../settings/env_var_reader_spec.rb | 116 ++++++++++++++++ .../remote_development/settings/main_spec.rb | 118 ++++++++++++++++ .../settings/public_api_spec.rb | 27 ++++ .../settings/settings_integration_spec.rb | 50 +++++++ .../create/main_integration_spec.rb | 9 +- .../project_cloner_component_injector_spec.rb | 4 + .../remote_development/workspace_spec.rb | 37 +++-- .../agent_config/update_service_spec.rb | 4 +- .../workspaces/create_service_spec.rb | 4 +- .../workspaces/reconcile_service_spec.rb | 5 +- .../workspaces/update_service_spec.rb | 9 +- 35 files changed, 947 insertions(+), 65 deletions(-) create mode 100644 ee/lib/remote_development/settings.rb create mode 100644 ee/lib/remote_development/settings/current_settings_reader.rb create mode 100644 ee/lib/remote_development/settings/defaults_initializer.rb create mode 100644 ee/lib/remote_development/settings/env_var_reader.rb create mode 100644 ee/lib/remote_development/settings/main.rb create mode 100644 ee/lib/remote_development/settings/public_api.rb create mode 100644 ee/spec/lib/remote_development/settings/current_settings_reader_spec.rb create mode 100644 ee/spec/lib/remote_development/settings/defaults_initializer_spec.rb create mode 100644 ee/spec/lib/remote_development/settings/env_var_reader_spec.rb create mode 100644 ee/spec/lib/remote_development/settings/main_spec.rb create mode 100644 ee/spec/lib/remote_development/settings/public_api_spec.rb create mode 100644 ee/spec/lib/remote_development/settings/settings_integration_spec.rb diff --git a/ee/app/assets/javascripts/remote_development/constants.js b/ee/app/assets/javascripts/remote_development/constants.js index e5c1da090bd25b..13cfbca1ef005c 100644 --- a/ee/app/assets/javascripts/remote_development/constants.js +++ b/ee/app/assets/javascripts/remote_development/constants.js @@ -3,7 +3,6 @@ import { s__ } from '~/locale'; export const DEFAULT_DEVFILE_PATH = '.devfile.yaml'; export const DEFAULT_EDITOR = 'webide'; -export const DEFAULT_MAX_HOURS_BEFORE_TERMINATION = 24; /* eslint-disable @gitlab/require-i18n-strings */ export const WORKSPACE_STATES = { diff --git a/ee/app/assets/javascripts/remote_development/init_workspaces_app.js b/ee/app/assets/javascripts/remote_development/init_workspaces_app.js index 41b0d650986605..9a08c8dc4ba121 100644 --- a/ee/app/assets/javascripts/remote_development/init_workspaces_app.js +++ b/ee/app/assets/javascripts/remote_development/init_workspaces_app.js @@ -2,6 +2,7 @@ import Vue from 'vue'; import VueApollo from 'vue-apollo'; import createDefaultClient from '~/lib/graphql'; import { injectVueAppBreadcrumbs } from '~/lib/utils/breadcrumbs'; +import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; import WorkspacesBreadcrumbs from './components/common/workspaces_breadcrumbs.vue'; import App from './pages/app.vue'; import createRouter from './router/index'; @@ -21,9 +22,10 @@ const initWorkspacesApp = () => { return null; } - const { workspacesListPath, emptyStateSvgPath } = el.dataset; + const options = convertObjectPropsToCamelCase(JSON.parse(el.dataset.options)); + // noinspection JSUnresolvedReference -- Avoids error on options property access. Adding `@returns {Object|Array}` to convertObjectPropsToCamelCase didn't help. TODO: Report and add to https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/ const router = createRouter({ - base: workspacesListPath, + base: options.workspacesListPath, }); injectVueAppBreadcrumbs(router, WorkspacesBreadcrumbs); return new Vue({ @@ -31,10 +33,7 @@ const initWorkspacesApp = () => { name: 'WorkspacesRoot', router, apolloProvider: createApolloProvider(), - provide: { - workspacesListPath, - emptyStateSvgPath, - }, + provide: options, render: (createElement) => createElement(App), }); }; diff --git a/ee/app/assets/javascripts/remote_development/pages/create.vue b/ee/app/assets/javascripts/remote_development/pages/create.vue index 596118354a651b..cd0bad1e99c1b4 100644 --- a/ee/app/assets/javascripts/remote_development/pages/create.vue +++ b/ee/app/assets/javascripts/remote_development/pages/create.vue @@ -25,7 +25,6 @@ import { DEFAULT_EDITOR, ROUTES, PROJECT_VISIBILITY, - DEFAULT_MAX_HOURS_BEFORE_TERMINATION, } from '../constants'; export const i18n = { @@ -73,6 +72,7 @@ export default { SearchProjectsListbox, GetProjectDetailsQuery, }, + inject: ['defaultMaxHoursBeforeTermination'], data() { return { selectedProject: null, @@ -82,7 +82,7 @@ export default { hasDevFile: null, projectId: null, rootRef: null, - maxHoursBeforeTermination: DEFAULT_MAX_HOURS_BEFORE_TERMINATION, + maxHoursBeforeTermination: this.defaultMaxHoursBeforeTermination, projectDetailsLoaded: false, error: '', }; diff --git a/ee/app/models/remote_development/workspace.rb b/ee/app/models/remote_development/workspace.rb index 6e09f2d8337f92..7d05999cf05b7e 100644 --- a/ee/app/models/remote_development/workspace.rb +++ b/ee/app/models/remote_development/workspace.rb @@ -8,8 +8,6 @@ class Workspace < ApplicationRecord include RemoteDevelopment::Workspaces::States include ::Gitlab::Utils::StrongMemoize - MAX_HOURS_BEFORE_TERMINATION_LIMIT = 120 - ignore_column :url_domain, remove_with: '16.9', remove_after: '2019-01-19' belongs_to :user, inverse_of: :workspaces @@ -37,7 +35,9 @@ class Workspace < ApplicationRecord validates :desired_state, inclusion: { in: VALID_DESIRED_STATES } validates :actual_state, inclusion: { in: VALID_ACTUAL_STATES } validates :editor, inclusion: { in: ['webide'], message: "'webide' is currently the only supported editor" } - validates :max_hours_before_termination, numericality: { less_than_or_equal_to: MAX_HOURS_BEFORE_TERMINATION_LIMIT } + validates :max_hours_before_termination, numericality: { + less_than_or_equal_to: :max_hours_before_termination_limit + } validate :enforce_permanent_termination validate :enforce_quotas, on: :create @@ -116,6 +116,10 @@ def url private + def max_hours_before_termination_limit + RemoteDevelopment::Settings.get_single_setting(:max_hours_before_termination_limit) + end + def validate_agent_config_present_and_enabled unless agent&.remote_development_agent_config errors.add(:agent, _('for Workspace must have an associated RemoteDevelopmentAgentConfig')) diff --git a/ee/app/models/remote_development/workspace_variable.rb b/ee/app/models/remote_development/workspace_variable.rb index 40e1737a4e53be..5ac49cfc2547e3 100644 --- a/ee/app/models/remote_development/workspace_variable.rb +++ b/ee/app/models/remote_development/workspace_variable.rb @@ -24,7 +24,7 @@ class WorkspaceVariable < ApplicationRecord attr_encrypted :value, mode: :per_attribute_iv, - key: Settings.attr_encrypted_db_key_base_32, + key: ::Settings.attr_encrypted_db_key_base_32, algorithm: 'aes-256-gcm' end end diff --git a/ee/app/services/remote_development/agent_config/update_service.rb b/ee/app/services/remote_development/agent_config/update_service.rb index cf29b2cd781dd6..2c919f1549dd3f 100644 --- a/ee/app/services/remote_development/agent_config/update_service.rb +++ b/ee/app/services/remote_development/agent_config/update_service.rb @@ -32,7 +32,11 @@ def execute(agent:, config:) # and still attempt to return an appropriate ServiceResponse object, even though it is ignored, # so that abstracts us somewhat from whatever we decide to do with this error handling at the Service # layer. - response_hash = Main.main(agent: agent, config: config) + response_hash = Main.main( + agent: agent, + config: config, + settings: ::RemoteDevelopment::Settings.get_all_settings + ) # TODO: https://gitlab.com/groups/gitlab-org/-/epics/10461 - Add at least some logging here. create_service_response(response_hash) diff --git a/ee/app/services/remote_development/workspaces/create_service.rb b/ee/app/services/remote_development/workspaces/create_service.rb index a5d1980678de4b..cd5bac11badbfe 100644 --- a/ee/app/services/remote_development/workspaces/create_service.rb +++ b/ee/app/services/remote_development/workspaces/create_service.rb @@ -22,7 +22,11 @@ def initialize(current_user:) # @param [Hash] params # @return [ServiceResponse] def execute(params:) - response_hash = Create::Main.main(current_user: current_user, params: params) + response_hash = Create::Main.main( + current_user: current_user, + params: params, + settings: ::RemoteDevelopment::Settings.get_all_settings + ) # Type-check payload using rightward assignment response_hash[:payload] => { workspace: Workspace } if response_hash[:payload] diff --git a/ee/app/services/remote_development/workspaces/reconcile_service.rb b/ee/app/services/remote_development/workspaces/reconcile_service.rb index 8e775b28dcb61d..9f5c2291f47e50 100644 --- a/ee/app/services/remote_development/workspaces/reconcile_service.rb +++ b/ee/app/services/remote_development/workspaces/reconcile_service.rb @@ -31,7 +31,8 @@ def execute(agent:, params:) # shape and contents of the params payload. original_params: params, agent: agent, - logger: logger + logger: logger, + settings: ::RemoteDevelopment::Settings.get_all_settings ) # Type-check payload using rightward assignment diff --git a/ee/app/services/remote_development/workspaces/update_service.rb b/ee/app/services/remote_development/workspaces/update_service.rb index d788375446f3ae..f0e60c614ea80c 100644 --- a/ee/app/services/remote_development/workspaces/update_service.rb +++ b/ee/app/services/remote_development/workspaces/update_service.rb @@ -22,7 +22,12 @@ def initialize(current_user:) # @param [Hash] params # @return [ServiceResponse] def execute(workspace:, params:) - response_hash = Update::Main.main(workspace: workspace, current_user: current_user, params: params) + response_hash = Update::Main.main( + workspace: workspace, + current_user: current_user, + params: params, + settings: ::RemoteDevelopment::Settings.get_all_settings + ) # Type-check payload using rightward assignment response_hash[:payload] => { workspace: Workspace } if response_hash[:payload] diff --git a/ee/app/views/remote_development/workspaces/index.html.haml b/ee/app/views/remote_development/workspaces/index.html.haml index cfc35ca66f405c..70cd791505401c 100644 --- a/ee/app/views/remote_development/workspaces/index.html.haml +++ b/ee/app/views/remote_development/workspaces/index.html.haml @@ -3,7 +3,10 @@ - add_page_specific_style 'page_bundles/remote_development' #js-workspaces{ data: { - workspaces_list_path: remote_development_workspaces_path, - empty_state_svg_path: image_path('illustrations/empty-state/empty-workspaces-md.svg') + options: { + workspaces_list_path: remote_development_workspaces_path, + empty_state_svg_path: image_path('illustrations/empty-state/empty-workspaces-md.svg'), + default_max_hours_before_termination: RemoteDevelopment::Settings.get_single_setting(:default_max_hours_before_termination) + }.to_json } } diff --git a/ee/lib/remote_development/README.md b/ee/lib/remote_development/README.md index 21b22703c28075..a386790a6db6cc 100644 --- a/ee/lib/remote_development/README.md +++ b/ee/lib/remote_development/README.md @@ -11,6 +11,7 @@ - [Railway Oriented Programming and the Result Class](#railway-oriented-programming-and-the-result-class) - [Benefits](#benefits) - [Differences from standard GitLab patterns](#differences-from-standard-gitlab-patterns) +- [Remote Development Settings](#settings) - [FAQ](#faq) ## TL;DR and Quickstart @@ -37,18 +38,35 @@ In the Remote Development feature, we strive to maintain a clean, layered archit ```mermaid flowchart TB direction TB - Client --> r - subgraph r[Rails Routing/Controllers] - direction TB - subgraph g[Grape/GraphQL API] + Client --> controllers + subgraph monolith[Rails Monolith] + subgraph routinglayer[Remote Development/GitLab Agent for Kubernetes: Routing/Controller layer] direction TB - subgraph s[Remote Development Services] + controllers[Controllers] + subgraph apilayer[Remote Development / GitLab Agent For Kubernetes: Grape/GraphQL API layer] direction TB - dl[Domain Logic] + apis[Grape APIs & GraphQL Resolvers/Mutations] + subgraph service[Remote Development: Service layer] + direction TB + services[Services] + subgraph domainlogiclayer[Remote Development: Domain Logic layer] + domainlogic[Domain Logic modules] + end + end end end - dl --> ActiveRecord - dl --> o[Other domain services] + subgraph settingslayer[Remote Development: Settings layer] + settings[RemoteDevelopment::Settings module] + end + models[ActiveRecord models] + domainlogic --> otherdomainservices[Other domain services] + controllers --> apis + apis --> services + services --> domainlogic + domainlogic --> models + controllers --> settings + services --> settings + models --> settings end ``` @@ -85,7 +103,7 @@ Although Ruby is traditionally weakly-typed, without null safety and little supp ### Union types -We also simulate ["Union Types"](https://en.wikipedia.org/wiki/Union_type) in Ruby. We do this through the use of a module which defines multiple class constants of the same type. The `RemoteDevelopment::Messages` module is an example of this. +We also simulate ["Union Types"](https://en.wikipedia.org/wiki/Union_type) in Ruby. We do this through the use of a module which defines multiple class constants of the same type. The `RemoteDevelopment::Messages` module is an example of this. ### Pattern matching with types @@ -94,25 +112,25 @@ We also simulate ["Union Types"](https://en.wikipedia.org/wiki/Union_type) in Ru - The `case ... in` structure can be used to pattern-match on types. When used with the approach of throwing an exception in the `else` clause, this can provide exhaustive type checking at runtime. #### Rightward assignment pattern matching and destructuring with types - + Example: Given a `Hash` `x` with an entry `y` which is an `Integer`, the following code would destructure the integer into `i`: ```ruby x = {y: 1} x => {y: Integer => i} puts i # 1 -``` +``` If `y` was not an integer type, a `NoMatchingPatternError` runtime exception with a descriptive message would be thrown: ```ruby x = {y: "Not an Integer"} x => {y: Integer => i} # {:y=>"Not an Integer"}: Integer === "Not an integer" does not return true (NoMatchingPatternError) -``` +``` - This is a powerful new feature of Ruby 3 which allows for type safety without requiring the use of type safety tools such as RBS or Sorbet. - Although rightward pattern matching with types is still an [experimental feature](https://rubychangelog.com/versions-latest/), it has been stable with [little negative feedback](https://bugs.ruby-lang.org/issues/17260)). -- Also, Matz has [stated his committment to the support of rightward assignement for pattern matching](https://bugs.ruby-lang.org/issues/17260#note-1). +- Also, Matz has [stated his committment to the support of rightward assignement for pattern matching](https://bugs.ruby-lang.org/issues/17260#note-1). - But even if the experimental support for types in rightward assignment was removed, it would be straightforward to change all occurrences to remove the types and go back to regular rightward assignment. We would just lose the type safety. Also note that `#deconstruct_keys` must be implemented in order to use these pattern matching features. @@ -128,9 +146,9 @@ Also note that the destructuring a hash or array, even without the type checks ( ### Null safety When accessing a `Hash` entry by key, where we expect that the value must present (or otherwise an upstream bug exists), we prefer to use `Hash#fetch` -instead of `Hash#[]`. +instead of `Hash#[]`. -However, this is only necessary in cases cases where it is not possible or desireable to otherwise use type safety or +However, this is only necessary in cases cases where it is not possible or desireable to otherwise use type safety or ## Functional patterns @@ -148,7 +166,7 @@ Wherever possible, we use immutable state. This leads to fewer state-related bug ["Higher order functions"](https://en.wikipedia.org/wiki/Higher-order_function) are the basis of many (or most) functional patterns. Ruby supports this by allowing lambdas, procs, or method object references to be passed as arguments to other methods. -In the Remote Development feature, we accomplish this by passing lambdas or "singleton" (class) `Method` objects as arguments. +In the Remote Development feature, we accomplish this by passing lambdas or "singleton" (class) `Method` objects as arguments. Note that we do not use procs (and enforce their non-usage), because of their behavior with regard to arguments and the `return` keyword. @@ -193,7 +211,7 @@ We prefer mixins/modules instead of superclasses/inheritance for sharing code. T ### Other OO patterns -We _currently_ do not make heavy or explicit use of [dependency injection](https://en.wikipedia.org/wiki/Dependency_injection) or [composition over inheritance](https://en.wikipedia.org/wiki/Composition_over_inheritance). But, we may adopt these patterns in the future if they provide benefits. +We _currently_ do not make heavy or explicit use of [dependency injection](https://en.wikipedia.org/wiki/Dependency_injection) or [composition over inheritance](https://en.wikipedia.org/wiki/Composition_over_inheritance). But, we may adopt these patterns in the future if they provide benefits. ## Railway Oriented Programming and the Result class @@ -351,6 +369,80 @@ Since much of the Domain Logic layer logic is in classes with a single singleton We also tend to group all base `let` fixture declarations in the top-level global describe block rather than trying to sort them out into their specific contexts, for ease of writing/maintenance/readability/consistency. Only `let` declarations which override a global one of the same name are included in a specific context. +## Remote Development Settings + +### Overview of Remote Development Settings module + +Remote Development has a dedicated module in the domain logic for handling settings. It is +`RemoteDevelopment::Settings`. The goals of this module are: + +1. Allow various methods to be used for providing settings depending on which are appropriate, including: + - Default values + - One of the following: + - `::Gitlab::CurrentSettings` + - `::Settings` (not yet supported) + - Cascading settings via `CascadingNamespaceSettingAttribute` (not yet supported) + - Environment variables, with the required prefix of `GITLAB_REMOTE_DEVELOPMENT_` + - Any other current or future method +1. Perform type checking of provided settings. +1. Provide precedence rules (or validation errors/exceptions, if appropriate) if the same setting is + defined by multiple methods. See the [Precedence of settings](#precedence-of-settings) section + below for more details. +1. Allow usage of the same settings module from both the backend domain logic as well as the frontend + Vue components, by obtaining necessary settings values in the Rails controller and passing them + through to the frontend. +1. Use inversion of control, or dependency injection, to avoid coupling the the + Remote Development domain logic to the rest of the monolith. At the domain logic layer + all settings are injected and represented as a simple Hash. This injection approach also allows the + Settings module to be used from controllers, models, or services without introducing + direct circular dependencies at the class/module level. + +### Adding a new setting + +To add a new Remote Development setting with a default value which is automatically configurable +via an ENV var, add a new entry in `ee/lib/remote_development/settings/defaults_initializer.rb` + +### Reading settings + +To read a single setting, use `RemoteDevelopment::Settings.get_single_setting(:setting_name)` + +To read all settings, use `RemoteDevelopment::Settings.get_all_settings` + +NOTE: A setting _MUST_ have an entry defined in `ee/lib/remote_development/settings/defaults_initializer.rb` +to be read, but the default value can be `nil`. This will likely be the case when you want to use +a setting from `Gitlab::CurrentSettings`. + +### Precedence of settings + +If a setting can be defined via multiple means (e.g. via an `ENV` var the `Settings` model), +there is a clear and simple set of precedence rules for which one "wins". These follow the +order of the steps in the +[RoP `Main` class of the Remote Development Settings module](./settings/main.rb): + +1. First, the default value is used +1. Next, one of the following values are used if defined and not `nil`: + 1.`::Gitlab::CurrentSettings` + 1.`::Gitlab::Settings` (not yet implemented) + 1. Cascading settings via `CascadingNamespaceSettingAttribute` (not yet implemented) +1. Next, an ENV values is used if defined (i.e. not `nil`). The ENV var is intentionally placed as + the last step and highest precedence, so it can always be used to easily override any settings for + local or temporary testing. + +In future iterations, we may want to provide more control of where a specific setting comes from, +or providing specific precedence rules to override the default precedence rules. For example, we could +allow them to be specified as a new field in the defaults declaration: + +``` + def self.default_settings + # third/last field is "sources to read from and their order" + { + default_branch_name: [UNDEFINED, String, [:env, :current_settings]], # reads ENV var first, which can be overridden by CurrentSettings + default_max_hours_before_termination: [24, Integer, [:current_settings, :env]], # reads CurrentSettings first, which can be overridden by ENV var + max_hours_before_termination_limit: [120, Integer] # Uses default precedence + } + end +``` + ## FAQ ### Why is the Result class in the top level lib directory? diff --git a/ee/lib/remote_development/messages.rb b/ee/lib/remote_development/messages.rb index a0fa680302e04e..e632252dde6fbf 100644 --- a/ee/lib/remote_development/messages.rb +++ b/ee/lib/remote_development/messages.rb @@ -33,6 +33,10 @@ module Messages # Workspace reconcile errors WorkspaceReconcileParamsValidationFailed = Class.new(Message) + # Settings errors + SettingsEnvironmentVariableReadFailed = Class.new(Message) + SettingsCurrentSettingsReadFailed = Class.new(Message) + #--------------------------------------------------------- # Domain Events - message name should describe the outcome #--------------------------------------------------------- @@ -46,6 +50,9 @@ module Messages WorkspaceUpdateSuccessful = Class.new(Message) WorkspaceReconcileSuccessful = Class.new(Message) + # Settings domain events + SettingsGetSuccessful = Class.new(Message) + # Workspace DB events PersonalAccessTokenModelCreateFailed = Class.new(Message) WorkspaceModelCreateFailed = Class.new(Message) diff --git a/ee/lib/remote_development/settings.rb b/ee/lib/remote_development/settings.rb new file mode 100644 index 00000000000000..2ca11ebaf78192 --- /dev/null +++ b/ee/lib/remote_development/settings.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module Settings + extend PublicApi + end +end diff --git a/ee/lib/remote_development/settings/current_settings_reader.rb b/ee/lib/remote_development/settings/current_settings_reader.rb new file mode 100644 index 00000000000000..7b14cff650b68d --- /dev/null +++ b/ee/lib/remote_development/settings/current_settings_reader.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module Settings + class CurrentSettingsReader + include Messages + + # @param [Hash] value + # @return [Result] + def self.read(value) + err_result = nil + value[:settings].each_key do |setting_name| + next unless Gitlab::CurrentSettings.respond_to?(setting_name) + + current_setting_value = Gitlab::CurrentSettings.send(setting_name) # rubocop:disable GitlabSecurity/PublicSend -- No other way to programatically call dynamic class method + + next if current_setting_value.nil? + + setting_type = value[:setting_types][setting_name] + + unless current_setting_value.is_a?(setting_type) + # err_result will be set to a non-nil Result.err if type check fails + err_result = Result.err(SettingsCurrentSettingsReadFailed.new( + details: "Gitlab::CurrentSettings.#{setting_name} type of '#{current_setting_value.class}' " \ + "did not match initialized Remote Development Settings type of '#{setting_type}'." # rubocop:disable Layout/LineEndStringConcatenationIndentation -- use default RubyMine formatting + )) + end + + # CurrentSettings entry of correct type found for declared default setting, use its value as override + value[:settings][setting_name] = current_setting_value + end + + return err_result if err_result + + Result.ok(value) + end + end + end +end diff --git a/ee/lib/remote_development/settings/defaults_initializer.rb b/ee/lib/remote_development/settings/defaults_initializer.rb new file mode 100644 index 00000000000000..178badf42cbd91 --- /dev/null +++ b/ee/lib/remote_development/settings/defaults_initializer.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module Settings + class DefaultsInitializer + include Messages + + UNDEFINED = nil + + # ALL REMOTE DEVELOPMENT SETTINGS MUST BE DECLARED HERE. + # See ../README.md for more details. + # @return [Hash] + def self.default_settings + { + # NOTE: default_branch_name is not actually used by Remote Development, it is simply a placeholder to drive + # the logic for reading settings from ::Gitlab::CurrentSettings. It can be replaced when there is an + # actual Remote Development entry in ::Gitlab::CurrentSettings. + default_branch_name: [UNDEFINED, String], + default_max_hours_before_termination: [24, Integer], + max_hours_before_termination_limit: [120, Integer], + project_cloner_image_name: ['alpine/git', String], + project_cloner_image_tag: ['2.36.3', String] + } + end + + # @param [Hash] value + # @return [Hash] + def self.init(value) + value[:settings] = {} + value[:setting_types] = {} + + default_settings.each do |setting_name, setting_value_and_type| + unless setting_value_and_type.is_a?(Array) && setting_value_and_type.length == 2 + raise "Remote Development Setting entry for '#{setting_name}' must " \ + "be a two-element array containing the value and type." # rubocop:disable Layout/LineEndStringConcatenationIndentation -- This is being changed in https://gitlab.com/gitlab-org/ruby/gems/gitlab-styles/-/merge_requests/212 + end + + setting_value, setting_type = setting_value_and_type + + unless setting_type.is_a?(Class) + raise "Remote Development Setting type for '#{setting_name}' " \ + "must be a class, but it was a #{setting_type.class}." # rubocop:disable Layout/LineEndStringConcatenationIndentation -- This is being changed in https://gitlab.com/gitlab-org/ruby/gems/gitlab-styles/-/merge_requests/212 + end + + if !setting_value.nil? && !setting_value.is_a?(setting_type) + # NOTE: We are raising an exception here instead of returning a Result.err, because this is + # a coding syntax error in the 'default_settings', not a user or data error. + raise "Remote Development Setting '#{setting_name}' has a type of '#{setting_value.class}', " \ + "which does not match declared type of '#{setting_type}'." # rubocop:disable Layout/LineEndStringConcatenationIndentation -- This is being changed in https://gitlab.com/gitlab-org/ruby/gems/gitlab-styles/-/merge_requests/212 + end + + value[:settings][setting_name] = setting_value + value[:setting_types][setting_name] = setting_type + end + + value + end + end + end +end diff --git a/ee/lib/remote_development/settings/env_var_reader.rb b/ee/lib/remote_development/settings/env_var_reader.rb new file mode 100644 index 00000000000000..9c619bfd41ed7a --- /dev/null +++ b/ee/lib/remote_development/settings/env_var_reader.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module Settings + class EnvVarReader + include Messages + + REQUIRED_ENV_VAR_PREFIX = "GITLAB_REMOTE_DEVELOPMENT" + + # @param [Hash] value + # @return [Result] + def self.read(value) + err_result = nil + value[:settings].each_key do |setting_name| + env_var_name = "#{REQUIRED_ENV_VAR_PREFIX}_#{setting_name.to_s.upcase}" + env_var_value_string = ENV[env_var_name] + + # If there is no matching ENV var, break the loop and go to the next setting + next unless env_var_value_string + + begin + env_var_value = cast_value( + env_var_name: env_var_name, + env_var_value_string: env_var_value_string, + setting_type: value[:setting_types][setting_name] + ) + rescue RuntimeError => e + # err_result will be set to a non-nil Result.err if casting fails + err_result = Result.err(SettingsEnvironmentVariableReadFailed.new(details: e.message)) + end + + # ENV var matches an existing setting and is of the correct type, use its value to override the default value + value[:settings][setting_name] = env_var_value + end + + return err_result if err_result + + Result.ok(value) + end + + # @param [String] env_var_name + # @param [Integer,String] env_var_value_string + # @param [Class] setting_type + # @return [Object] + # @raise [RuntimeError] + def self.cast_value(env_var_name:, env_var_value_string:, setting_type:) + if setting_type == String + env_var_value_string + elsif setting_type == Integer + unless env_var_value_string.match?(/^-?[0123456789]+$/) + raise "ENV var '#{env_var_name}' value could not be cast to #{setting_type} type." + end + + env_var_value_string.to_i + else + raise "Unsupported Remote Development setting type: #{setting_type}" + end + end + end + end +end diff --git a/ee/lib/remote_development/settings/main.rb b/ee/lib/remote_development/settings/main.rb new file mode 100644 index 00000000000000..3c7b99bdf30077 --- /dev/null +++ b/ee/lib/remote_development/settings/main.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module Settings + class Main + include Messages + + extend MessageSupport + private_class_method :generate_error_response_from_message + + # @return [Hash] + # @raise [UnmatchedResultError] + def self.get_settings + initial_result = Result.ok({}) + + # The order of the chain determines the precedence of settings. I.e., defaults are + # overridden by env vars, and any subsequent steps override env vars. + result = + initial_result + .map(DefaultsInitializer.method(:init)) + .and_then(CurrentSettingsReader.method(:read)) + # NOTE: EnvVarReader is kept as last step, so it can always be used to easily override any settings for + # local or temporary testing. + .and_then(EnvVarReader.method(:read)) + .map( + # As the final step, return the settings in a SettingsGetSuccessful message + ->(value) do + RemoteDevelopment::Messages::SettingsGetSuccessful.new( + settings: value.fetch(:settings) + ) + end + ) + # rubocop:disable Lint/DuplicateBranch -- Rubocop doesn't know the branches are different due to destructuring + case result + in { err: SettingsEnvironmentVariableReadFailed => message } + generate_error_response_from_message(message: message, reason: :internal_server_error) + in { err: SettingsCurrentSettingsReadFailed => message } + generate_error_response_from_message(message: message, reason: :internal_server_error) + in { ok: SettingsGetSuccessful => message } + { settings: message.context.fetch(:settings), status: :success } + else + raise UnmatchedResultError.new(result: result) + end + # rubocop:enable Lint/DuplicateBranch + end + end + end +end diff --git a/ee/lib/remote_development/settings/public_api.rb b/ee/lib/remote_development/settings/public_api.rb new file mode 100644 index 00000000000000..82febf6f576b28 --- /dev/null +++ b/ee/lib/remote_development/settings/public_api.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module Settings + # The PublicApi module is the public interface that callers can use to retrieve settings values. + # + # It is extended in the RemoteDevelopment::Settings module, which makes all of its methods available to + # be called directly on the RemoteDevelopment::Settings module. + # + # It encapsulates the error handling and transformation of the "ServiceResponse" type structure returned by + # RemoteDevelopment::Settings::Main.get_settings + module PublicApi + # @param [Symbol] setting_name + # @return [Object] + # @raise [RuntimeError] + def get_single_setting(setting_name) + raise "Setting name must be a Symbol" unless setting_name.is_a?(Symbol) + + is_valid_setting_name = get_all_settings.key?(setting_name) + raise "Unsupported Remote Development setting name: '#{setting_name}'" unless is_valid_setting_name + + get_all_settings.fetch(setting_name) + end + + # @return [Hash] + # @raise [RuntimeError] + def get_all_settings + response_hash = RemoteDevelopment::Settings::Main.get_settings + + raise response_hash.fetch(:message).to_s if response_hash.fetch(:status) == :error + + response_hash.fetch(:settings).to_h + end + end + end +end diff --git a/ee/lib/remote_development/workspaces/create/project_cloner_component_injector.rb b/ee/lib/remote_development/workspaces/create/project_cloner_component_injector.rb index 36b4f11bce16a1..c28bd8746ae706 100644 --- a/ee/lib/remote_development/workspaces/create/project_cloner_component_injector.rb +++ b/ee/lib/remote_development/workspaces/create/project_cloner_component_injector.rb @@ -12,17 +12,20 @@ def self.inject(value) value => { processed_devfile: Hash => processed_devfile, volume_mounts: Hash => volume_mounts, - params: Hash => params + params: Hash => params, + settings: Hash => settings } volume_mounts => { data_volume: Hash => data_volume } data_volume => { path: String => volume_path } params => { project: Project => project } + settings => { + project_cloner_image_name: String => image_name, + project_cloner_image_tag: String => image_tag + } # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/408448 # replace the alpine/git docker image with one that is published by gitlab for security / reliability # reasons - image_name = 'alpine/git' - image_tag = '2.36.3' clone_dir = "#{volume_path}/#{project.path}" project_url = project.http_url_to_repo project_ref = project.default_branch diff --git a/ee/spec/frontend/remote_development/init_workspaces_app_spec.js b/ee/spec/frontend/remote_development/init_workspaces_app_spec.js index 5f790414895599..a2b099cce5c67e 100644 --- a/ee/spec/frontend/remote_development/init_workspaces_app_spec.js +++ b/ee/spec/frontend/remote_development/init_workspaces_app_spec.js @@ -1,6 +1,5 @@ import { createWrapper } from '@vue/test-utils'; import { injectVueAppBreadcrumbs } from '~/lib/utils/breadcrumbs'; -import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; import { initWorkspacesApp } from 'ee/remote_development/init_workspaces_app'; import WorkspaceList from 'ee/remote_development/pages/list.vue'; import WorkspacesBreadcrumbs from 'ee/remote_development/components/common/workspaces_breadcrumbs.vue'; @@ -10,22 +9,48 @@ jest.mock('~/lib/utils/breadcrumbs'); describe('ee/remote_development/init_workspaces_app', () => { let wrapper; + let el; - beforeEach(() => { - setHTMLFixture(`
`); - }); + const createAppRoot = () => { + el = document.createElement('div'); + el.setAttribute('id', 'js-workspaces'); + const options = { + workspaces_list_path: '/aaa', + empty_state_svg_path: '/bbb', + }; + el.dataset.options = JSON.stringify(options); + document.body.appendChild(el); + + const app = initWorkspacesApp(); + wrapper = createWrapper(app); + }; afterEach(() => { - resetHTMLFixture(); + el = null; }); describe('initWorkspacesApp - integration', () => { beforeEach(() => { - wrapper = createWrapper(initWorkspacesApp()); + createAppRoot(); + }); + + it('renders list component', () => { + const workspaceListComponent = wrapper.findComponent(WorkspaceList); + expect(workspaceListComponent.exists()).toBe(true); + }); + + it('renders create component', () => { + const createWorkspaceComponent = wrapper.findComponent(WorkspaceList); + expect(createWorkspaceComponent.exists()).toBe(true); }); - it('renders list state', () => { - expect(wrapper.findComponent(WorkspaceList).exists()).toBe(true); + it('provides options to components', () => { + const createWorkspaceComponent = wrapper.findComponent(WorkspaceList); + // eslint-disable-next-line no-underscore-dangle + expect(createWorkspaceComponent.vm._provided).toEqual({ + workspacesListPath: '/aaa', + emptyStateSvgPath: '/bbb', + }); }); it('inits breadcrumbs', () => { diff --git a/ee/spec/frontend/remote_development/pages/create_spec.js b/ee/spec/frontend/remote_development/pages/create_spec.js index a1e1a1e1d20751..e65e39eeef566d 100644 --- a/ee/spec/frontend/remote_development/pages/create_spec.js +++ b/ee/spec/frontend/remote_development/pages/create_spec.js @@ -33,6 +33,7 @@ jest.mock('~/lib/logger'); jest.mock('~/alert'); describe('remote_development/pages/create.vue', () => { + const DEFAULT_MAX_HOURS_BEFORE_TERMINATION = 42; const selectedProjectFixture = { fullPath: 'gitlab-org/gitlab', nameWithNamespace: 'GitLab Org / GitLab', @@ -96,6 +97,9 @@ describe('remote_development/pages/create.vue', () => { // noinspection JSCheckFunctionSignatures - TODO: Address in https://gitlab.com/gitlab-org/gitlab/-/issues/437600 wrapper = shallowMountExtended(WorkspaceCreate, { apolloProvider: mockApollo, + provide: { + defaultMaxHoursBeforeTermination: DEFAULT_MAX_HOURS_BEFORE_TERMINATION, + }, stubs: { GlFormSelect: GlFormSelectStub, GlSprintf, diff --git a/ee/spec/frontend/remote_development/router/index_spec.js b/ee/spec/frontend/remote_development/router/index_spec.js index ad808ba69c448f..752a7028d52fe1 100644 --- a/ee/spec/frontend/remote_development/router/index_spec.js +++ b/ee/spec/frontend/remote_development/router/index_spec.js @@ -19,6 +19,7 @@ Vue.use(VueRouter); Vue.use(VueApollo); const SVG_PATH = '/assets/illustrations/empty_states/empty_workspaces.svg'; +const DEFAULT_MAX_HOURS_BEFORE_TERMINATION = 42; describe('remote_development/router/index.js', () => { let router; @@ -51,6 +52,7 @@ describe('remote_development/router/index.js', () => { ]), provide: { emptyStateSvgPath: SVG_PATH, + defaultMaxHoursBeforeTermination: DEFAULT_MAX_HOURS_BEFORE_TERMINATION, }, stubs: { SearchProjectsListbox: { diff --git a/ee/spec/lib/remote_development/settings/current_settings_reader_spec.rb b/ee/spec/lib/remote_development/settings/current_settings_reader_spec.rb new file mode 100644 index 00000000000000..b2dee64c00d059 --- /dev/null +++ b/ee/spec/lib/remote_development/settings/current_settings_reader_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::RemoteDevelopment::Settings::CurrentSettingsReader, feature_category: :remote_development do + include ResultMatchers + + let(:overridden_setting_type) { String } + let(:overridden_setting_value_from_current_settings) { "value_from_current_settings" } + + let(:value) do + { + settings: { + non_overridden_setting: "not_overridden", + overridden_setting: "original_value" + }, + setting_types: { + non_overridden_setting: String, + overridden_setting: overridden_setting_type + } + } + end + + subject(:result) do + described_class.read(value) + end + + before do + create(:application_setting) + stub_application_setting(overridden_setting: overridden_setting_value_from_current_settings) + end + + context "when there are no errors" do + it "returns ::Gitlab::CurrentSettings overridden settings and non-overridden settings" do + expect(result).to eq(Result.ok( + { + settings: { + non_overridden_setting: "not_overridden", + overridden_setting: overridden_setting_value_from_current_settings + }, + setting_types: { + non_overridden_setting: String, + overridden_setting: String + } + } + )) + end + end + + context "when the type from GitLab::CurrentSettings does not match the declared remote development setting type" do + let(:overridden_setting_type) { Integer } + + it "returns an err Result containing a Gitlab::CurrentSettings read failed message with details" do + expect(result).to be_err_result( + RemoteDevelopment::Messages::SettingsCurrentSettingsReadFailed.new( + details: "Gitlab::CurrentSettings.overridden_setting type of 'String' " \ + "did not match initialized Remote Development Settings type of '#{overridden_setting_type}'." # rubocop:disable Layout/LineEndStringConcatenationIndentation -- use default RubyMine formatting + ) + ) + end + end +end diff --git a/ee/spec/lib/remote_development/settings/defaults_initializer_spec.rb b/ee/spec/lib/remote_development/settings/defaults_initializer_spec.rb new file mode 100644 index 00000000000000..4759f48a5b39ee --- /dev/null +++ b/ee/spec/lib/remote_development/settings/defaults_initializer_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require_relative '../fast_spec_helper' + +RSpec.describe RemoteDevelopment::Settings::DefaultsInitializer, feature_category: :remote_development do + let(:value) { {} } + + subject(:returned_value) do + described_class.init(value) + end + + context "when settings values and types all match" do + it "returns default settings and setting_types" do + expect(returned_value).to match( + { + settings: hash_including(default_max_hours_before_termination: 24), + setting_types: hash_including(default_max_hours_before_termination: Integer) + } + ) + end + end + + context "when a setting value has a type mismatch" do + before do + allow(described_class).to receive(:default_settings).and_return( + { + setting: ["not an integer", Integer] + } + ) + end + + it "raises a descriptive exception" do + expect { returned_value }.to raise_error( + "Remote Development Setting 'setting' has a type of 'String', which does not match declared type of 'Integer'." + ) + end + end + + context "when a default_settings entry is not an array" do + before do + allow(described_class).to receive(:default_settings).and_return( + { + setting: "Just a value" + } + ) + end + + it "raises a descriptive exception" do + expect { returned_value }.to raise_error( + "Remote Development Setting entry for 'setting' must be a two-element array containing the value and type." + ) + end + end + + context "when settings type is not specified as a Class" do + before do + allow(described_class).to receive(:default_settings).and_return( + { + setting: ["value", 1] + } + ) + end + + it "raises a descriptive exception" do + expect { returned_value }.to raise_error( + "Remote Development Setting type for 'setting' must be a class, but it was a Integer." + ) + end + end +end diff --git a/ee/spec/lib/remote_development/settings/env_var_reader_spec.rb b/ee/spec/lib/remote_development/settings/env_var_reader_spec.rb new file mode 100644 index 00000000000000..0ada0aa9753f02 --- /dev/null +++ b/ee/spec/lib/remote_development/settings/env_var_reader_spec.rb @@ -0,0 +1,116 @@ +# frozen_string_literal: true + +require_relative "../fast_spec_helper" + +RSpec.describe RemoteDevelopment::Settings::EnvVarReader, feature_category: :remote_development do + include ResultMatchers + + let(:default_setting_value) { 42 } + let(:setting_type) { Integer } + let(:value) do + { + settings: { + the_setting: default_setting_value + }, + setting_types: { + the_setting: setting_type + } + } + end + + subject(:result) do + described_class.read(value) + end + + before do + allow(ENV).to receive(:[]).and_call_original + allow(ENV).to receive(:[]).with(env_var_name) { env_var_value } + end + + context "when an ENV var overrides a default setting" do + let(:env_var_name) { "GITLAB_REMOTE_DEVELOPMENT_THE_SETTING" } + + context "when setting_type is String" do + let(:env_var_value) { "a string" } + let(:setting_type) { String } + + it "uses the string value of the overridden ENV var value" do + expect(result).to eq(Result.ok( + { + settings: { the_setting: env_var_value }, + setting_types: { the_setting: setting_type } + } + )) + end + end + + context "when setting_type is Integer" do + let(:env_var_value) { "999" } + let(:setting_type) { Integer } + + it "uses the casted type of the overridden ENV var value" do + expect(result).to eq(Result.ok( + { + settings: { the_setting: env_var_value.to_i }, + setting_types: { the_setting: setting_type } + } + )) + end + end + end + + context "when no ENV var overrides a default setting" do + let(:env_var_name) { "GITLAB_REMOTE_DEVELOPMENT_NON_MATCHING_SETTING" } + let(:env_var_value) { "0" } + + it "uses the default setting value which was passed" do + expect(result).to eq(Result.ok( + { + settings: { the_setting: default_setting_value }, + setting_types: { the_setting: setting_type } + } + )) + end + end + + context "when an ENV matches the pattern but there is no default setting value defined" do + let(:env_var_name) { "GITLAB_REMOTE_DEVELOPMENT_NONEXISTENT_SETTING" } + let(:env_var_value) { "maybe some old deprecated setting, doesn't matter, it's ignored" } + + it "ignores the ENV var" do + expect(result).to eq(Result.ok( + { + settings: { the_setting: default_setting_value }, + setting_types: { the_setting: setting_type } + } + )) + end + end + + context "when ENV var contains an incorrect type" do + let(:env_var_name) { "GITLAB_REMOTE_DEVELOPMENT_THE_SETTING" } + let(:env_var_value) { "not an Integer type" } + + it "returns an err Result containing a settings environment variable read failed message with details" do + expect(result).to be_err_result( + RemoteDevelopment::Messages::SettingsEnvironmentVariableReadFailed.new( + details: "ENV var '#{env_var_name}' value could not be cast to #{setting_type} type." + ) + ) + end + end + + context "when setting_type is an unsupported type" do + let(:env_var_name) { "GITLAB_REMOTE_DEVELOPMENT_THE_SETTING" } + let(:env_var_value) { "42" } + let(:setting_type) { Float } + + it "returns an err Result containing a settings environment variable read failed message with details" do + expect(result).to be_err_result( + RemoteDevelopment::Messages::SettingsEnvironmentVariableReadFailed.new( + details: "Unsupported Remote Development setting type: #{setting_type}" + ) + ) + end + end +end diff --git a/ee/spec/lib/remote_development/settings/main_spec.rb b/ee/spec/lib/remote_development/settings/main_spec.rb new file mode 100644 index 00000000000000..f13640fcb703b5 --- /dev/null +++ b/ee/spec/lib/remote_development/settings/main_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require_relative '../fast_spec_helper' + +RSpec.describe RemoteDevelopment::Settings::Main, feature_category: :remote_development do + include RemoteDevelopment::RailwayOrientedProgrammingHelpers + + let(:value) { {} } + let(:error_details) { 'some error details' } + let(:err_message_context) { { details: error_details } } + + # Classes + + let(:defaults_initializer_class) { RemoteDevelopment::Settings::DefaultsInitializer } + let(:current_settings_reader_class) { RemoteDevelopment::Settings::CurrentSettingsReader } + let(:env_var_reader_class) { RemoteDevelopment::Settings::EnvVarReader } + + # Methods + + let(:defaults_initializer_method) { defaults_initializer_class.singleton_method(:init) } + let(:current_settings_reader_method) { current_settings_reader_class.singleton_method(:read) } + let(:env_var_reader_method) { env_var_reader_class.singleton_method(:read) } + + # Subject + + subject(:response) { described_class.get_settings } + + before do + allow(defaults_initializer_class).to receive(:method) { defaults_initializer_method } + allow(current_settings_reader_class).to(receive(:method)) { current_settings_reader_method } + allow(env_var_reader_class).to(receive(:method)) { env_var_reader_method } + end + + context 'when the CurrentSettingsReader returns an err Result' do + before do + stub_methods_to_return_value(defaults_initializer_method) + stub_methods_to_return_err_result( + method: current_settings_reader_method, + message_class: RemoteDevelopment::Messages::SettingsCurrentSettingsReadFailed + ) + end + + it 'returns an error response' do + expect(response).to eq({ + status: :error, + message: "Settings current settings read failed: #{error_details}", + reason: :internal_server_error + }) + end + end + + context 'when the CurrentSettingsReader returns an ok Result' do + before do + stub_methods_to_return_value(defaults_initializer_method) + allow(current_settings_reader_method).to receive(:call).with(value) do + Result.ok({ settings: { int_setting: 1 } }) + end + end + + it 'returns a settings get success response with the settings as the payload' do + expect(response).to eq({ + status: :success, + settings: { int_setting: 1 } + }) + end + end + + context 'when the EnvVarReader returns an err Result' do + before do + stub_methods_to_return_value(defaults_initializer_method) + stub_methods_to_return_ok_result(current_settings_reader_method) + stub_methods_to_return_err_result( + method: env_var_reader_method, + message_class: RemoteDevelopment::Messages::SettingsEnvironmentVariableReadFailed + ) + end + + it 'returns an error response' do + expect(response).to eq({ + status: :error, + message: "Settings environment variable read failed: #{error_details}", + reason: :internal_server_error + }) + end + end + + context 'when the EnvVarReader returns an ok Result' do + before do + stub_methods_to_return_value(defaults_initializer_method) + stub_methods_to_return_ok_result(current_settings_reader_method) + allow(env_var_reader_method).to receive(:call).with(value) do + Result.ok({ settings: { int_setting: 1 } }) + end + end + + it 'returns a settings get success response with the settings as the payload' do + expect(response).to eq({ + status: :success, + settings: { int_setting: 1 } + }) + end + end + + context 'when an invalid Result is returned' do + before do + stub_methods_to_return_value(defaults_initializer_method) + stub_methods_to_return_ok_result(current_settings_reader_method) + stub_methods_to_return_err_result( + method: env_var_reader_method, + message_class: RemoteDevelopment::Messages::WorkspaceCreateSuccessful + ) + end + + it 'raises an UnmatchedResultError' do + expect { response }.to raise_error(RemoteDevelopment::UnmatchedResultError) + end + end +end diff --git a/ee/spec/lib/remote_development/settings/public_api_spec.rb b/ee/spec/lib/remote_development/settings/public_api_spec.rb new file mode 100644 index 00000000000000..da0bd5536c60c1 --- /dev/null +++ b/ee/spec/lib/remote_development/settings/public_api_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RemoteDevelopment::Settings::PublicApi, feature_category: :remote_development do + describe "get_single_setting" do + context "when passed a valid setting name" do + it "returns the setting value" do + expect(RemoteDevelopment::Settings.get_single_setting(:max_hours_before_termination_limit)).to eq(120) + end + end + + context "when passed an invalid setting name" do + it "raises an exception with a descriptive message" do + expect { RemoteDevelopment::Settings.get_single_setting(:invalid_setting_name) } + .to raise_error("Unsupported Remote Development setting name: 'invalid_setting_name'") + end + end + end + + describe "get_all_settings" do + it "returns a Hash containing all settings" do + expect(RemoteDevelopment::Settings.get_all_settings) + .to match(hash_including(max_hours_before_termination_limit: 120)) + end + end +end diff --git a/ee/spec/lib/remote_development/settings/settings_integration_spec.rb b/ee/spec/lib/remote_development/settings/settings_integration_spec.rb new file mode 100644 index 00000000000000..380d8545a460aa --- /dev/null +++ b/ee/spec/lib/remote_development/settings/settings_integration_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::RemoteDevelopment::Settings, feature_category: :remote_development do # rubocop:disable RSpec/FilePath -- Not sure why this is being flagged + subject(:settings_module) do + described_class + end + + context "when there is no override" do + before do + # Ensure the test doesn't fail if the setting's env var happens to be set in current environment + stub_env("GITLAB_REMOTE_DEVELOPMENT_MAX_HOURS_BEFORE_TERMINATION_LIMIT", nil) + end + + it "uses default value" do + expect(settings_module.get_single_setting(:max_hours_before_termination_limit)).to eq(120) + expect(settings_module.get_single_setting(:default_branch_name)).to be_nil + end + end + + context "when there is an env var override" do + before do + stub_env("GITLAB_REMOTE_DEVELOPMENT_MAX_HOURS_BEFORE_TERMINATION_LIMIT", "42") + end + + it "uses the env var override value and casts it" do + expect(settings_module.get_single_setting(:max_hours_before_termination_limit)).to eq(42) + end + end + + context "when there is and ENV var override and also a ::Gitlab::CurrentSettings override" do + let(:override_value_from_env) { "value_from_env" } + let(:override_value_from_current_settings) { "value_from_current_settings" } + + before do + stub_env("GITLAB_REMOTE_DEVELOPMENT_DEFAULT_BRANCH_NAME", override_value_from_env) + + create(:application_setting) + stub_application_setting(default_branch_name: override_value_from_current_settings) + end + + it "uses the ENV var value and not the CurrentSettings value" do + # fixture sanity check + expect(Gitlab::CurrentSettings.default_branch_name).to eq(override_value_from_current_settings) + + expect(settings_module.get_single_setting(:default_branch_name)).to eq(override_value_from_env) + end + end +end diff --git a/ee/spec/lib/remote_development/workspaces/create/main_integration_spec.rb b/ee/spec/lib/remote_development/workspaces/create/main_integration_spec.rb index e720ad181c0b24..cdb48009aeaf8d 100644 --- a/ee/spec/lib/remote_development/workspaces/create/main_integration_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/main_integration_spec.rb @@ -43,7 +43,14 @@ } end - let(:value) { { current_user: current_user, params: params } } + let(:settings) do + { + project_cloner_image_name: 'alpine/git', + project_cloner_image_tag: '2.36.3' + } + end + + let(:value) { { current_user: current_user, params: params, settings: settings } } subject(:response) do described_class.main(value) diff --git a/ee/spec/lib/remote_development/workspaces/create/project_cloner_component_injector_spec.rb b/ee/spec/lib/remote_development/workspaces/create/project_cloner_component_injector_spec.rb index d35515347fc37f..eb4845428a97f6 100644 --- a/ee/spec/lib/remote_development/workspaces/create/project_cloner_component_injector_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/project_cloner_component_injector_spec.rb @@ -25,6 +25,10 @@ data_volume: { path: "/projects" } + }, + settings: { + project_cloner_image_name: 'alpine/git', + project_cloner_image_tag: '2.36.3' } } end diff --git a/ee/spec/models/remote_development/workspace_spec.rb b/ee/spec/models/remote_development/workspace_spec.rb index aacdd94cea5c11..47b9332497d8c5 100644 --- a/ee/spec/models/remote_development/workspace_spec.rb +++ b/ee/spec/models/remote_development/workspace_spec.rb @@ -93,17 +93,28 @@ end describe 'validations' do - it 'validates max_hours_before_termination is no more than 120' do - workspace.max_hours_before_termination = described_class::MAX_HOURS_BEFORE_TERMINATION_LIMIT - expect(workspace).to be_valid + context "on max_hours_before_termination" do + let(:limit) { 42 } - workspace.max_hours_before_termination = described_class::MAX_HOURS_BEFORE_TERMINATION_LIMIT + 1 - expect(workspace).not_to be_valid + before do + allow(RemoteDevelopment::Settings) + .to receive(:get_single_setting).with(:max_hours_before_termination_limit) { limit } + end + + it 'validates max_hours_before_termination is no more than limit specified by settings' do + workspace.max_hours_before_termination = limit + expect(workspace).to be_valid + + workspace.max_hours_before_termination = limit + 1 + expect(workspace).not_to be_valid + end end - it 'validates editor is webide' do - workspace.editor = 'not-webide' - expect(workspace).not_to be_valid + context "on editor" do + it 'validates editor is webide' do + workspace.editor = 'not-webide' + expect(workspace).not_to be_valid + end end context 'on remote_development_agent_config' do @@ -265,23 +276,23 @@ let_it_be(:agent1, reload: true) { create(:ee_cluster_agent, :with_remote_development_agent_config) } let_it_be(:agent2, reload: true) { create(:ee_cluster_agent, :with_remote_development_agent_config) } let_it_be(:workspace1) do - create(:workspace, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::RUNNING) + create(:workspace, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::RUNNING) end let_it_be(:workspace2) do - create(:workspace, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::TERMINATED) + create(:workspace, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::TERMINATED) end let_it_be(:workspace3) do - create(:workspace, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::STOPPED) + create(:workspace, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::STOPPED) end let_it_be(:workspace4) do - create(:workspace, agent: agent2, desired_state: ::RemoteDevelopment::Workspaces::States::TERMINATED) + create(:workspace, agent: agent2, desired_state: ::RemoteDevelopment::Workspaces::States::TERMINATED) end let_it_be(:workspace5) do - create(:workspace, agent: agent2, desired_state: ::RemoteDevelopment::Workspaces::States::RUNNING) + create(:workspace, agent: agent2, desired_state: ::RemoteDevelopment::Workspaces::States::RUNNING) end it "returns the correct count for the current agent" do diff --git a/ee/spec/services/remote_development/agent_config/update_service_spec.rb b/ee/spec/services/remote_development/agent_config/update_service_spec.rb index ed20c3dfa1257c..1d87b7f8ef2544 100644 --- a/ee/spec/services/remote_development/agent_config/update_service_spec.rb +++ b/ee/spec/services/remote_development/agent_config/update_service_spec.rb @@ -5,6 +5,7 @@ RSpec.describe RemoteDevelopment::AgentConfig::UpdateService, feature_category: :remote_development do let(:agent) { instance_double(Clusters::Agent) } let(:config) { instance_double(Hash) } + let(:settings) { instance_double(Hash) } let(:agent_config) { instance_double(RemoteDevelopment::RemoteDevelopmentAgentConfig) } let(:licensed) { true } @@ -13,8 +14,9 @@ end before do + allow(RemoteDevelopment::Settings).to receive(:get_all_settings).and_return(settings) allow(RemoteDevelopment::AgentConfig::Main) - .to receive(:main).with(agent: agent, config: config).and_return(response_hash) + .to receive(:main).with(agent: agent, config: config, settings: settings).and_return(response_hash) end context 'when success' do diff --git a/ee/spec/services/remote_development/workspaces/create_service_spec.rb b/ee/spec/services/remote_development/workspaces/create_service_spec.rb index dbf51f75e27e2a..99607fffd11161 100644 --- a/ee/spec/services/remote_development/workspaces/create_service_spec.rb +++ b/ee/spec/services/remote_development/workspaces/create_service_spec.rb @@ -6,6 +6,7 @@ let(:workspace) { build_stubbed(:workspace) } let(:user) { instance_double(User) } let(:params) { instance_double(Hash) } + let(:settings) { instance_double(Hash) } describe '#execute' do subject(:service_response) do @@ -13,8 +14,9 @@ end before do + allow(RemoteDevelopment::Settings).to receive(:get_all_settings).and_return(settings) allow(RemoteDevelopment::Workspaces::Create::Main) - .to receive(:main).with(current_user: user, params: params).and_return(response_hash) + .to receive(:main).with(current_user: user, params: params, settings: settings).and_return(response_hash) end context 'when success' do diff --git a/ee/spec/services/remote_development/workspaces/reconcile_service_spec.rb b/ee/spec/services/remote_development/workspaces/reconcile_service_spec.rb index 8fc07f04b5d84d..905b86d5ce4168 100644 --- a/ee/spec/services/remote_development/workspaces/reconcile_service_spec.rb +++ b/ee/spec/services/remote_development/workspaces/reconcile_service_spec.rb @@ -5,10 +5,12 @@ RSpec.describe ::RemoteDevelopment::Workspaces::ReconcileService, feature_category: :remote_development do let(:agent) { instance_double(Clusters::Agent, id: 1) } let(:params) { instance_double(Hash) } + let(:settings) { instance_double(Hash) } let(:workspace_rails_infos) { [] } describe '#execute' do subject(:service_response) do + allow(RemoteDevelopment::Settings).to receive(:get_all_settings).and_return(settings) described_class.new.execute(agent: agent, params: params) end @@ -17,7 +19,8 @@ .to receive(:main).with( agent: agent, original_params: params, - logger: instance_of(RemoteDevelopment::Logger) + logger: instance_of(RemoteDevelopment::Logger), + settings: settings ).and_return(response_hash) end diff --git a/ee/spec/services/remote_development/workspaces/update_service_spec.rb b/ee/spec/services/remote_development/workspaces/update_service_spec.rb index feaa8b2e6aa088..52a60dcdf13e14 100644 --- a/ee/spec/services/remote_development/workspaces/update_service_spec.rb +++ b/ee/spec/services/remote_development/workspaces/update_service_spec.rb @@ -6,6 +6,7 @@ let(:workspace) { build_stubbed(:workspace) } let(:user) { instance_double(User) } let(:params) { instance_double(Hash) } + let(:settings) { instance_double(Hash) } describe '#execute' do subject(:service_response) do @@ -13,8 +14,14 @@ end before do + allow(RemoteDevelopment::Settings).to receive(:get_all_settings).and_return(settings) allow(RemoteDevelopment::Workspaces::Update::Main) - .to receive(:main).with(workspace: workspace, current_user: user, params: params).and_return(response_hash) + .to receive(:main).with( + workspace: workspace, + current_user: user, + params: params, + settings: settings + ).and_return(response_hash) end context 'when success' do -- GitLab From 72387f05a1001096a04054e23425ec51e58ac331 Mon Sep 17 00:00:00 2001 From: Chad Woolley Date: Tue, 27 Feb 2024 21:37:31 -0800 Subject: [PATCH 2/3] review feedback --- ee/lib/remote_development/README.md | 2 +- ee/lib/remote_development/settings/defaults_initializer.rb | 4 ++-- ee/lib/remote_development/settings/env_var_reader.rb | 3 ++- .../workspaces/create/project_cloner_component_injector.rb | 5 ++--- .../workspaces/create/main_integration_spec.rb | 3 +-- .../create/project_cloner_component_injector_spec.rb | 3 +-- 6 files changed, 9 insertions(+), 11 deletions(-) diff --git a/ee/lib/remote_development/README.md b/ee/lib/remote_development/README.md index a386790a6db6cc..f4df31f6bcc478 100644 --- a/ee/lib/remote_development/README.md +++ b/ee/lib/remote_development/README.md @@ -391,7 +391,7 @@ Remote Development has a dedicated module in the domain logic for handling setti 1. Allow usage of the same settings module from both the backend domain logic as well as the frontend Vue components, by obtaining necessary settings values in the Rails controller and passing them through to the frontend. -1. Use inversion of control, or dependency injection, to avoid coupling the the +1. Use inversion of control, or dependency injection, to avoid coupling the Remote Development domain logic to the rest of the monolith. At the domain logic layer all settings are injected and represented as a simple Hash. This injection approach also allows the Settings module to be used from controllers, models, or services without introducing diff --git a/ee/lib/remote_development/settings/defaults_initializer.rb b/ee/lib/remote_development/settings/defaults_initializer.rb index 178badf42cbd91..795d28d3fd2104 100644 --- a/ee/lib/remote_development/settings/defaults_initializer.rb +++ b/ee/lib/remote_development/settings/defaults_initializer.rb @@ -18,13 +18,13 @@ def self.default_settings default_branch_name: [UNDEFINED, String], default_max_hours_before_termination: [24, Integer], max_hours_before_termination_limit: [120, Integer], - project_cloner_image_name: ['alpine/git', String], - project_cloner_image_tag: ['2.36.3', String] + project_cloner_image: ['alpine/git:2.36.3', String] } end # @param [Hash] value # @return [Hash] + # @raise [RuntimeError] def self.init(value) value[:settings] = {} value[:setting_types] = {} diff --git a/ee/lib/remote_development/settings/env_var_reader.rb b/ee/lib/remote_development/settings/env_var_reader.rb index 9c619bfd41ed7a..09cc05dfd9adae 100644 --- a/ee/lib/remote_development/settings/env_var_reader.rb +++ b/ee/lib/remote_development/settings/env_var_reader.rb @@ -47,7 +47,8 @@ def self.cast_value(env_var_name:, env_var_value_string:, setting_type:) if setting_type == String env_var_value_string elsif setting_type == Integer - unless env_var_value_string.match?(/^-?[0123456789]+$/) + # NOTE: The following line works because String#to_i does not raise exceptions for non-integer values + unless env_var_value_string.to_i.to_s == env_var_value_string raise "ENV var '#{env_var_name}' value could not be cast to #{setting_type} type." end diff --git a/ee/lib/remote_development/workspaces/create/project_cloner_component_injector.rb b/ee/lib/remote_development/workspaces/create/project_cloner_component_injector.rb index c28bd8746ae706..e5aeb122c4be75 100644 --- a/ee/lib/remote_development/workspaces/create/project_cloner_component_injector.rb +++ b/ee/lib/remote_development/workspaces/create/project_cloner_component_injector.rb @@ -19,8 +19,7 @@ def self.inject(value) data_volume => { path: String => volume_path } params => { project: Project => project } settings => { - project_cloner_image_name: String => image_name, - project_cloner_image_tag: String => image_tag + project_cloner_image: String => image, } # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/408448 @@ -50,7 +49,7 @@ def self.inject(value) cloner_component = { 'name' => 'gl-cloner-injector', 'container' => { - 'image' => "#{image_name}:#{image_tag}", + 'image' => image, 'args' => [container_args], # command has been overridden here as the default command in the alpine/git # container invokes git directly diff --git a/ee/spec/lib/remote_development/workspaces/create/main_integration_spec.rb b/ee/spec/lib/remote_development/workspaces/create/main_integration_spec.rb index cdb48009aeaf8d..90e7bb0624040f 100644 --- a/ee/spec/lib/remote_development/workspaces/create/main_integration_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/main_integration_spec.rb @@ -45,8 +45,7 @@ let(:settings) do { - project_cloner_image_name: 'alpine/git', - project_cloner_image_tag: '2.36.3' + project_cloner_image: 'alpine/git:2.36.3' } end diff --git a/ee/spec/lib/remote_development/workspaces/create/project_cloner_component_injector_spec.rb b/ee/spec/lib/remote_development/workspaces/create/project_cloner_component_injector_spec.rb index eb4845428a97f6..0e59e31165f8fc 100644 --- a/ee/spec/lib/remote_development/workspaces/create/project_cloner_component_injector_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/project_cloner_component_injector_spec.rb @@ -27,8 +27,7 @@ } }, settings: { - project_cloner_image_name: 'alpine/git', - project_cloner_image_tag: '2.36.3' + project_cloner_image: 'alpine/git:2.36.3' } } end -- GitLab From a1196480a841272e5b48b65a63e00dfa63d18348 Mon Sep 17 00:00:00 2001 From: Chad Woolley Date: Tue, 27 Feb 2024 21:46:01 -0800 Subject: [PATCH 3/3] Apply patch from Paul --- .../remote_development/init_workspaces_app.js | 6 ++- .../init_workspaces_app_spec.js | 44 ++++++++----------- 2 files changed, 22 insertions(+), 28 deletions(-) diff --git a/ee/app/assets/javascripts/remote_development/init_workspaces_app.js b/ee/app/assets/javascripts/remote_development/init_workspaces_app.js index 9a08c8dc4ba121..4b7188bc27bdba 100644 --- a/ee/app/assets/javascripts/remote_development/init_workspaces_app.js +++ b/ee/app/assets/javascripts/remote_development/init_workspaces_app.js @@ -22,10 +22,12 @@ const initWorkspacesApp = () => { return null; } - const options = convertObjectPropsToCamelCase(JSON.parse(el.dataset.options)); + const { workspacesListPath, ...options } = convertObjectPropsToCamelCase( + JSON.parse(el.dataset.options), + ); // noinspection JSUnresolvedReference -- Avoids error on options property access. Adding `@returns {Object|Array}` to convertObjectPropsToCamelCase didn't help. TODO: Report and add to https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/ const router = createRouter({ - base: options.workspacesListPath, + base: workspacesListPath, }); injectVueAppBreadcrumbs(router, WorkspacesBreadcrumbs); return new Vue({ diff --git a/ee/spec/frontend/remote_development/init_workspaces_app_spec.js b/ee/spec/frontend/remote_development/init_workspaces_app_spec.js index a2b099cce5c67e..e386e42f939ca8 100644 --- a/ee/spec/frontend/remote_development/init_workspaces_app_spec.js +++ b/ee/spec/frontend/remote_development/init_workspaces_app_spec.js @@ -1,56 +1,48 @@ +import { escape } from 'lodash'; +import { GlEmptyState } from '@gitlab/ui'; import { createWrapper } from '@vue/test-utils'; import { injectVueAppBreadcrumbs } from '~/lib/utils/breadcrumbs'; import { initWorkspacesApp } from 'ee/remote_development/init_workspaces_app'; import WorkspaceList from 'ee/remote_development/pages/list.vue'; import WorkspacesBreadcrumbs from 'ee/remote_development/components/common/workspaces_breadcrumbs.vue'; +import { resetHTMLFixture, setHTMLFixture } from 'helpers/fixtures'; jest.mock('~/lib/logger'); jest.mock('~/lib/utils/breadcrumbs'); describe('ee/remote_development/init_workspaces_app', () => { let wrapper; - let el; - const createAppRoot = () => { - el = document.createElement('div'); - el.setAttribute('id', 'js-workspaces'); - const options = { + beforeEach(() => { + const options = JSON.stringify({ workspaces_list_path: '/aaa', empty_state_svg_path: '/bbb', - }; - el.dataset.options = JSON.stringify(options); - document.body.appendChild(el); + }); - const app = initWorkspacesApp(); - wrapper = createWrapper(app); - }; + setHTMLFixture(`
`); + }); afterEach(() => { - el = null; + resetHTMLFixture(); }); describe('initWorkspacesApp - integration', () => { beforeEach(() => { - createAppRoot(); + wrapper = createWrapper(initWorkspacesApp()); }); - it('renders list component', () => { - const workspaceListComponent = wrapper.findComponent(WorkspaceList); - expect(workspaceListComponent.exists()).toBe(true); + it('creates router', () => { + expect(wrapper.vm.$router.options.base).toBe('/aaa'); }); - it('renders create component', () => { - const createWorkspaceComponent = wrapper.findComponent(WorkspaceList); - expect(createWorkspaceComponent.exists()).toBe(true); + it('renders empty state', () => { + expect(wrapper.findComponent(GlEmptyState).props('svgPath')).toBe('/bbb'); }); - it('provides options to components', () => { - const createWorkspaceComponent = wrapper.findComponent(WorkspaceList); - // eslint-disable-next-line no-underscore-dangle - expect(createWorkspaceComponent.vm._provided).toEqual({ - workspacesListPath: '/aaa', - emptyStateSvgPath: '/bbb', - }); + it('renders list component', () => { + const workspaceListComponent = wrapper.findComponent(WorkspaceList); + + expect(workspaceListComponent.exists()).toBe(true); }); it('inits breadcrumbs', () => { -- GitLab