Bump SDK #9

JDutil · 2017-01-09T20:12:14Z

We're interested in using this library with the latest sdk so this is the current WIP for our updates.

I've also got a branch containing a bunch of syntax updates I made while reading the code to fix rubocop warnings that I'd like to contribute, but am trying to keep this PR's changes minimal to just what is required first. You can check those out over at master...JDutil:bump-aws-sdk

fixes #7

proper hash for Client#update_item

JDutil · 2017-01-09T21:38:51Z

I've fixed the broken specs I was running into so this build should now be passing.

phstc · 2017-01-25T14:47:49Z

#7 (comment)

@awood45 any chance to get this reviewed? 🙏

awood45 · 2017-01-30T20:07:33Z

I've just returned from vacation, I'm going to start looking at this now. This will have to be a major version bump of this Gem, so will also look to see if there are other improvements we should bundle with such a change.

awood45

I can tell that there is going to be a need for extensive testing of a V2 bump of this library. Let's address these comments, and then I can create a branch/beta release.

awood45 · 2017-02-01T17:16:40Z

aws-sessionstore-dynamodb.gemspec

@@ -13,6 +13,6 @@ Gem::Specification.new do |spec|
  spec.test_files    = spec.files.grep(%r{^(test|spec|features)/})
  spec.require_paths = ["lib"]

-  spec.add_dependency 'aws-sdk-v1'
-  spec.add_dependency 'rack', '~> 1.0'
+  spec.add_dependency 'aws-sdk'


Should still keep a major version lock on this, such as '~> 2.0'.

Both because this in theory would allow a V1 dependency which would then mysteriously fail, and because if there's ever a V3, we don't want this to hard fail.

awood45 · 2017-02-01T17:17:54Z

lib/aws/session_store/dynamo_db/configuration.rb

@@ -12,7 +12,7 @@
 # language governing permissions and limitations under the License.

 require 'yaml'
-require 'aws-sdk-v1'
+require 'aws-sdk'

 module AWS::SessionStore::DynamoDB


Thing I'm unsure about: If we should change the top-level module to be Aws to match the nomenclature across other such libraries. There are valid arguments to both sides of this question.

@awood45 it would be nice to change the top-level module, but I'd do that in it's own PR to make code review simpler which is why I didn't bother for now.

That's reasonable enough. Like I said, there's probably a number of refactors we need to make or want to make before a full release of this. For example, I'm wondering if using Aws::Record wouldn't simplify a ton of this code, now that it exists.

awood45 · 2017-02-01T17:18:22Z

lib/aws/session_store/dynamo_db/configuration.rb

@@ -154,7 +153,7 @@ class Configuration
    #   See AWS DynamoDB documentation for table write_capacity for more
    #   information on this setting.
    # @option options [DynamoDB Client] :dynamo_db_client
-    #   (AWS::DynamoDB::ClientV2) DynamoDB client used to perform database
+    #   (Aws::DynamoDB::ClientV2) DynamoDB client used to perform database


There is no ClientV2 in V2 of the SDK. Just Client.

awood45 · 2017-02-01T17:20:03Z

lib/aws/session_store/dynamo_db/locking/base.rb

@@ -56,7 +56,7 @@ def delete_session(env, sid)
    def handle_error(env = nil, &block)
      begin
        yield
-      rescue AWS::DynamoDB::Errors::Base => e
+      rescue Aws::DynamoDB::Errors::Base => e


Aws::DynamoDB::Errors::ServiceError is the "Base" error class in V2.

@awood45 I tried changing to ServiceError, but it breaks one of the tests:

2.3.3 in aws-sessionstore-dynamodb-ruby/ on bump-sdk › be rake bundle exec rspec -t ~integration Run options: exclude {:integration=>true} .........F................ Failures: 1) AWS::SessionStore::DynamoDB Error handling for Rack Middleware with default error handler catches exception for inaccurate table key Failure/Error: lambda do expected #<Aws::DynamoDB::Errors::ValidationException: The provided key element does not match the schema> but nothing was raised # ./spec/aws/session_store/dynamo_db/error/default_error_handler_spec.rb:44:in `block (3 levels) in <top (required)>' Finished in 0.12099 seconds 26 examples, 1 failure Failed examples: rspec ./spec/aws/session_store/dynamo_db/error/default_error_handler_spec.rb:41 # AWS::SessionStore::DynamoDB Error handling for Rack Middleware with default error handler catches exception for inaccurate table key rake aborted! Command failed with status (1): [bundle exec rspec -t ~integration...] /Users/JDutil/Code/aws-sessionstore-dynamodb-ruby/tasks/test.rake:17:in `block (2 levels) in <top (required)>' /Users/JDutil/.rbenv/versions/2.3.3/bin/bundle:22:in `load' /Users/JDutil/.rbenv/versions/2.3.3/bin/bundle:22:in `<main>' Tasks: TOP => default => test:unit (See full trace by running task with --trace)

@awood45 There was another reference to base class I needed to update, but specs are passing again.

JDutil · 2017-02-01T18:08:30Z

@awood45 okay initial feedback is updated. Let me know if you need anything else.

dkoprov · 2017-02-27T13:51:50Z

@awood45 any updates on this issue?

awood45 · 2017-02-27T21:21:55Z

Sorry, I've been swamped lately. Also, admittedly, waiting to see if the newest DynamoDB feature could play a role once released.

phstc · 2017-02-27T21:29:24Z

@awood45 hehe ... Is the TTL configuration already available through the SDK?

awood45 · 2017-02-27T21:32:14Z

I will certainly update this space when it is. If we are major version bumping the session store, it may be worth using that version as the minimum, since TTL may nicely solve some performance issues in the previous iteration of this gem.

JDutil · 2017-02-27T21:34:57Z

Would be nice to move forward with this specific issue of updating the sdk version used, and then look at TTL after. Even if that means just updating master branch, and waiting to release the major version bump until TTL support is added. There is another open issue related to using TTL here #12

awood45 · 2017-02-27T21:43:32Z

We can do that - there's also the option of a pre-release of the MV bump.

phstc · 2017-02-27T21:51:51Z

@awood45 I think a specific issue and PR for TTL as mentioned by @JDutil does make sense.

cjyclaire · 2017-08-14T22:16:59Z

Latest release contains merged PR #17 that supports V2 SDK, it covers all this PR covers as well, closing : ) Thank you for the contribution all the same!

phstc · 2017-08-15T11:49:31Z

@cjyclaire great! How about the other opened pull requests adding the expire_at attribute?

Jeff Dutil and others added 8 commits January 9, 2017 08:34

Bump to aws-sdk and fix AWS to Aws namespace.

9cd10f0

Fix be_true deprecation.

9cd59a4

proper hash for Client#update_item

d0814d2

Merge pull request #1 from huoxito/proper-hash

ffd0c28

proper hash for Client#update_item

Fix more references to attribute types.

594281b

Spec fixes and update Aws namespace in comments.

f8aca7d

Fix spec.

4ed211e

Fix spec.

4d422d1

JDutil mentioned this pull request Jan 9, 2017

aws-sdk v2? #7

Closed

Jeff Dutil added 2 commits January 19, 2017 08:14

Bump rack to 2.0

11bf7bd

Set to >= 1.6.4 rack.

7f0ef8d

This was referenced Jan 25, 2017

Remove expected attributes from deleting session. #11

Closed

Cleanup code styling/quality according to rubocop. #14

Closed

awood45 suggested changes Feb 1, 2017

View reviewed changes

Jeff Dutil added 2 commits February 1, 2017 10:39

Set aws-sdk for v2 only.

9d41158

Fix comment reference to Client.

a1291dc

JDutil force-pushed the bump-sdk branch from 3532f14 to a1291dc Compare February 1, 2017 17:45

Jeff Dutil added 2 commits February 1, 2017 11:02

Change to ServiceError.

d684614

Fix spec.

f5a4165

cjyclaire closed this Aug 14, 2017

Bump SDK #9

Bump SDK #9

Uh oh!

Conversation

JDutil commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JDutil commented Jan 9, 2017

Uh oh!

phstc commented Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awood45 commented Jan 30, 2017

Uh oh!

awood45 left a comment

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!

JDutil commented Feb 1, 2017

Uh oh!

dkoprov commented Feb 27, 2017

Uh oh!

awood45 commented Feb 27, 2017

Uh oh!

phstc commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awood45 commented Feb 27, 2017

Uh oh!

JDutil commented Feb 27, 2017

Uh oh!

awood45 commented Feb 27, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phstc commented Feb 27, 2017

Uh oh!

cjyclaire commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phstc commented Aug 15, 2017

Uh oh!

Uh oh!

JDutil commented Jan 9, 2017 •

edited

Loading

phstc commented Jan 25, 2017 •

edited

Loading

phstc commented Feb 27, 2017 •

edited

Loading

awood45 commented Feb 27, 2017 via email •

edited

Loading

cjyclaire commented Aug 14, 2017 •

edited

Loading