refactor: use generator methods, and class instance variables to separate duties between classes by jim80net · Pull Request #143 · scribd/datadog_backup

jim80net · 2022-10-26T18:06:54Z

Uses the factory method to manage the differences and commonalities in the DataDog API.

The major features of this refactor are:

instance methods are responsible for manipulating singular resources
class methods are used for manipulating multiples of resources.

TODO:

client_spec.rb
cli_spec.rb
consider collapsing local_filesystem into resources.rb
consider collapsing class_methods.rb into parent files.

…rate duties between classes

grimnebulin · 2022-11-10T00:17:43Z

.rubocop.yml

+Style/ClassVars:
+  Enabled: false
+
+Style/GlobalVars:


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?

grimnebulin · 2022-11-10T00:27:41Z

lib/datadog_backup/cli.rb

-    def any_resource_instance
-      resource_instances.first
+    def initialize(options)
+      $options = options


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.

grimnebulin · 2022-11-10T00:29:47Z

lib/datadog_backup/cli.rb

+    def diffs
+      $options[:resources].each do |resource|
+        resource.all.each do |resource_instance|
+          next if resource_instance.diff.nil? || resource_instance.diff.empty?


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?

grimnebulin · 2022-11-10T01:16:47Z

lib/datadog_backup/resources.rb

+        outhash.delete(key) # delete returns the value at the deleted key, hence the tap wrapper
+      end
+      outhash
    end


This method could be more concisely implemented as:

@banlist.each_with_object(hash.dup) do |key, outhash| outhash.delete(key) end

grimnebulin · 2022-11-10T01:19:49Z

lib/datadog_backup/resources/local_filesystem.rb

+        file.write(data)
+      ensure
+        file.close
+      end


The File.open method can be given a block, and will take care of closing the file afterwards:

::File.open(filename, 'w') do |file| file.write(data) end

grimnebulin · 2022-11-11T21:26:06Z

lib/datadog_backup/client.rb

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


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)

grimnebulin · 2022-11-11T21:31:47Z

lib/datadog_backup/resources/class_methods.rb

+        @all ||= get_all.map do |resource|
+          new_resource(id: resource.fetch(@id_keyname), body: resource)
+        end
+        LOGGER.info "Found #{@all.length} #{@api_resource_name}s in Datadog"


This logging will happen even if the get_all response has already been cached in the @all instance variable. Is that intended?

grimnebulin · 2022-11-11T21:32:16Z

lib/datadog_backup/resources/class_methods.rb

+        @get_all = @dig_in_list_body ? body.fetch(*@dig_in_list_body) : body
+      end
+
+      # Fetch the specified resource from Datadog and remove the @banlist elements


Is this comment accurate? The method is very short and doesn't seem to affect @banlist at all.

grimnebulin · 2022-11-11T21:33:56Z

lib/datadog_backup/resources/class_methods.rb

+      end
+
+      def backup_all
+        all.map(&:backup)


I see only one place backup_all is called, and the result is discarded. If the result of calling backup is not needed, you can signal that more explicitly by saying all.each(&:backup) here.

grimnebulin · 2022-11-11T21:35:25Z

lib/datadog_backup/resources/local_filesystem.rb

+  class Resources
+    ##
+    # Meant to be mixed into DatadogBackup::Resources
+    # Relies on $options[:backup_dir] and $options[:output_format]


I don't see $options[:backup_dir] mentioned in this file; is this comment accurate?

grimnebulin · 2022-11-11T21:40:18Z

spec/datadog_backup/cli_spec.rb

@@ -3,142 +3,4 @@
 require 'spec_helper'

 describe DatadogBackup::Cli do


This file is now nearly empty and does no testing; can it be removed?

grimnebulin · 2022-11-11T21:40:43Z

spec/datadog_backup/client_spec.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+describe DatadogBackup::Client do


Is this file necessary? It doesn't seem to do anything.

grimnebulin · 2022-11-11T21:43:30Z

spec/factories.rb

+    @strategy = FactoryBot.strategy_by_name(:create).new
+  end
+
+  #delegate :association, to: :@strategy


Better to just delete this line probably.

scribdbot · 2026-02-05T17:18:51Z

⚠️ PR Age Notice: This pull request was opened over 30 days ago and has been labeled for visibility. If there's no activity for 7 days after this label is applied, it will be automatically closed.

To keep it open, please leave a comment or push an update. You can also label it as 'pinned' to prevent auto-closure.

jim80net added 9 commits October 26, 2022 11:06

refactor: use generator methods, and class instance variables to sepa…

200b0ad

…rate duties between classes

supply default DD_API_KEY for testing

9c1a77a

set format to json for consistency

7d4d7a6

refactor: cleanup deprecation warnings

93b8624

avoid "warning: instance variable @Body not initialized" error

77f3d47

rubocop -A

de7d014

fix: dashboards needs to do a get for each element in the list

9df4d97

fix: include id for monitors

27393e9

fix: restore multithreading for Dashboards.all

8169497

jim80net marked this pull request as draft October 27, 2022 01:49

jim80net added 4 commits November 6, 2022 16:35

fix: restore multithreading for Dashboards.all

a003595

update CLI to use new API

157dd43

refactor: move faraday to client.rb

91d50d6

chore: add FactoryBot to specs

9baaf3e

jim80net marked this pull request as ready for review November 7, 2022 22:47

jim80net marked this pull request as draft November 7, 2022 22:52

grimnebulin reviewed Nov 10, 2022

View reviewed changes

grimnebulin reviewed Nov 11, 2022

View reviewed changes

jim80net mentioned this pull request Feb 10, 2023

Lack of integration testing hiding problems in the workflow. #149

Open

scribdbot added the 30+ days open PR has been open for 30+ days label Feb 5, 2026

		@@ -3,142 +3,4 @@
		require 'spec_helper'

		describe DatadogBackup::Cli do

		@@ -0,0 +1,4 @@
		# frozen_string_literal: true

		describe DatadogBackup::Client do

Conversation

jim80net commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grimnebulin Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scribdbot commented Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

jim80net commented Oct 26, 2022 •

edited

Loading

grimnebulin Nov 10, 2022 •

edited

Loading