diff --git a/CHANGELOG.md b/CHANGELOG.md index 763e3c91..6d3d61cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +### 7.4.0 +* Warn when `KNAPSACK_PRO_*` environment variables are set manually if their values could be automatically determined from supported CI environments. + + https://github.com/KnapsackPro/knapsack_pro-ruby/pull/254 + +https://github.com/KnapsackPro/knapsack_pro-ruby/compare/v7.3.0...v7.4.0 + ### 7.3.0 * [Queue Mode][RSpec] Pass each batch of tests to the queue hooks: `KnapsackPro::Hooks::Queue.before_subset_queue` and `KnapsackPro::Hooks::Queue.after_subset_queue` diff --git a/lib/knapsack_pro/config/env.rb b/lib/knapsack_pro/config/env.rb index eeeecb38..ce4fd33e 100644 --- a/lib/knapsack_pro/config/env.rb +++ b/lib/knapsack_pro/config/env.rb @@ -13,30 +13,21 @@ class Env class << self def ci_node_total - (ENV['KNAPSACK_PRO_CI_NODE_TOTAL'] || - ci_env_for(:node_total) || - 1).to_i + (env_for('KNAPSACK_PRO_CI_NODE_TOTAL', :node_total) || 1).to_i end def ci_node_index - (ENV['KNAPSACK_PRO_CI_NODE_INDEX'] || - ci_env_for(:node_index) || - 0).to_i + (env_for('KNAPSACK_PRO_CI_NODE_INDEX', :node_index) || 0).to_i end def ci_node_build_id env_name = 'KNAPSACK_PRO_CI_NODE_BUILD_ID' - ENV[env_name] || - ci_env_for(:node_build_id) || + env_for(env_name, :node_build_id) || raise("Missing environment variable #{env_name}. Read more at #{KnapsackPro::Urls::KNAPSACK_PRO_CI_NODE_BUILD_ID}") end def ci_node_retry_count - ( - ENV['KNAPSACK_PRO_CI_NODE_RETRY_COUNT'] || - ci_env_for(:node_retry_count) || - 0 - ).to_i + (env_for('KNAPSACK_PRO_CI_NODE_RETRY_COUNT', :node_retry_count) || 0).to_i end def max_request_retries @@ -47,23 +38,19 @@ def max_request_retries end def commit_hash - ENV['KNAPSACK_PRO_COMMIT_HASH'] || - ci_env_for(:commit_hash) + env_for('KNAPSACK_PRO_COMMIT_HASH', :commit_hash) end def branch - ENV['KNAPSACK_PRO_BRANCH'] || - ci_env_for(:branch) + env_for('KNAPSACK_PRO_BRANCH', :branch) end def project_dir - ENV['KNAPSACK_PRO_PROJECT_DIR'] || - ci_env_for(:project_dir) + env_for('KNAPSACK_PRO_PROJECT_DIR', :project_dir) end def user_seat - ENV['KNAPSACK_PRO_USER_SEAT'] || - ci_env_for(:user_seat) + env_for('KNAPSACK_PRO_USER_SEAT', :user_seat) end def masked_user_seat @@ -183,7 +170,7 @@ def fixed_test_suite_split? def fixed_queue_split @fixed_queue_split ||= begin env_name = 'KNAPSACK_PRO_FIXED_QUEUE_SPLIT' - computed = ENV.fetch(env_name, ci_env_for(:fixed_queue_split)).to_s + computed = env_for(env_name, :fixed_queue_split).to_s if !ENV.key?(env_name) KnapsackPro.logger.info("#{env_name} is not set. Using default value: #{computed}. Learn more at #{KnapsackPro::Urls::FIXED_QUEUE_SPLIT}") @@ -249,10 +236,6 @@ def mode end end - def ci_env_for(env_name) - detected_ci.new.send(env_name) - end - def detected_ci detected = KnapsackPro::Config::CI.constants.map do |constant| Object.const_get("KnapsackPro::Config::CI::#{constant}").new.detected @@ -293,6 +276,21 @@ def ci? def required_env(env_name) ENV[env_name] || raise("Missing environment variable #{env_name}") end + + def env_for(knapsack_env_name, ci_env_method) + knapsack_env_value = ENV[knapsack_env_name] + ci_env_value = ci_env_for(ci_env_method) + + if !knapsack_env_value.nil? && !ci_env_value.nil? && knapsack_env_value != ci_env_value.to_s + KnapsackPro.logger.info("You have set the environment variable #{knapsack_env_name} to #{knapsack_env_value} which could be automatically determined from the CI environment as #{ci_env_value}.") + end + + knapsack_env_value != nil ? knapsack_env_value : ci_env_value + end + + def ci_env_for(env_name) + detected_ci.new.send(env_name) + end end end end diff --git a/spec/knapsack_pro/config/env_spec.rb b/spec/knapsack_pro/config/env_spec.rb index fc7028c7..8aabaa3e 100644 --- a/spec/knapsack_pro/config/env_spec.rb +++ b/spec/knapsack_pro/config/env_spec.rb @@ -17,6 +17,40 @@ it { should eq 4 } end + + context 'when both KNAPSACK_PRO_CI_NODE_TOTAL and CI environment have value' do + let(:logger) { instance_double(Logger, info: nil) } + + before do + stub_const("ENV", { 'KNAPSACK_PRO_CI_NODE_TOTAL' => env_value }) + expect(described_class).to receive(:ci_env_for).with(:node_total).and_return(ci_value) + allow(KnapsackPro).to receive(:logger).and_return(logger) + end + + context 'when values are different' do + let(:env_value) { '5' } + let(:ci_value) { 4 } + + it { should eq 5 } + + it 'logs a warning' do + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_CI_NODE_TOTAL to 5 which could be automatically determined from the CI environment as 4.' + ) + subject + end + end + + context 'when values are the same' do + let(:env_value) { '5' } + let(:ci_value) { 5 } + + it 'does not log a warning' do + expect(logger).not_to receive(:info) + subject + end + end + end end context "when ENV doesn't exist" do @@ -41,6 +75,40 @@ it { should eq 2 } end + context 'when both KNAPSACK_PRO_CI_NODE_INDEX and CI environment have value' do + let(:logger) { instance_double(Logger, info: nil) } + + before do + stub_const("ENV", { 'KNAPSACK_PRO_CI_NODE_INDEX' => env_value }) + expect(described_class).to receive(:ci_env_for).with(:node_index).and_return(ci_value) + allow(KnapsackPro).to receive(:logger).and_return(logger) + end + + context 'when values are different' do + let(:env_value) { '3' } + let(:ci_value) { 2 } + + it { should eq 3 } + + it 'logs a warning' do + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_CI_NODE_INDEX to 3 which could be automatically determined from the CI environment as 2.' + ) + subject + end + end + + context 'when values are the same' do + let(:env_value) { '3' } + let(:ci_value) { 3 } + + it 'does not log a warning' do + expect(logger).not_to receive(:info) + subject + end + end + end + context 'when order of loading envs does matter' do context 'when GitLab CI' do before { stub_const("ENV", { 'CI_NODE_INDEX' => '2', 'GITLAB_CI' => 'true' }) } @@ -70,6 +138,40 @@ it { should eq '8' } end + + context 'when both KNAPSACK_PRO_CI_NODE_BUILD_ID and CI environment have value' do + let(:logger) { instance_double(Logger, info: nil) } + + before do + stub_const("ENV", { 'KNAPSACK_PRO_CI_NODE_BUILD_ID' => env_value }) + expect(described_class).to receive(:ci_env_for).with(:node_build_id).and_return(ci_value) + allow(KnapsackPro).to receive(:logger).and_return(logger) + end + + context 'when values are different' do + let(:env_value) { '7' } + let(:ci_value) { '8' } + + it { should eq '7' } + + it 'logs a warning' do + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_CI_NODE_BUILD_ID to 7 which could be automatically determined from the CI environment as 8.' + ) + subject + end + end + + context 'when values are the same' do + let(:env_value) { '7' } + let(:ci_value) { '7' } + + it 'does not log a warning' do + expect(logger).not_to receive(:info) + subject + end + end + end end context "when ENV does not exist" do @@ -95,6 +197,40 @@ it { should eq 2 } end + + context 'when both KNAPSACK_PRO_CI_NODE_RETRY_COUNT and CI environment have value' do + let(:logger) { instance_double(Logger, info: nil) } + + before do + stub_const("ENV", { 'KNAPSACK_PRO_CI_NODE_RETRY_COUNT' => env_value }) + expect(described_class).to receive(:ci_env_for).with(:node_retry_count).and_return(ci_value) + allow(KnapsackPro).to receive(:logger).and_return(logger) + end + + context 'when values are different' do + let(:env_value) { '1' } + let(:ci_value) { 2 } + + it { should eq 1 } + + it 'logs a warning' do + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_CI_NODE_RETRY_COUNT to 1 which could be automatically determined from the CI environment as 2.' + ) + subject + end + end + + context 'when values are the same' do + let(:env_value) { '7' } + let(:ci_value) { '7' } + + it 'does not log a warning' do + expect(logger).not_to receive(:info) + subject + end + end + end end context "when ENV doesn't exist" do @@ -131,6 +267,40 @@ it { should eq 'fe61a08118d0d52e97c38666eba1eaf3' } end + + context 'when both KNAPSACK_PRO_COMMIT_HASH and CI environment have value' do + let(:logger) { instance_double(Logger, info: nil) } + + before do + stub_const("ENV", { 'KNAPSACK_PRO_COMMIT_HASH' => env_value }) + expect(described_class).to receive(:ci_env_for).with(:commit_hash).and_return(ci_value) + allow(KnapsackPro).to receive(:logger).and_return(logger) + end + + context 'when values are different' do + let(:env_value) { '3fa64859337f6e56409d49f865d13fd7' } + let(:ci_value) { 'fe61a08118d0d52e97c38666eba1eaf3' } + + it { should eq '3fa64859337f6e56409d49f865d13fd7' } + + it 'logs a warning' do + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_COMMIT_HASH to 3fa64859337f6e56409d49f865d13fd7 which could be automatically determined from the CI environment as fe61a08118d0d52e97c38666eba1eaf3.' + ) + subject + end + end + + context 'when values are the same' do + let(:env_value) { '3fa64859337f6e56409d49f865d13fd7' } + let(:ci_value) { '3fa64859337f6e56409d49f865d13fd7' } + + it 'does not log a warning' do + expect(logger).not_to receive(:info) + subject + end + end + end end context "when ENV doesn't exist" do @@ -154,6 +324,40 @@ it { should eq 'feature-branch' } end + + context 'when both KNAPSACK_PRO_BRANCH and CI environment have value' do + let(:logger) { instance_double(Logger, info: nil) } + + before do + stub_const("ENV", { 'KNAPSACK_PRO_BRANCH' => env_value }) + expect(described_class).to receive(:ci_env_for).with(:branch).and_return(ci_value) + allow(KnapsackPro).to receive(:logger).and_return(logger) + end + + context 'when values are different' do + let(:env_value) { 'master' } + let(:ci_value) { 'feature-branch' } + + it { should eq 'master' } + + it 'logs a warning' do + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_BRANCH to master which could be automatically determined from the CI environment as feature-branch.' + ) + subject + end + end + + context 'when values are the same' do + let(:env_value) { 'master' } + let(:ci_value) { 'master' } + + it 'does not log a warning' do + expect(logger).not_to receive(:info) + subject + end + end + end end context "when ENV doesn't exist" do @@ -177,6 +381,40 @@ it { should eq '/home/runner/myapp' } end + + context 'when both KNAPSACK_PRO_PROJECT_DIR and CI environment have value' do + let(:logger) { instance_double(Logger, info: nil) } + + before do + stub_const("ENV", { 'KNAPSACK_PRO_PROJECT_DIR' => env_value }) + expect(described_class).to receive(:ci_env_for).with(:project_dir).and_return(ci_value) + allow(KnapsackPro).to receive(:logger).and_return(logger) + end + + context 'when values are different' do + let(:env_value) { '/home/user/myapp' } + let(:ci_value) { '/home/runner/myapp' } + + it { should eq '/home/user/myapp' } + + it 'logs a warning' do + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_PROJECT_DIR to /home/user/myapp which could be automatically determined from the CI environment as /home/runner/myapp.' + ) + subject + end + end + + context 'when values are the same' do + let(:env_value) { '/home/user/myapp' } + let(:ci_value) { '/home/user/myapp' } + + it 'does not log a warning' do + expect(logger).not_to receive(:info) + subject + end + end + end end context "when ENV doesn't exist" do @@ -200,6 +438,40 @@ it { should eq 'Jane Doe' } end + + context 'when both KNAPSACK_PRO_USER_SEAT and CI environment have value' do + let(:logger) { instance_double(Logger, info: nil) } + + before do + stub_const("ENV", { 'KNAPSACK_PRO_USER_SEAT' => env_value }) + expect(described_class).to receive(:ci_env_for).with(:user_seat).and_return(ci_value) + allow(KnapsackPro).to receive(:logger).and_return(logger) + end + + context 'when values are different' do + let(:env_value) { 'John Doe' } + let(:ci_value) { 'Jane Doe' } + + it { should eq 'John Doe' } + + it 'logs a warning' do + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_USER_SEAT to John Doe which could be automatically determined from the CI environment as Jane Doe.' + ) + subject + end + end + + context 'when values are the same' do + let(:env_value) { 'John Doe' } + let(:ci_value) { 'John Doe' } + + it 'does not log a warning' do + expect(logger).not_to receive(:info) + subject + end + end + end end context "when ENV doesn't exist" do @@ -345,7 +617,6 @@ end end - describe '.regular_mode?' do subject { described_class.regular_mode? } @@ -630,26 +901,32 @@ context 'when ENV exists' do context 'when KNAPSACK_PRO_FIXED_QUEUE_SPLIT=false' do [ - ['AppVeyor', { 'APPVEYOR' => '123' }], - ['Buildkite', { 'BUILDKITE' => 'true' }], - ['CircleCI', { 'CIRCLECI' => 'true' }], - ['Cirrus CI', { 'CIRRUS_CI' => 'true' }], - ['Codefresh', { 'CF_BUILD_ID' => '123' }], - ['Codeship', { 'CI_NAME' => 'codeship' }], - ['GitHub Actions', { 'GITHUB_ACTIONS' => 'true' }], - ['GitLab CI', { 'GITLAB_CI' => 'true' }], - ['Heroku CI', { 'HEROKU_TEST_RUN_ID' => '123' }], - ['Semaphore CI 1.0', { 'SEMAPHORE_BUILD_NUMBER' => '123' }], - ['Semaphore CI 2.0', { 'SEMAPHORE' => 'true', 'SEMAPHORE_WORKFLOW_ID' => '123' }], - ['Travis CI', { 'TRAVIS' => 'true' }], - ['Unsupported CI', {}], - ].each do |ci, env| + ['AppVeyor', { 'APPVEYOR' => '123' }, false], + ['Buildkite', { 'BUILDKITE' => 'true' }, true], + ['CircleCI', { 'CIRCLECI' => 'true' }, false], + ['Cirrus CI', { 'CIRRUS_CI' => 'true' }, false], + ['Codefresh', { 'CF_BUILD_ID' => '123' }, false], + ['Codeship', { 'CI_NAME' => 'codeship' }, true], + ['GitHub Actions', { 'GITHUB_ACTIONS' => 'true' }, true], + ['GitLab CI', { 'GITLAB_CI' => 'true' }, true], + ['Heroku CI', { 'HEROKU_TEST_RUN_ID' => '123' }, false], + ['Semaphore CI 1.0', { 'SEMAPHORE_BUILD_NUMBER' => '123' }, false], + ['Semaphore CI 2.0', { 'SEMAPHORE' => 'true', 'SEMAPHORE_WORKFLOW_ID' => '123' }, false], + ['Travis CI', { 'TRAVIS' => 'true' }, true], + ['Unsupported CI', {}, true], + ].each do |ci, env, ci_env| it "on #{ci} it is false" do + stub_const("ENV", env.merge({ 'KNAPSACK_PRO_FIXED_QUEUE_SPLIT' => 'false' })) + logger = instance_double(Logger) allow(KnapsackPro).to receive(:logger).and_return(logger) - expect(logger).not_to receive(:info) - - stub_const("ENV", env.merge({ 'KNAPSACK_PRO_FIXED_QUEUE_SPLIT' => 'false' })) + if ci_env == false + expect(logger).not_to receive(:info) + else + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_FIXED_QUEUE_SPLIT to false which could be automatically determined from the CI environment as true.' + ) + end expect(subject).to eq(false) end @@ -658,26 +935,32 @@ context 'when KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true' do [ - ['AppVeyor', { 'APPVEYOR' => '123' }], - ['Buildkite', { 'BUILDKITE' => 'true' }], - ['CircleCI', { 'CIRCLECI' => 'true' }], - ['Cirrus CI', { 'CIRRUS_CI' => 'true' }], - ['Codefresh', { 'CF_BUILD_ID' => '123' }], - ['Codeship', { 'CI_NAME' => 'codeship' }], - ['GitHub Actions', { 'GITHUB_ACTIONS' => 'true' }], - ['GitLab CI', { 'GITLAB_CI' => 'true' }], - ['Heroku CI', { 'HEROKU_TEST_RUN_ID' => '123' }], - ['Semaphore CI 1.0', { 'SEMAPHORE_BUILD_NUMBER' => '123' }], - ['Semaphore CI 2.0', { 'SEMAPHORE' => 'true', 'SEMAPHORE_WORKFLOW_ID' => '123' }], - ['Travis CI', { 'TRAVIS' => 'true' }], - ['Unsupported CI', {}], - ].each do |ci, env| + ['AppVeyor', { 'APPVEYOR' => '123' }, false], + ['Buildkite', { 'BUILDKITE' => 'true' }, true], + ['CircleCI', { 'CIRCLECI' => 'true' }, false], + ['Cirrus CI', { 'CIRRUS_CI' => 'true' }, false], + ['Codefresh', { 'CF_BUILD_ID' => '123' }, false], + ['Codeship', { 'CI_NAME' => 'codeship' }, true], + ['GitHub Actions', { 'GITHUB_ACTIONS' => 'true' }, true], + ['GitLab CI', { 'GITLAB_CI' => 'true' }, true], + ['Heroku CI', { 'HEROKU_TEST_RUN_ID' => '123' }, false], + ['Semaphore CI 1.0', { 'SEMAPHORE_BUILD_NUMBER' => '123' }, false], + ['Semaphore CI 2.0', { 'SEMAPHORE' => 'true', 'SEMAPHORE_WORKFLOW_ID' => '123' }, false], + ['Travis CI', { 'TRAVIS' => 'true' }, true], + ['Unsupported CI', {}, true], + ].each do |ci, env, ci_env| it "on #{ci} it is true" do + stub_const("ENV", env.merge({ 'KNAPSACK_PRO_FIXED_QUEUE_SPLIT' => 'true' })) + logger = instance_double(Logger) allow(KnapsackPro).to receive(:logger).and_return(logger) - expect(logger).not_to receive(:info) - - stub_const("ENV", env.merge({ 'KNAPSACK_PRO_FIXED_QUEUE_SPLIT' => 'true' })) + if ci_env == true + expect(logger).not_to receive(:info) + else + expect(logger).to receive(:info).with( + 'You have set the environment variable KNAPSACK_PRO_FIXED_QUEUE_SPLIT to true which could be automatically determined from the CI environment as false.' + ) + end expect(subject).to eq(true) end @@ -702,12 +985,12 @@ ['Unsupported CI', {}, true], ].each do |ci, env, expected| it "on #{ci} it is #{expected}" do + stub_const("ENV", env) + logger = instance_double(Logger) expect(KnapsackPro).to receive(:logger).and_return(logger) expect(logger).to receive(:info).with("KNAPSACK_PRO_FIXED_QUEUE_SPLIT is not set. Using default value: #{expected}. Learn more at #{KnapsackPro::Urls::FIXED_QUEUE_SPLIT}") - stub_const("ENV", env) - expect(subject).to eq(expected) end end