From 07fd5b037006885f1cdd6ee334dfb9b906ea4018 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Fri, 28 Feb 2020 09:00:52 +0000 Subject: [PATCH 1/2] Remove `Configurable#helpers` method Simplify configurations by defining helpers by default --- ee/lib/gitlab/web_ide/config/entry/global.rb | 2 -- .../gitlab/web_ide/config/entry/terminal.rb | 2 -- lib/gitlab/ci/config/entry/artifacts.rb | 2 -- lib/gitlab/ci/config/entry/bridge.rb | 2 -- lib/gitlab/ci/config/entry/cache.rb | 2 -- lib/gitlab/ci/config/entry/default.rb | 2 -- lib/gitlab/ci/config/entry/job.rb | 5 --- lib/gitlab/ci/config/entry/processable.rb | 2 -- lib/gitlab/ci/config/entry/release.rb | 2 -- lib/gitlab/ci/config/entry/release/assets.rb | 2 -- lib/gitlab/ci/config/entry/root.rb | 2 -- lib/gitlab/ci/config/entry/service.rb | 33 +++++++++++++++++-- lib/gitlab/config/entry/configurable.rb | 11 +++++-- 13 files changed, 40 insertions(+), 29 deletions(-) diff --git a/ee/lib/gitlab/web_ide/config/entry/global.rb b/ee/lib/gitlab/web_ide/config/entry/global.rb index 58818903442773..50c3f2d294f407 100644 --- a/ee/lib/gitlab/web_ide/config/entry/global.rb +++ b/ee/lib/gitlab/web_ide/config/entry/global.rb @@ -21,8 +21,6 @@ class Global < ::Gitlab::Config::Entry::Node entry :terminal, Entry::Terminal, description: 'Configuration of the webide terminal.' - helpers :terminal - attributes :terminal end end diff --git a/ee/lib/gitlab/web_ide/config/entry/terminal.rb b/ee/lib/gitlab/web_ide/config/entry/terminal.rb index b0121e0f6f65e4..403e308d45b418 100644 --- a/ee/lib/gitlab/web_ide/config/entry/terminal.rb +++ b/ee/lib/gitlab/web_ide/config/entry/terminal.rb @@ -42,8 +42,6 @@ class Terminal < ::Gitlab::Config::Entry::Node entry :variables, ::Gitlab::Ci::Config::Entry::Variables, description: 'Environment variables available for this job.' - helpers :before_script, :script, :image, :variables, :services - attributes :tags def value diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index aebc1675bec923..241c73db3bbf26 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -44,8 +44,6 @@ class Artifacts < ::Gitlab::Config::Entry::Node end end - helpers :reports - def value @config[:reports] = reports_value if @config.key?(:reports) @config diff --git a/lib/gitlab/ci/config/entry/bridge.rb b/lib/gitlab/ci/config/entry/bridge.rb index 721c7c8b6d7062..6fdaa3731706b2 100644 --- a/lib/gitlab/ci/config/entry/bridge.rb +++ b/lib/gitlab/ci/config/entry/bridge.rb @@ -49,8 +49,6 @@ class Bridge < ::Gitlab::Config::Entry::Node description: 'Environment variables available for this job.', inherit: false - helpers :trigger, :needs, :variables - attributes :when, :allow_failure def self.matching?(name, config) diff --git a/lib/gitlab/ci/config/entry/cache.rb b/lib/gitlab/ci/config/entry/cache.rb index ef07c319ce4bcb..a304d9b724f920 100644 --- a/lib/gitlab/ci/config/entry/cache.rb +++ b/lib/gitlab/ci/config/entry/cache.rb @@ -28,8 +28,6 @@ class Cache < ::Gitlab::Config::Entry::Node entry :paths, Entry::Paths, description: 'Specify which paths should be cached across builds.' - helpers :key - attributes :policy def value diff --git a/lib/gitlab/ci/config/entry/default.rb b/lib/gitlab/ci/config/entry/default.rb index 88db17a75da7cb..ab493ff7d78625 100644 --- a/lib/gitlab/ci/config/entry/default.rb +++ b/lib/gitlab/ci/config/entry/default.rb @@ -61,8 +61,6 @@ class Default < ::Gitlab::Config::Entry::Node description: 'Default artifacts.', inherit: false - helpers :before_script, :image, :services, :after_script, :cache - private def overwrite_entry(deps, key, current_entry) diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 931f769e920b6b..8db21b116eb499 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -128,11 +128,6 @@ class Job < ::Gitlab::Config::Entry::Node description: 'This job will produce a release.', inherit: false - helpers :before_script, :script, :type, :after_script, - :cache, :image, :services, :variables, - :artifacts, :environment, :coverage, :retry, - :needs, :interruptible, :release, :tags - attributes :script, :tags, :allow_failure, :when, :dependencies, :needs, :retry, :parallel, :start_in, :interruptible, :timeout, :resource_group, :release diff --git a/lib/gitlab/ci/config/entry/processable.rb b/lib/gitlab/ci/config/entry/processable.rb index 19e6601e31f4f7..bfa2905ed77e9d 100644 --- a/lib/gitlab/ci/config/entry/processable.rb +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -54,8 +54,6 @@ module Processable allowed_when: %w[on_success on_failure always never manual delayed].freeze } - helpers :stage, :only, :except, :rules - attributes :extends, :rules end diff --git a/lib/gitlab/ci/config/entry/release.rb b/lib/gitlab/ci/config/entry/release.rb index 3eceaa0ccd994b..b4e4c1497306b7 100644 --- a/lib/gitlab/ci/config/entry/release.rb +++ b/lib/gitlab/ci/config/entry/release.rb @@ -33,8 +33,6 @@ def description validates :description, type: String, presence: true end - helpers :assets - def value @config[:assets] = assets_value if @config.key?(:assets) @config diff --git a/lib/gitlab/ci/config/entry/release/assets.rb b/lib/gitlab/ci/config/entry/release/assets.rb index 82ed39f51e01b3..1f7057d1bf6008 100644 --- a/lib/gitlab/ci/config/entry/release/assets.rb +++ b/lib/gitlab/ci/config/entry/release/assets.rb @@ -23,8 +23,6 @@ class Assets < ::Gitlab::Config::Entry::Node validates :links, array_of_hashes: true, presence: true end - helpers :links - def value @config[:links] = links_value if @config.key?(:links) @config diff --git a/lib/gitlab/ci/config/entry/root.rb b/lib/gitlab/ci/config/entry/root.rb index 620f6a95e9dcf7..e54e3f641941a4 100644 --- a/lib/gitlab/ci/config/entry/root.rb +++ b/lib/gitlab/ci/config/entry/root.rb @@ -67,8 +67,6 @@ class Root < ::Gitlab::Config::Entry::Node entry :workflow, Entry::Workflow, description: 'List of evaluable rules to determine Pipeline status' - helpers :default, :stages, :types, :variables, :workflow - helpers :jobs, dynamic: true delegate :before_script_value, diff --git a/lib/gitlab/ci/config/entry/service.rb b/lib/gitlab/ci/config/entry/service.rb index 8d16371e857b4f..c3a026103d487a 100644 --- a/lib/gitlab/ci/config/entry/service.rb +++ b/lib/gitlab/ci/config/entry/service.rb @@ -7,8 +7,12 @@ module Entry ## # Entry that represents a configuration of Docker service. # - class Service < Image + # TODO: remove duplication with Image superclass by defining a common + # Imageable concern. + class Service < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Validatable + include ::Gitlab::Config::Entry::Attributable + include ::Gitlab::Config::Entry::Configurable ALLOWED_KEYS = %i[name entrypoint command alias ports].freeze @@ -16,9 +20,9 @@ class Service < Image validates :config, hash_or_string: true validates :config, allowed_keys: ALLOWED_KEYS validates :config, disallowed_keys: %i[ports], unless: :with_image_ports? - validates :name, type: String, presence: true validates :entrypoint, array_of_strings: true, allow_nil: true + validates :command, array_of_strings: true, allow_nil: true validates :alias, type: String, allow_nil: true validates :alias, type: String, presence: true, unless: ->(record) { record.ports.blank? } @@ -27,6 +31,8 @@ class Service < Image entry :ports, Entry::Ports, description: 'Ports used to expose the service' + attributes :ports + def alias value[:alias] end @@ -34,6 +40,29 @@ def alias def command value[:command] end + + def name + value[:name] + end + + def entrypoint + value[:entrypoint] + end + + def value + return { name: @config } if string? + return @config if hash? + + {} + end + + def with_image_ports? + opt(:with_image_ports) + end + + def skip_config_hash_validation? + true + end end end end diff --git a/lib/gitlab/config/entry/configurable.rb b/lib/gitlab/config/entry/configurable.rb index 75e15cd8cb1d36..dd2e6175c3ccb2 100644 --- a/lib/gitlab/config/entry/configurable.rb +++ b/lib/gitlab/config/entry/configurable.rb @@ -75,7 +75,8 @@ def reserved_node_names # rubocop: disable CodeReuse/ActiveRecord def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: nil, metadata: {}) - raise ArgumentError, "Entry #{key} already defined" if @nodes.to_h[key.to_sym] + entry_name = key.to_sym + raise ArgumentError, "Entry #{key} already defined" if @nodes.to_h[entry_name] factory = ::Gitlab::Config::Entry::Factory.new(entry) .with(description: description) @@ -84,7 +85,13 @@ def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: ni .with(reserved: reserved) .metadata(metadata) - (@nodes ||= {}).merge!(key.to_sym => factory) + (@nodes ||= {})[entry_name] = factory + + if method_defined?("#{entry_name}_defined?") || method_defined?("#{entry_name}_value") + raise ArgumentError, "Method #{entry_name}_defined? or #{entry_name}_value already defined" + end + + helpers(entry_name) end # rubocop: enable CodeReuse/ActiveRecord -- GitLab From 0fd2a5cb936cbd76bf95ffbcb3f09d6c44c29f82 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Tue, 3 Mar 2020 11:04:53 +0000 Subject: [PATCH 2/2] Define dynamic_helpers for special cases --- lib/gitlab/ci/config/entry/root.rb | 2 +- lib/gitlab/ci/config/entry/service.rb | 1 + lib/gitlab/config/entry/configurable.rb | 11 ++++++----- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/ci/config/entry/root.rb b/lib/gitlab/ci/config/entry/root.rb index e54e3f641941a4..caa0725c4bdbc8 100644 --- a/lib/gitlab/ci/config/entry/root.rb +++ b/lib/gitlab/ci/config/entry/root.rb @@ -67,7 +67,7 @@ class Root < ::Gitlab::Config::Entry::Node entry :workflow, Entry::Workflow, description: 'List of evaluable rules to determine Pipeline status' - helpers :jobs, dynamic: true + dynamic_helpers :jobs delegate :before_script_value, :image_value, diff --git a/lib/gitlab/ci/config/entry/service.rb b/lib/gitlab/ci/config/entry/service.rb index c3a026103d487a..247bf930d3b9e8 100644 --- a/lib/gitlab/ci/config/entry/service.rb +++ b/lib/gitlab/ci/config/entry/service.rb @@ -9,6 +9,7 @@ module Entry # # TODO: remove duplication with Image superclass by defining a common # Imageable concern. + # https://gitlab.com/gitlab-org/gitlab/issues/208774 class Service < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Validatable include ::Gitlab::Config::Entry::Attributable diff --git a/lib/gitlab/config/entry/configurable.rb b/lib/gitlab/config/entry/configurable.rb index dd2e6175c3ccb2..3fd562c2904073 100644 --- a/lib/gitlab/config/entry/configurable.rb +++ b/lib/gitlab/config/entry/configurable.rb @@ -85,16 +85,17 @@ def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: ni .with(reserved: reserved) .metadata(metadata) - (@nodes ||= {})[entry_name] = factory - - if method_defined?("#{entry_name}_defined?") || method_defined?("#{entry_name}_value") - raise ArgumentError, "Method #{entry_name}_defined? or #{entry_name}_value already defined" - end + @nodes ||= {} + @nodes[entry_name] = factory helpers(entry_name) end # rubocop: enable CodeReuse/ActiveRecord + def dynamic_helpers(*nodes) + helpers(*nodes, dynamic: true) + end + def helpers(*nodes, dynamic: false) nodes.each do |symbol| if method_defined?("#{symbol}_defined?") || method_defined?("#{symbol}_value") -- GitLab