Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract endpoints into config file #27

Closed
wants to merge 8 commits into from
Closed

Extract endpoints into config file #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f011596
Extract endpoints into config file
willzoltan Feb 2, 2024
4ea2077
Change config names to ping_url and monitor_url
willzoltan Feb 4, 2024
7c0e1e2
Improve spec for new config settings
willzoltan Feb 4, 2024
16d225e
Fix ENV variable names
willzoltan Feb 4, 2024
03e84e0
Add specs for custom ENV vars
willzoltan Feb 4, 2024
b59bd46
Fix Cronitor leaking global values in specs
willzoltan Feb 4, 2024
38b3092
Define default_ping_url and default_monitor_url
willzoltan Feb 7, 2024
2fa6076
Fix monitor URL
willzoltan Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/cronitor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def self.job(key, &block)
end

def self.monitor_api_url
'https://cronitor.io/api/monitors'
"#{Cronitor.monitor_url}/api/monitors"
end
end

Expand Down
12 changes: 11 additions & 1 deletion lib/cronitor/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,19 @@ module Cronitor
YAML_KEYS = MONITOR_TYPES.map { |t| "#{t}s" }

class << self
attr_accessor :api_key, :api_version, :environment, :logger, :config, :timeout, :ping_timeout, :auto_discover_sidekiq
attr_accessor :api_key, :api_version, :environment, :logger, :config, :timeout, :ping_timeout, :auto_discover_sidekiq, :ping_url, :monitor_url

def configure(&block)
block.call(self)
end

def default_ping_url
ENV.fetch('CRONITOR_PING_URL', 'https://cronitor.link')
end

def default_monitor_url
ENV.fetch('CRONITOR_MONITOR_URL', 'https://cronitor.io')
end
end

self.api_key = ENV.fetch('CRONITOR_API_KEY', nil)
Expand All @@ -23,6 +31,8 @@ def configure(&block)
self.ping_timeout = ENV.fetch('CRONITOR_PING_TIMEOUT', nil) || 5
self.config = ENV.fetch('CRONITOR_CONFIG', nil)
self.auto_discover_sidekiq = ENV.fetch('CRONITOR_AUTO_DISCOVER_SIDEKIQ', 'true').casecmp('true').zero? # https://github.com/cronitorio/cronitor-sidekiq
self.ping_url = default_ping_url
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kinda nit-picky on my part, so take it with a grain of salt, but since it's reading from the ENV var, these aren't really "defaults". Could we keep setting these the same as before, and then in the test set up where these methods are used just set the defaults correctly there?

Or, if you want to keep these methods, just have them return the string only instead of calling ENV.fetch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't originally going to add these methods, but basically we need a method to reset the ping_url and the monitor_url from the test suite. This is because Crontior is like a god object, so when the first test sets the values of ping_url and the monitor_url, the second test retains the previous values. There is no way to flush or reload the Cronitor class.

But we need to load a fresh Cronitor class twice for the test suite. Once for when there are no ENV vars. And then again when there are ENV vars.

So these default_* methods allow me to call self.ping_url = default_ping_url before each test, just like they are called when the Cronitor class loads, so that when we change the ENV vars for the second test, the tests then behave properly and actually read the ENV vars instead of retaining the values from the first test.

We could copy the code which sets the initial ping_url and monitor_url values into the test suite and have something like below in the spec instead.

In my view this is worse, because it is not DRY and if the code in lib/cronitor/config.rb changes, then we are not testing the Cronitor class, but testing the code that is in the before block.

Would changing the name of the methods from default_* to initial_* be better?

describe '#ping_url and #monitor_url' do
  let(:env_vars) { ENV }

  before do
    stub_const('ENV', env_vars)
    # We need to reset the ping_url and monitor_url since the previous spec sets them globally
    Cronitor.configure do |cronitor|
      cronitor.ping_url = ENV.fetch('CRONITOR_PING_URL', 'https://cronitor.link') # copied from lib/cronitor/config.rb
      cronitor.monitor_url = ENV.fetch('CRONITOR_MONITOR_URL', 'https://cronitor.io') # copied from lib/cronitor/config.rb
    end
  end

  context 'when custom CRONITOR_PING_URL and CRONITOR_MONITOR_URL ENV variables are set' do
    let(:env_vars) do
      ENV.to_hash.merge(
        'CRONITOR_PING_URL' => 'https://ping.com',
        'CRONITOR_MONITOR_URL' => 'https://monitor.com'
      )
    end

    it 'returns the custom Cronitor URLs' do
      expect(Cronitor.ping_url).to eq('https://ping.com')
      expect(Cronitor.monitor_url).to eq('https://monitor.com')
    end
  end

  context 'when no CRONITOR_PING_URL or CRONITOR_MONITOR_URL ENV variables are set' do
    it 'returns the default Cronitor URLs' do
      expect(Cronitor.ping_url).to eq('https://cronitor.link')
      expect(Cronitor.monitor_url).to eq('https://cronitor.io')
    end
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aflanagan

self.monitor_url = default_monitor_url
self.logger = Logger.new($stdout)
logger.level = Logger::INFO
end
2 changes: 1 addition & 1 deletion lib/cronitor/monitor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ def unpause
end

def ping_api_url
"https://cronitor.link/p/#{api_key}/#{key}"
"#{Cronitor.ping_url}/p/#{api_key}/#{key}"
end

def fallback_ping_api_url
Expand Down
34 changes: 34 additions & 0 deletions spec/cronitor_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,40 @@
end
end

describe '#ping_url and #monitor_url' do
let(:env_vars) { ENV }

before do
stub_const('ENV', env_vars)
# We need to reset the ping_url and monitor_url since the previous spec sets them globally
Cronitor.configure do |cronitor|
cronitor.ping_url = cronitor.default_ping_url
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willzoltan thank you for these! I think the specs need to be updated to not reference default_XXX since those aren't defined on the module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I forgot to commit all my changes... Fixed!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed again. Sorry I don't have the test suite set up on my computer

cronitor.monitor_url = cronitor.default_monitor_url
end
end

context 'when custom CRONITOR_PING_URL and CRONITOR_MONITOR_URL ENV variables are set' do
let(:env_vars) do
ENV.to_hash.merge(
'CRONITOR_PING_URL' => 'https://ping.com',
'CRONITOR_MONITOR_URL' => 'https://monitor.com'
)
end

it 'returns the custom Cronitor URLs' do
expect(Cronitor.ping_url).to eq('https://ping.com')
expect(Cronitor.monitor_url).to eq('https://monitor.com')
end
end

context 'when no CRONITOR_PING_URL or CRONITOR_MONITOR_URL ENV variables are set' do
it 'returns the default Cronitor URLs' do
expect(Cronitor.ping_url).to eq('https://cronitor.link')
expect(Cronitor.monitor_url).to eq('https://cronitor.io')
end
end
end

describe 'YAML configuration' do
before(:all) do
Cronitor.configure do |cronitor|
Expand Down
Loading