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

refactor: use generator methods, and class instance variables to separate duties between classes #143

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

refactor: use generator methods, and class instance variables to separate duties between classes #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
200b0ad
refactor: use generator methods, and class instance variables to sepa…
jim80net Aug 31, 2022
9c1a77a
supply default DD_API_KEY for testing
jim80net Oct 26, 2022
7d4d7a6
set format to json for consistency
jim80net Oct 26, 2022
93b8624
refactor: cleanup deprecation warnings
jim80net Oct 26, 2022
77f3d47
avoid "warning: instance variable @body not initialized" error
jim80net Oct 26, 2022
de7d014
rubocop -A
jim80net Oct 26, 2022
9df4d97
fix: dashboards needs to do a get for each element in the list
jim80net Oct 26, 2022
27393e9
fix: include id for monitors
jim80net Oct 26, 2022
8169497
fix: restore multithreading for Dashboards.all
jim80net Oct 26, 2022
a003595
fix: restore multithreading for Dashboards.all
jim80net Oct 26, 2022
157dd43
update CLI to use new API
jim80net Nov 7, 2022
91d50d6
refactor: move faraday to client.rb
jim80net Nov 7, 2022
9baaf3e
chore: add FactoryBot to specs
jim80net Nov 7, 2022
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
7 changes: 7 additions & 0 deletions .rubocop.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ AllCops:
TargetRubyVersion: 2.7
NewCops: enable


Layout/LineLength:
Enabled: false

Expand All @@ -34,4 +35,10 @@ RSpec/MultipleMemoizedHelpers:
Enabled: false

RSpec/ExampleLength:
Enabled: false

Style/ClassVars:
Enabled: false

Style/GlobalVars:
Copy link

Choose a reason for hiding this comment

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

The proscription against class and global variables is in general pretty sensible. I haven't yet digested enough of this PR to know whether they can be done without, but if not, I think the Rubocop checks should be disabled on a linewise basis rather than disabling them globally. For example, one a line with a global variable like $options, you can append a comment:

# rubocop:disable Style/GlobalVars

...to turn off the warning for that line only. It's a bit noisy, but I think it's better to draw attention to places where the usual style rules are being overridden by necessity.

...Also, when I check out your branch and search the codebase for the string @@, I don't find any instances. I would expect to find some if Ruby class variables are being employed somewhere. Is it actually necessary to disable the Style/ClassVars rule at all?

Enabled: false
1 change: 1 addition & 0 deletions datadog_backup.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Gem::Specification.new do |spec|
spec.add_dependency 'faraday-retry'

spec.add_development_dependency 'bundler'
spec.add_development_dependency 'factory_bot'
spec.add_development_dependency 'guard-rspec'
spec.add_development_dependency 'pry'
spec.add_development_dependency 'pry-byebug'
Expand Down
12 changes: 9 additions & 3 deletions lib/datadog_backup.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,22 @@

require 'concurrent'

require_relative 'datadog_backup/local_filesystem'
require_relative 'datadog_backup/options'
require_relative 'datadog_backup/cli'
require_relative 'datadog_backup/client'
require_relative 'datadog_backup/thread_pool'

require_relative 'datadog_backup/resources/local_filesystem/class_methods'
require_relative 'datadog_backup/resources/local_filesystem'
require_relative 'datadog_backup/resources/class_methods'
require_relative 'datadog_backup/resources'

require_relative 'datadog_backup/dashboards'
require_relative 'datadog_backup/monitors'
require_relative 'datadog_backup/synthetics'
require_relative 'datadog_backup/thread_pool'

require_relative 'datadog_backup/version'
require_relative 'datadog_backup/deprecations'

DatadogBackup::Deprecations.check

# DatadogBackup is a gem for backing up and restoring Datadog monitors and dashboards.
Expand Down
125 changes: 46 additions & 79 deletions lib/datadog_backup/cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,109 +6,80 @@
module DatadogBackup
# CLI is the command line interface for the datadog_backup gem.
class Cli
include ::DatadogBackup::Options

def all_diff_futures
LOGGER.info("Starting diffs on #{::DatadogBackup::ThreadPool::TPOOL.max_length} threads")
any_resource_instance
.all_file_ids_for_selected_resources
.map do |file_id|
Concurrent::Promises.future_on(::DatadogBackup::ThreadPool::TPOOL, file_id) do |fid|
[fid, getdiff(fid)]
end
end
end

def any_resource_instance
resource_instances.first
def initialize(options)
$options = options
Copy link

@grimnebulin grimnebulin Nov 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The original code stored the options in an instance variable rather than a global variable, which would be the more normal practice. This PR is a little large for me to take in all at once, so I'll just ask here: What other code needs to access the options passed in here via the global variable $options?

...Coming back to this comment a bit later, I have some ideas for how to minimize the drawbacks of the use of a global variable. (That is, if its use can't be easily eliminated.)

First, it would be cleaner if the code that instantiates the DatadogBackup::Cli object assigns the options to the global variable rather than leaving it up to the class to do it. I see the code that does the instantiation in bin/datadog_backup:

DatadogBackup::Cli.new(prereqs(defaults)).run!

I'd suggest instead something like:

$options = prereqs(defaults).freeze
DatadogBackup::Cli.new($options).run!

I see also that the Cli class only cares about the resources, force_restore, and action option values. The class could be constructed in a more standard style be storing these as instance variables in the class.

def initialize(resources:, force_restore:, action:, **)
  @resources = resources
  @force_restore = force_restore
  @action = action
end

(The ** will cause any unknown keys in the hash to be ignored.)

Then in the rest of the class you can write just @resources instead of $options[:resources], and likewise for the other keys.

The above only applies if no other code is going to update the options on the fly, which appears to be the case.

end

def backup
resource_instances.each(&:purge)
resource_instances.each(&:backup)
$options[:resources].each(&:purge)
$options[:resources].each(&:backup_all)
any_resource_instance.all_files
end

def definitive_resource_instance(id)
matching_resource_instance(any_resource_instance.class_from_id(id))
end
def diffs
$options[:resources].each do |resource|
resource.all.each do |resource_instance|
next if resource_instance.diff.nil? || resource_instance.diff.empty?
Copy link

Choose a reason for hiding this comment

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

It doesn't seem like ActiveSupport is available in this repo...? If it is, you can combine these two tests into just:

next if resource_instance.diff.blank?


def getdiff(id)
result = definitive_resource_instance(id).diff(id)
case result
when '---' || '' || "\n" || '<div class="diff"></div>'
nil
else
result
end
end

# rubocop:disable Style/StringConcatenation
def format_diff_output(diff_output)
case diff_format
when nil, :color
diff_output.map do |id, diff|
" ---\n id: #{id}\n#{diff}"
end.join("\n")
when :html
'<html><head><style>' +
Diffy::CSS +
'</style></head><body>' +
diff_output.map do |id, diff|
"<br><br> ---<br><strong> id: #{id}</strong><br>#{diff}"
end.join('<br>') +
'</body></html>'
else
raise 'Unexpected diff_format.'
puts resource_instance.diff
end
end
end
# rubocop:enable Style/StringConcatenation

def initialize(options)
@options = options
end

def restore
futures = all_diff_futures
watcher = ::DatadogBackup::ThreadPool.watcher

futures.each do |future|
id, diff = *future.value!
next if diff.nil? || diff.empty?

if @options[:force_restore]
definitive_resource_instance(id).restore(id)
else
ask_to_restore(id, diff)
$options[:resources].each do |resource|
resource.all.each do |resource_instance|
next if resource_instance.diff.nil? || resource_instance.diff.empty?

if $options[:force_restore]
resource_instance.restore
else
ask_to_restore(resource_instance)
end
end
end
watcher.join if watcher.status
end

def run!
puts(send(action.to_sym))
case $options[:action]
when 'backup'
LOGGER.info('Starting backup.')
backup
when 'restore'
LOGGER.info('Starting restore.')
restore
when 'diffs'
LOGGER.info('Starting diffs.')
diffs
else
fatal 'No action specified.'
end
LOGGER.info('Done.')
rescue SystemExit, Interrupt
::DatadogBackup::ThreadPool.shutdown
end

private

def ask_to_restore(id, diff)
##
# Interact with the user

def ask_to_restore(resource_instance)
puts '--------------------------------------------------------------------------------'
puts format_diff_output([id, diff])
puts resource_instance.diff
puts '(r)estore to Datadog, overwrite local changes and (d)ownload, (s)kip, or (q)uit?'
loop do
response = $stdin.gets.chomp
case response
when 'q'
exit
when 'r'
puts "Restoring #{id} to Datadog."
definitive_resource_instance(id).restore(id)
puts "Restoring #{resource_instance.id} to Datadog."
resource_instance.restore
break
when 'd'
puts "Downloading #{id} from Datadog."
definitive_resource_instance(id).get_and_write_file(id)
puts "Downloading #{resource_instance.id} from Datadog."
resource_instance.backup
break
when 's'
break
Expand All @@ -118,14 +89,10 @@ def ask_to_restore(id, diff)
end
end

def matching_resource_instance(klass)
resource_instances.select { |resource_instance| resource_instance.instance_of?(klass) }.first
end

def resource_instances
@resource_instances ||= resources.map do |resource|
resource.new(@options)
end
##
# Finding the right resource instance to use.
def any_resource_instance
$options[:resources].first
end
end
end
49 changes: 49 additions & 0 deletions lib/datadog_backup/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# frozen_string_literal: true
require 'faraday'
require 'faraday/retry'

module DatadogBackup
class Client
RETRY_OPTIONS = {
max: 5,
interval: 0.05,
interval_randomness: 0.5,
backoff_factor: 2
}.freeze

def initialize
@client = Faraday.new(
url: ENV.fetch('DD_SITE_URL', 'https://api.datadoghq.com/'),
Copy link

Choose a reason for hiding this comment

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

The default url might be better stored as a class constant, eg:

DEFAULT_DD_SITE_URL = 'https://api.datadoghq.com/'

And later:

url: ENV.fetch('DD_SITE_URL', DEFAULT_DD_SITE_URL)

headers: {
'DD-API-KEY' => ENV.fetch('DD_API_KEY'),
'DD-APPLICATION-KEY' => ENV.fetch('DD_APP_KEY')
}
) do |faraday|
faraday.request :json
faraday.request :retry, RETRY_OPTIONS
faraday.response(:logger, LOGGER, { headers: true, bodies: LOGGER.debug?, log_level: :debug }) do |logger|
logger.filter(/(DD-API-KEY:)([^&]+)/, '\1[REDACTED]')
logger.filter(/(DD-APPLICATION-KEY:)([^&]+)/, '\1[REDACTED]')
end
faraday.response :raise_error
faraday.response :json
faraday.adapter Faraday.default_adapter
end
end

def get_body(path, params = {}, headers = {})
response = @client.get(path, params, headers)
response.body
end

def post_body(path, body, headers = {})
response = @client.post(path, body, headers)
response.body
end

def put_body(path, body, headers = {})
response = @client.put(path, body, headers)
response.body
end
end
end
48 changes: 16 additions & 32 deletions lib/datadog_backup/dashboards.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,42 +3,26 @@
module DatadogBackup
# Dashboards specific overrides for backup and restore.
class Dashboards < Resources
def all
get_all.fetch('dashboards')
end

def backup
LOGGER.info("Starting diffs on #{::DatadogBackup::ThreadPool::TPOOL.max_length} threads")
futures = all.map do |dashboard|
Concurrent::Promises.future_on(::DatadogBackup::ThreadPool::TPOOL, dashboard) do |board|
id = board[id_keyname]
get_and_write_file(id)
@api_version = 'v1'
@api_resource_name = 'dashboard'
@id_keyname = 'id'
@banlist = %w[modified_at url].freeze
@api_service = DatadogBackup::Client.new
@dig_in_list_body = 'dashboards'

def self.all
return @all if @all

futures = get_all.map do |resource|
Concurrent::Promises.future_on(DatadogBackup::ThreadPool::TPOOL, resource) do |r|
new_resource(id: r.fetch(@id_keyname))
end
end
LOGGER.info "Found #{futures.length} #{@api_resource_name}s in Datadog"

watcher = ::DatadogBackup::ThreadPool.watcher
watcher = DatadogBackup::ThreadPool.watcher
watcher.join if watcher.status

Concurrent::Promises.zip(*futures).value!
end

def initialize(options)
super(options)
@banlist = %w[modified_at url].freeze
end

private

def api_version
'v1'
end

def api_resource_name
'dashboard'
end

def id_keyname
'id'
@all = Concurrent::Promises.zip(*futures).value!
end
end
end
Loading