diff --git a/ee/app/assets/javascripts/remote_development/constants.js b/ee/app/assets/javascripts/remote_development/constants.js index e5c1da090bd25b95081e28a54aa2fd65ffc69de0..13cfbca1ef005c31d0cc28662c2cc0c1de4c94cb 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 41b0d6509866051c603d39111e14005f3667e49e..4b7188bc27bdbaaad10c099107676c91bf5fadc9 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,7 +22,10 @@ const initWorkspacesApp = () => { return null; } - const { workspacesListPath, emptyStateSvgPath } = el.dataset; + 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: workspacesListPath, }); @@ -31,10 +35,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 596118354a651bd6463370d4c553d3eda3ea5c6d..cd0bad1e99c1b40ba397be07fbf640720435b10d 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 6e09f2d8337f926a8394fdb3d8e75db09303863d..7d05999cf05b7e2f72b63f18daacb40e222fed35 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 40e1737a4e53be746319b1da254ad536d84469f8..5ac49cfc2547e3960acea766284eaf9b33ea3a4a 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 cf29b2cd781dd6e72437d342c91d3938b0b61068..2c919f1549dd3ff4af24e1b810750ce57293c573 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 a5d1980678de4bef95b7a280c4fa0cdd4d423d82..cd5bac11badbfe237144fc1a30191f19cb32680a 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 8e775b28dcb61d1e5ebceb5f3ef957e1b716d759..9f5c2291f47e502ea55b083d496b16fba22516d8 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 d788375446f3ae24ae76d6df0b1a986ae1cc2fac..f0e60c614ea80c7ea0036b3f918df79f569f1b62 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 cfc35ca66f405cf19c1442147764a5d9fd643a72..70cd791505401c2bd608d963e6e2c4a569c6f59d 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 21b22703c28075c56da3ef2313a7b5f3b6b5181b..f4df31f6bcc47850e23a6944d93f5c235aaabd4e 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 + 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 a0fa680302e04ef986004e82687a5c99da0eb40c..e632252dde6fbf07d3df74c3f42b1a17e7d8916f 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 0000000000000000000000000000000000000000..2ca11ebaf781928ff553d1aca2c5587a41a0c984 --- /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 0000000000000000000000000000000000000000..7b14cff650b68d9146b501e751ed5056735b38ae --- /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 0000000000000000000000000000000000000000..795d28d3fd2104fe3218123d639868315aa88f36 --- /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: ['alpine/git:2.36.3', String] + } + end + + # @param [Hash] value + # @return [Hash] + # @raise [RuntimeError] + 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 0000000000000000000000000000000000000000..09cc05dfd9adae908527b0e479a91ea5572864b4 --- /dev/null +++ b/ee/lib/remote_development/settings/env_var_reader.rb @@ -0,0 +1,62 @@ +# 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 + # 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 + + 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 0000000000000000000000000000000000000000..3c7b99bdf30077b34e2a75349fc97001bc3a560b --- /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 0000000000000000000000000000000000000000..82febf6f576b282d8a443bb9d12bcff00faf7024 --- /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 36b4f11bce16a18d9f7a84298a122a2e1f372d8e..e5aeb122c4be758298513f27cac754cf6ac8d52d 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,19 @@ 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: String => image, + } # 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 @@ -47,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/frontend/remote_development/init_workspaces_app_spec.js b/ee/spec/frontend/remote_development/init_workspaces_app_spec.js index 5f7904148955994c4f2debc8130e5f195ebc6d92..e386e42f939ca8c22e7aafc58e08dbd066776c9b 100644 --- a/ee/spec/frontend/remote_development/init_workspaces_app_spec.js +++ b/ee/spec/frontend/remote_development/init_workspaces_app_spec.js @@ -1,9 +1,11 @@ +import { escape } from 'lodash'; +import { GlEmptyState } from '@gitlab/ui'; 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'; +import { resetHTMLFixture, setHTMLFixture } from 'helpers/fixtures'; jest.mock('~/lib/logger'); jest.mock('~/lib/utils/breadcrumbs'); @@ -12,7 +14,12 @@ describe('ee/remote_development/init_workspaces_app', () => { let wrapper; beforeEach(() => { - setHTMLFixture(`
`); + const options = JSON.stringify({ + workspaces_list_path: '/aaa', + empty_state_svg_path: '/bbb', + }); + + setHTMLFixture(`
`); }); afterEach(() => { @@ -24,8 +31,18 @@ describe('ee/remote_development/init_workspaces_app', () => { wrapper = createWrapper(initWorkspacesApp()); }); - it('renders list state', () => { - expect(wrapper.findComponent(WorkspaceList).exists()).toBe(true); + it('creates router', () => { + expect(wrapper.vm.$router.options.base).toBe('/aaa'); + }); + + it('renders empty state', () => { + expect(wrapper.findComponent(GlEmptyState).props('svgPath')).toBe('/bbb'); + }); + + it('renders list component', () => { + const workspaceListComponent = wrapper.findComponent(WorkspaceList); + + expect(workspaceListComponent.exists()).toBe(true); }); 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 a1e1a1e1d20751c8b9a41d39feed3b9067a484e1..e65e39eeef566df2bb936ec2243a5f1774ef09d8 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 ad808ba69c448ff960b1fd605f50a1914c348a78..752a7028d52fe1d1213fb5a36acd30b7af6348d5 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 0000000000000000000000000000000000000000..b2dee64c00d059994c128c0d1a83dc737096ccdf --- /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 0000000000000000000000000000000000000000..4759f48a5b39eefb19462d8be973fff41dd0998f --- /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 0000000000000000000000000000000000000000..0ada0aa9753f02391c15590b1d674df48c1baaba --- /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 0000000000000000000000000000000000000000..f13640fcb703b506b41344b6955156605e0ca96d --- /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 0000000000000000000000000000000000000000..da0bd5536c60c1ff538e13f9965a4f835e53fe29 --- /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 0000000000000000000000000000000000000000..380d8545a460aa34e1527eb93e0dac6d9143bbd7 --- /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 e720ad181c0b249364d1a0e50a1bd30cae225f07..90e7bb0624040f199fee6e7665f466b2580fc1b7 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,13 @@ } end - let(:value) { { current_user: current_user, params: params } } + let(:settings) do + { + project_cloner_image: 'alpine/git: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 d35515347fc37f871fe4b9b85e088888ffd8032d..0e59e31165f8fca2b25c01893cd61a3178e601aa 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,9 @@ data_volume: { path: "/projects" } + }, + settings: { + project_cloner_image: 'alpine/git: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 aacdd94cea5c115aeb3e9f103bc950293bb98daa..47b9332497d8c5c1742f42964cd55fa23329281d 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 ed20c3dfa1257c557725e193a2988e0ee71beffa..1d87b7f8ef25442c60d3ab58d9d841a40417c7bd 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 dbf51f75e27e2a804dfaa09faa4ba28571d76e3d..99607fffd11161f8867709b995ce6ecde9b9f053 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 8fc07f04b5d84d6852f3d42992d710b9b735fc8a..905b86d5ce4168c2d68974da741dba83ceb309ce 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 feaa8b2e6aa0887dc9ac63eba87b0a26b550184a..52a60dcdf13e1491105530f818c9520f07834aa2 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