From 4e7f8175ea89c777abcc3bcb8d7973aec846b227 Mon Sep 17 00:00:00 2001 From: Nicolas Brousse Date: Mon, 17 Jan 2022 07:30:19 +0100 Subject: [PATCH] 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 --- README.md | 31 ++---- .../adapters/aws_sdk_adapter.rb | 48 +++++----- .../adapters/aws_sdk_adapter_spec.rb | 94 ++++++++++++------- 3 files changed, 89 insertions(+), 84 deletions(-) diff --git a/README.md b/README.md index e704be46..eb526813 100644 --- 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 index 93c483eb..b89bed4b 100644 --- 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 index 11aa6180..1225e567 100644 --- 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