This repository has been archived by the owner on Apr 12, 2021. It is now read-only.
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
gitaly: revert a commit that broke config loading
- Loading branch information
Milan Pässler
committed
May 18, 2020
1 parent
47c8e52
commit e32bf64
Showing
2 changed files
with
324 additions
and
1 deletion.
There are no files selected for viewing
320 changes: 320 additions & 0 deletions
320
...-management/gitlab/gitaly/0001-Revert-Allow-gitlabshell-config-to-be-passed-in-thro.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,320 @@ | ||
| From 3df6278a00d29a0941cf0142848fac8bfba09ebf Mon Sep 17 00:00:00 2001 | ||
| From: =?UTF-8?q?Milan=20P=C3=A4ssler?= <me@pbb.lc> | ||
| Date: Tue, 19 May 2020 01:12:04 +0200 | ||
| Subject: [PATCH] Revert "Allow gitlabshell config to be passed in through a | ||
| config variable" | ||
|
|
||
| This reverts commit 14d8bae73b9e0d604c048d1cbcc1f4413c922bef. | ||
| --- | ||
| ruby/gitlab-shell/lib/gitlab_config.rb | 46 +++---- | ||
| ruby/gitlab-shell/lib/gitlab_init.rb | 10 +- | ||
| ruby/gitlab-shell/spec/gitlab_config_spec.rb | 116 +++++------------- | ||
| .../spec/gitlab_custom_hook_spec.rb | 2 +- | ||
| ruby/gitlab-shell/spec/gitlab_logger_spec.rb | 2 +- | ||
| ruby/gitlab-shell/spec/spec_helper.rb | 4 + | ||
| 6 files changed, 54 insertions(+), 126 deletions(-) | ||
|
|
||
| diff --git a/ruby/gitlab-shell/lib/gitlab_config.rb b/ruby/gitlab-shell/lib/gitlab_config.rb | ||
| index 9020ff7f..19f03c34 100644 | ||
| --- a/ruby/gitlab-shell/lib/gitlab_config.rb | ||
| +++ b/ruby/gitlab-shell/lib/gitlab_config.rb | ||
| @@ -2,46 +2,44 @@ require 'yaml' | ||
|
|
||
| class GitlabConfig | ||
| def secret_file | ||
| - fetch_from_config('secret_file', fetch_from_legacy_config('secret_file', File.join(ROOT_PATH, '.gitlab_shell_secret'))) | ||
| + fetch_from_legacy_config('secret_file',File.join(ROOT_PATH, '.gitlab_shell_secret')) | ||
| end | ||
|
|
||
| # Pass a default value because this is called from a repo's context; in which | ||
| # case, the repo's hooks directory should be the default. | ||
| # | ||
| def custom_hooks_dir(default: nil) | ||
| - fetch_from_config('custom_hooks_dir', fetch_from_legacy_config('custom_hooks_dir', File.join(ROOT_PATH, 'hooks'))) | ||
| + fetch_from_legacy_config('custom_hooks_dir', File.join(ROOT_PATH, 'hooks')) | ||
| end | ||
|
|
||
| def gitlab_url | ||
| - fetch_from_config('gitlab_url', fetch_from_legacy_config('gitlab_url',"http://localhost:8080").sub(%r{/*$}, '')) | ||
| + fetch_from_legacy_config('gitlab_url',"http://localhost:8080").sub(%r{/*$}, '') | ||
| end | ||
|
|
||
| def http_settings | ||
| - fetch_from_config('http_settings', fetch_from_legacy_config('http_settings', {})) | ||
| + fetch_from_legacy_config('http_settings', {}) | ||
| end | ||
|
|
||
| def log_file | ||
| - log_path = Pathname.new(fetch_from_config('log_path', LOG_PATH)) | ||
| + return File.join(LOG_PATH, 'gitlab-shell.log') unless LOG_PATH.empty? | ||
|
|
||
| - log_path = ROOT_PATH if log_path === '' | ||
| - | ||
| - return log_path.join('gitlab-shell.log') | ||
| + fetch_from_legacy_config('log_file', File.join(ROOT_PATH, 'gitlab-shell.log')) | ||
| end | ||
|
|
||
| def log_level | ||
| - log_level = fetch_from_config('log_level', LOG_LEVEL) | ||
| - | ||
| - return log_level unless log_level.empty? | ||
| + return LOG_LEVEL unless LOG_LEVEL.empty? | ||
|
|
||
| - 'INFO' | ||
| + fetch_from_legacy_config('log_level', 'INFO') | ||
| end | ||
|
|
||
| def log_format | ||
| - log_format = fetch_from_config('log_format', LOG_FORMAT) | ||
| + return LOG_FORMAT unless LOG_FORMAT.empty? | ||
|
|
||
| - return log_format unless log_format.empty? | ||
| + fetch_from_legacy_config('log_format', 'text') | ||
| + end | ||
|
|
||
| - 'text' | ||
| + def metrics_log_file | ||
| + fetch_from_legacy_config('metrics_log_file', File.join(ROOT_PATH, 'gitlab-shell-metrics.log')) | ||
| end | ||
|
|
||
| def to_json | ||
| @@ -53,6 +51,7 @@ class GitlabConfig | ||
| log_file: log_file, | ||
| log_level: log_level, | ||
| log_format: log_format, | ||
| + metrics_log_file: metrics_log_file | ||
| }.to_json | ||
| end | ||
|
|
||
| @@ -62,23 +61,8 @@ class GitlabConfig | ||
|
|
||
| private | ||
|
|
||
| - def fetch_from_config(key, default) | ||
| - value = config[key] | ||
| - | ||
| - return default if value.nil? || value.empty? | ||
| - | ||
| - value | ||
| - end | ||
| - | ||
| - def config | ||
| - @config ||= JSON.parse(ENV.fetch('GITALY_GITLAB_SHELL_CONFIG', '{}')) | ||
| - end | ||
| - | ||
| def legacy_config | ||
| # TODO: deprecate @legacy_config that is parsing the gitlab-shell config.yml | ||
| - legacy_file = ROOT_PATH.join('config.yml') | ||
| - return {} unless legacy_file.exist? | ||
| - | ||
| - @legacy_config ||= YAML.load_file(legacy_file) | ||
| + @legacy_config ||= YAML.load_file(File.join(ROOT_PATH, 'config.yml')) | ||
| end | ||
| end | ||
| diff --git a/ruby/gitlab-shell/lib/gitlab_init.rb b/ruby/gitlab-shell/lib/gitlab_init.rb | ||
| index 94913549..ce54e2d4 100644 | ||
| --- a/ruby/gitlab-shell/lib/gitlab_init.rb | ||
| +++ b/ruby/gitlab-shell/lib/gitlab_init.rb | ||
| @@ -1,10 +1,8 @@ | ||
| # GITLAB_SHELL_DIR has been deprecated | ||
| -require 'pathname' | ||
| - | ||
| -ROOT_PATH = Pathname.new(ENV['GITALY_GITLAB_SHELL_DIR'] || ENV['GITLAB_SHELL_DIR'] || File.expand_path('..', __dir__)).freeze | ||
| -LOG_PATH = Pathname.new(ENV.fetch('GITALY_LOG_DIR', ROOT_PATH)) | ||
| -LOG_LEVEL = ENV.fetch('GITALY_LOG_LEVEL', 'INFO') | ||
| -LOG_FORMAT = ENV.fetch('GITALY_LOG_FORMAT', 'text') | ||
| +ROOT_PATH = ENV['GITALY_GITLAB_SHELL_DIR'] || ENV['GITLAB_SHELL_DIR'] || File.expand_path('..', __dir__) | ||
| +LOG_PATH = ENV.fetch('GITALY_LOG_DIR', "") | ||
| +LOG_LEVEL = ENV.fetch('GITALY_LOG_LEVEL', "") | ||
| +LOG_FORMAT = ENV.fetch('GITALY_LOG_FORMAT', "") | ||
|
|
||
| # We are transitioning parts of gitlab-shell into the gitaly project. In | ||
| # gitaly, GITALY_EMBEDDED will be true. | ||
| diff --git a/ruby/gitlab-shell/spec/gitlab_config_spec.rb b/ruby/gitlab-shell/spec/gitlab_config_spec.rb | ||
| index b0f12381..6de0cd08 100644 | ||
| --- a/ruby/gitlab-shell/spec/gitlab_config_spec.rb | ||
| +++ b/ruby/gitlab-shell/spec/gitlab_config_spec.rb | ||
| @@ -1,112 +1,54 @@ | ||
| require_relative 'spec_helper' | ||
| require_relative '../lib/gitlab_config' | ||
| -require 'json' | ||
|
|
||
| describe GitlabConfig do | ||
| let(:config) { GitlabConfig.new } | ||
| + let(:config_data) { {} } | ||
|
|
||
| - context "without ENV['GITALY_GITLAB_SHELL_CONFIG'] is passed in" do | ||
| - let(:config_data) { {} } | ||
| + before { expect(YAML).to receive(:load_file).and_return(config_data) } | ||
|
|
||
| - before { expect(YAML).to receive(:load_file).and_return(config_data) } | ||
| + describe '#gitlab_url' do | ||
| + let(:url) { 'http://test.com' } | ||
|
|
||
| - describe '#gitlab_url' do | ||
| - let(:url) { 'http://test.com' } | ||
| + subject { config.gitlab_url } | ||
|
|
||
| - subject { config.gitlab_url } | ||
| + before { config_data['gitlab_url'] = url } | ||
|
|
||
| - before { config_data['gitlab_url'] = url } | ||
| + it { is_expected.not_to be_empty } | ||
| + it { is_expected.to eq(url) } | ||
|
|
||
| - it { is_expected.not_to be_empty } | ||
| - it { is_expected.to eq(url) } | ||
| - | ||
| - context 'remove trailing slashes' do | ||
| - before { config_data['gitlab_url'] = url + '//' } | ||
| - | ||
| - it { is_expected.to eq(url) } | ||
| - end | ||
| - end | ||
| - | ||
| - describe '#secret_file' do | ||
| - subject { config.secret_file } | ||
| + context 'remove trailing slashes' do | ||
| + before { config_data['gitlab_url'] = url + '//' } | ||
|
|
||
| - it 'returns ".gitlab_shell_secret" by default' do | ||
| - is_expected.to eq(File.join(File.expand_path('..', __dir__),'.gitlab_shell_secret')) | ||
| - end | ||
| - end | ||
| - | ||
| - describe '#fetch_from_legacy_config' do | ||
| - let(:key) { 'yaml_key' } | ||
| - | ||
| - where(:yaml_value, :default, :expected_value) do | ||
| - [ | ||
| - ['a', 'b', 'a'], | ||
| - [nil, 'b', 'b'], | ||
| - ['a', nil, 'a'], | ||
| - [nil, {}, {}] | ||
| - ] | ||
| - end | ||
| - | ||
| - with_them do | ||
| - it 'returns the correct value' do | ||
| - config_data[key] = yaml_value | ||
| - | ||
| - expect(config.fetch_from_legacy_config(key, default)).to eq(expected_value) | ||
| - end | ||
| - end | ||
| + it { is_expected.to eq(url) } | ||
| end | ||
| end | ||
|
|
||
| - context "when ENV['GITALY_GITLAB_SHELL_CONFIG'] is passed in" do | ||
| - let(:config_data) { {'secret_file': 'path/to/secret/file', | ||
| - 'custom_hooks_dir': '/path/to/custom_hooks', | ||
| - 'gitlab_url': 'http://localhost:123454', | ||
| - 'http_settings': {'user': 'user_123', 'password':'password123', 'ca_file': '/path/to/ca_file', 'ca_path': 'path/to/ca_path'}, | ||
| - 'log_path': '/path/to/log', | ||
| - 'log_level': 'myloglevel', | ||
| - 'log_format': 'log_format'} } | ||
| + describe '#log_format' do | ||
| + subject { config.log_format } | ||
|
|
||
| - before { allow(ENV).to receive(:fetch).with('GITALY_GITLAB_SHELL_CONFIG', '{}').and_return(config_data.to_json) } | ||
| - | ||
| - describe '#secret_file' do | ||
| - it 'returns the correct secret_file' do | ||
| - expect(config.secret_file).to eq(config_data[:secret_file]) | ||
| - end | ||
| - end | ||
| - | ||
| - describe '#custom_hooks_dir' do | ||
| - it 'returns the correct custom_hooks_dir' do | ||
| - expect(config.custom_hooks_dir).to eq(config_data[:custom_hooks_dir]) | ||
| - end | ||
| - end | ||
| - | ||
| - describe '#http_settings' do | ||
| - it 'returns the correct http_settings' do | ||
| - expect(config.http_settings).to eq(config_data[:http_settings].transform_keys(&:to_s)) | ||
| - end | ||
| + it 'returns "text" by default' do | ||
| + is_expected.to eq('text') | ||
| end | ||
| + end | ||
|
|
||
| - describe '#gitlab_url' do | ||
| - it 'returns the correct gitlab_url' do | ||
| - expect(config.gitlab_url).to eq(config_data[:gitlab_url]) | ||
| - end | ||
| - end | ||
| + describe '#fetch_from_legacy_config' do | ||
| + let(:key) { 'yaml_key' } | ||
|
|
||
| - describe '#log_path' do | ||
| - it 'returns the correct log_path' do | ||
| - expect(config.log_file).to eq(Pathname.new(File.join(config_data[:log_path], 'gitlab-shell.log'))) | ||
| - end | ||
| + where(:yaml_value, :default, :expected_value) do | ||
| + [ | ||
| + ['a', 'b', 'a'], | ||
| + [nil, 'b', 'b'], | ||
| + ['a', nil, 'a'], | ||
| + [nil, {}, {}] | ||
| + ] | ||
| end | ||
|
|
||
| - describe '#log_level' do | ||
| - it 'returns the correct log_level' do | ||
| - expect(config.log_level).to eq(config_data[:log_level]) | ||
| - end | ||
| - end | ||
| + with_them do | ||
| + it 'returns the correct value' do | ||
| + config_data[key] = yaml_value | ||
|
|
||
| - describe '#log_format' do | ||
| - it 'returns the correct log_format' do | ||
| - expect(config.log_format).to eq(config_data[:log_format]) | ||
| + expect(config.fetch_from_legacy_config(key, default)).to eq(expected_value) | ||
| end | ||
| end | ||
| end | ||
| diff --git a/ruby/gitlab-shell/spec/gitlab_custom_hook_spec.rb b/ruby/gitlab-shell/spec/gitlab_custom_hook_spec.rb | ||
| index c19b90f8..50865331 100644 | ||
| --- a/ruby/gitlab-shell/spec/gitlab_custom_hook_spec.rb | ||
| +++ b/ruby/gitlab-shell/spec/gitlab_custom_hook_spec.rb | ||
| @@ -99,7 +99,7 @@ describe GitlabCustomHook do | ||
| FileUtils.symlink(File.join(tmp_root_path, 'hooks'), File.join(tmp_repo_path, 'hooks')) | ||
| FileUtils.symlink(File.join(ROOT_PATH, 'config.yml.example'), File.join(tmp_root_path, 'config.yml')) | ||
|
|
||
| - stub_const('ROOT_PATH', Pathname.new(tmp_root_path)) | ||
| + stub_const('ROOT_PATH', tmp_root_path) | ||
| end | ||
|
|
||
| after do | ||
| diff --git a/ruby/gitlab-shell/spec/gitlab_logger_spec.rb b/ruby/gitlab-shell/spec/gitlab_logger_spec.rb | ||
| index 0523a4cc..0da64735 100644 | ||
| --- a/ruby/gitlab-shell/spec/gitlab_logger_spec.rb | ||
| +++ b/ruby/gitlab-shell/spec/gitlab_logger_spec.rb | ||
| @@ -142,7 +142,7 @@ describe GitlabLogger do | ||
| test_logger_status = system('bin/test-logger', msg) | ||
| expect(test_logger_status).to eq(true) | ||
|
|
||
| - grep_status = system('grep', '-q', '-e', msg, GitlabConfig.new.log_file.to_s) | ||
| + grep_status = system('grep', '-q', '-e', msg, GitlabConfig.new.log_file) | ||
| expect(grep_status).to eq(true) | ||
| end | ||
| end | ||
| diff --git a/ruby/gitlab-shell/spec/spec_helper.rb b/ruby/gitlab-shell/spec/spec_helper.rb | ||
| index 684f87b1..be5ff9c6 100644 | ||
| --- a/ruby/gitlab-shell/spec/spec_helper.rb | ||
| +++ b/ruby/gitlab-shell/spec/spec_helper.rb | ||
| @@ -10,4 +10,8 @@ Dir[File.expand_path('support/**/*.rb', __dir__)].each { |f| require f } | ||
| RSpec.configure do |config| | ||
| config.run_all_when_everything_filtered = true | ||
| config.filter_run :focus | ||
| + | ||
| + config.before(:each) do | ||
| + stub_const('ROOT_PATH', File.expand_path('..', __dir__)) | ||
| + end | ||
| end | ||
| -- | ||
| 2.26.2 | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters