refactor: allow AwsSdkAdapter to receive any Aws::S3::Client options (#…

…351) * refactor: allow AwsSdkAdapter to receive any Aws::S3::Client options * Only set credentials when necessary * Make the deprecated options optional * Update docs * Support legacy aws_endpoint option. Don't set credentials * Remove duplicate docs * Only set region and endpoint if not already set Co-authored-by: Karl Varga <kjvarga@gmail.com>
kjvarga · Jan 17, 2022 · 4e7f817 · 4e7f817
1 parent fce9c0c
commit 4e7f817
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 84 deletions.
diff --git a/README.md b/README.md
@@ -87,7 +87,6 @@ Successful ping of Bing
         - [`SitemapGenerator::FogAdapter`](#sitemapgeneratorfogadapter)
         - [`SitemapGenerator::S3Adapter`](#sitemapgenerators3adapter)
         - [`SitemapGenerator::AwsSdkAdapter`](#sitemapgeneratorawssdkadapter)
-        - [`SitemapGenerator::AwsSdkAdapter (DigitalOcean Spaces)`](#sitemapgeneratorawssdkadapter-digitalocean-spaces)
         - [`SitemapGenerator::WaveAdapter`](#sitemapgeneratorwaveadapter)
         - [`SitemapGenerator::GoogleStorageAdapter`](#sitemapgeneratorgooglestorageadapter)
       - [An Example of Using an Adapter](#an-example-of-using-an-adapter)
@@ -379,7 +378,7 @@ name but capitalized, e.g. `FOG_PATH_STYLE`.
 ##### `SitemapGenerator::AwsSdkAdapter`
 
   Uses `Aws::S3::Resource` to upload to Amazon S3 storage.  Includes automatic detection of your AWS
-  credentials using `Aws::Credentials`.
+  credentials and region.
 
   You must `require 'aws-sdk-s3'` in your sitemap config before using this adapter,
   or `require` another library that defines `Aws::S3::Resource` and `Aws::Credentials`.
@@ -388,30 +387,18 @@ name but capitalized, e.g. `FOG_PATH_STYLE`.
 
   ```ruby
   SitemapGenerator::Sitemap.adapter = SitemapGenerator::AwsSdkAdapter.new('s3_bucket',
-    aws_access_key_id: 'AKIAI3SW5CRAZBL4WSTA',
-    aws_secret_access_key: 'asdfadsfdsafsadf',
-    aws_region: 'us-east-1'
+    access_key_id: 'AKIAI3SW5CRAZBL4WSTA',
+    secret_access_key: 'asdfadsfdsafsadf',
+    region: 'us-east-1',
+    endpoint: 'https://sfo2.digitaloceanspaces.com'
   )
   ```
 
-##### `SitemapGenerator::AwsSdkAdapter (DigitalOcean Spaces)`
+  Where the first argument is the S3 bucket name, and the rest are keyword argument options which
+  are passed directly to the AWS client.
 
-  Uses `Aws::S3::Resource` to upload to Amazon S3 storage.  Includes automatic detection of your AWS
-  credentials using `Aws::Credentials`.
-
-  You must `require 'aws-sdk-s3'` in your sitemap config before using this adapter,
-  or `require` another library that defines `Aws::S3::Resource` and `Aws::Credentials`.
-
-  An example of using this adapter in your sitemap configuration:
-
-  ```ruby
-  SitemapGenerator::Sitemap.adapter = SitemapGenerator::AwsSdkAdapter.new('s3_bucket',
-    aws_access_key_id: 'AKIAI3SW5CRAZBL4WSTA',
-    aws_secret_access_key: 'asdfadsfdsafsadf',
-    aws_region: 'sfo2',
-    aws_endpoint: 'https://sfo2.digitaloceanspaces.com'
-  )
-  ```
+  See https://docs.aws.amazon.com/sdk-for-ruby/v2/api/Aws/S3/Client.html#initialize-instance_method
+  for a full list of supported options.
 
 ##### `SitemapGenerator::WaveAdapter`
 

diff --git a/lib/sitemap_generator/adapters/aws_sdk_adapter.rb b/lib/sitemap_generator/adapters/aws_sdk_adapter.rb
@@ -8,23 +8,30 @@ module SitemapGenerator
   class AwsSdkAdapter
     # Specify your AWS bucket name, credentials, and/or region.  By default
     # the AWS SDK will auto-detect your credentials and region, but you can use
-    # the following options to configure - or override - them manually:
-    #
-    # Options:
-    #   :aws_access_key_id [String] Your AWS access key id
-    #   :aws_secret_access_key [String] Your AWS secret access key
-    #   :aws_region [String] Your AWS region
+    # the options to configure them manually.
     #
     # Requires Aws::S3::Resource and Aws::Credentials to be defined.
     #
-    # @param [String] bucket Name of the S3 bucket
-    # @param [Hash] options AWS credential overrides, see above
-    def initialize(bucket, options = {})
+    # @param bucket [String] Name of the S3 bucket
+    # @param options [Hash] Options passed directly to AWS to control the Resource created.  See Options below.
+    #
+    # Options:
+    #   **Deprecated, use :endpoint instead** :aws_endpoint [String] The object storage endpoint,
+    #                                                                if not AWS, e.g. 'https://sfo2.digitaloceanspaces.com'
+    #   **Deprecated, use :access_key_id instead** :access_key_id [String] Your AWS access key id
+    #   **Deprecated, use :secret_access_key instead** :aws_secret_access_key [String] Your AWS secret access key
+    #   **Deprecated, use :region instead** :aws_region [String] Your AWS region
+    #
+    #   All other options you provide are passed directly to the AWS client.
+    #   See https://docs.aws.amazon.com/sdk-for-ruby/v2/api/Aws/S3/Client.html#initialize-instance_method
+    #   for a full list of supported options.
+    def initialize(bucket, aws_access_key_id: nil, aws_secret_access_key: nil, aws_region: nil, aws_endpoint: nil, **options)
       @bucket = bucket
-      @aws_access_key_id = options[:aws_access_key_id]
-      @aws_secret_access_key = options[:aws_secret_access_key]
-      @aws_region = options[:aws_region]
-      @aws_endpoint = options[:aws_endpoint]
+      @options = options
+      @options[:access_key_id] ||= aws_access_key_id
+      @options[:secret_access_key] ||= aws_secret_access_key
+      @options[:region] ||= aws_region
+      @options[:endpoint] ||= aws_endpoint
     end
 
     # Call with a SitemapLocation and string data
@@ -41,20 +48,7 @@ def write(location, raw_data)
     private
 
     def s3_resource
-      @s3_resource ||= Aws::S3::Resource.new(s3_resource_options)
-    end
-
-    def s3_resource_options
-      options = {}
-      options[:region] = @aws_region if !@aws_region.nil?
-      options[:endpoint] = @aws_endpoint if !@aws_endpoint.nil?
-      if !@aws_access_key_id.nil? && !@aws_secret_access_key.nil?
-        options[:credentials] = Aws::Credentials.new(
-          @aws_access_key_id,
-          @aws_secret_access_key
-        )
-      end
-      options
+      @s3_resource ||= Aws::S3::Resource.new(@options)
     end
   end
 end
diff --git a/spec/sitemap_generator/adapters/aws_sdk_adapter_spec.rb b/spec/sitemap_generator/adapters/aws_sdk_adapter_spec.rb
@@ -2,9 +2,10 @@
 require 'aws-sdk-core'
 require 'aws-sdk-s3'
 
-describe 'SitemapGenerator::AwsSdkAdapter' do
+describe SitemapGenerator::AwsSdkAdapter do
+  subject(:adapter)  { described_class.new('bucket', **options) }
+
   let(:location) { SitemapGenerator::SitemapLocation.new(compress: compress) }
-  let(:adapter)  { SitemapGenerator::AwsSdkAdapter.new('bucket', options) }
   let(:options) { {} }
   let(:compress) { nil }
 
@@ -46,7 +47,7 @@
     end
   end
 
-  describe 'write' do
+  describe '#write' do
     context 'with no compress option' do
       let(:content_type) { 'application/xml' }
 
@@ -61,57 +62,80 @@
     end
   end
 
-  describe 's3_resource' do
-    it 'returns a new S3 resource' do
-      s3_resource_options = double(:s3_resource_options)
-      expect(adapter).to receive(:s3_resource_options).and_return(s3_resource_options)
-      expect(Aws::S3::Resource).to receive(:new).with(s3_resource_options).and_return('resource')
-      expect(adapter.send(:s3_resource)).to eql('resource')
+  describe '#initialize' do
+    context 'with region option' do
+      let(:options) { { region: 'region' } }
+
+      it 'sets region in options' do
+        expect(adapter.instance_variable_get(:@options)[:region]).to eql('region')
+      end
     end
-  end
 
-  describe 's3_resource_options' do
-    it 'does not include region' do
-      expect(adapter.send(:s3_resource_options)[:region]).to be_nil
+    context 'with deprecated aws_region option' do
+      let(:options) { { aws_region: 'region' } }
+
+      it 'sets region in options' do
+        expect(adapter.instance_variable_get(:@options)[:region]).to eql('region')
+      end
     end
 
-    it 'does not include credentials' do
-      expect(adapter.send(:s3_resource_options)[:credentials]).to be_nil
+    context 'with access_key_id option' do
+      let(:options) do
+        { access_key_id: 'access_key_id' }
+      end
+
+      it 'sets access_key_id in options' do
+        expect(adapter.instance_variable_get(:@options)[:access_key_id]).to eq('access_key_id')
+      end
     end
 
-    context 'with AWS region option' do
-      let(:options) { { aws_region: 'region' } }
+    context 'with deprecated aws_access_key_id option' do
+      let(:options) do
+        { aws_access_key_id: 'access_key_id' }
+      end
 
-      it 'includes the region' do
-        expect(adapter.send(:s3_resource_options)[:region]).to eql('region')
+      it 'sets access_key_id in options' do
+        expect(adapter.instance_variable_get(:@options)[:access_key_id]).to eq('access_key_id')
       end
     end
 
-    it 'does not include endpoint' do
-      expect(adapter.send(:s3_resource_options)[:endpoint]).to be_nil
+    context 'with secret_access_key option' do
+      let(:options) do
+        { secret_access_key: 'secret_access_key' }
+      end
+
+      it 'sets secret_access_key in options' do
+        expect(adapter.instance_variable_get(:@options)[:secret_access_key]).to eq('secret_access_key')
+      end
     end
 
-    context 'with AWS endpoint option' do
-      let(:options) { { aws_endpoint: 'endpoint' } }
+    context 'with deprecated aws_secret_access_key option' do
+      let(:options) do
+        { aws_secret_access_key: 'secret_access_key' }
+      end
+
+      it 'sets secret_access_key in options' do
+        expect(adapter.instance_variable_get(:@options)[:secret_access_key]).to eq('secret_access_key')
+      end
+    end
+
+    context 'with endpoint option' do
+      let(:options) do
+        { endpoint: 'endpoint' }
+      end
 
-      it 'includes the endpoint' do
-        expect(adapter.send(:s3_resource_options)[:endpoint]).to eql('endpoint')
+      it 'sets endpoint in options' do
+        expect(adapter.instance_variable_get(:@options)[:endpoint]).to eq('endpoint')
       end
     end
 
-    context 'with AWS access key id and secret access key options' do
+    context 'with deprecated aws_endpoint option' do
       let(:options) do
-        {
-          aws_access_key_id: 'access_key_id',
-          aws_secret_access_key: 'secret_access_key'
-        }
+        { aws_endpoint: 'endpoint' }
       end
 
-      it 'includes the credentials' do
-        credentials = adapter.send(:s3_resource_options)[:credentials]
-        expect(credentials).to be_a(Aws::Credentials)
-        expect(credentials.access_key_id).to eql('access_key_id')
-        expect(credentials.secret_access_key).to eql('secret_access_key')
+      it 'sets endpoint in options' do
+        expect(adapter.instance_variable_get(:@options)[:endpoint]).to eq('endpoint')
       end
     end
   end