delete useless resource files; dramatically simplified retry strategies and response parsing #18

apurvis · 2017-02-10T22:44:47Z

This includes all changes from #17 so if you prefer you can either merge that first, or just not merge that and go with this.

It also includes

delete useless resource files
move services to a folder
use a unified and reasonable retry strategy with the Retriable gem instead of chaos theory
ReadOnlyService superclass
deprecating LogLevelDataDownloadService - all the functionality lives in LogLevelDataService with the LogLevelDataDownloadService class to be removed whenever there's a 2.0 release

There are no functional changes to external APIs though if someone is reaching into the gem and mucking with classes like AdvertiserResource, this will break their code. In my opinion that means this doesn't warrant a major or even minor version bump - 1.0.1 would be fine.

@joshuawscott @infinite-monkeys

Merge master

… add CHANGELOG.md

Get rid of resource files

Better retries; move service files to their own directory

* Add APPNEXUS_API_DEBUG env variable falg; Add respond_to_missing * Just use an accessor for raw_json * Get rid of :return_response flag; it's not used

… time; set log level to debug if debug mode turned on

…ion class with default logger (#6) * Consolidate download service * Fix final issues with log level data download * Real spec for log level data download; default feed

apurvis · 2017-02-14T03:14:31Z

lib/appnexusapi/faraday/raise_http_error.rb

+          if body['error_code'] == RATE_EXCEEDED_ERROR
+            raise AppnexusApi::RateLimitExceeded, "Retry after #{response.response_headers['retry-after']}s"
+          elsif body['error_id'] && !body['error_id'].empty?
+            # TODO: this should raise an error, but a lot of the specs currently have error responses


this is kind of a bummer; not sure this is because of my account's access and @infinite-monkeys will be able to replace the .yml files but the specs were passing even with these error responses (in fact this kind of error response never even generated any errors, which is ... problematic)

…ified with

apurvis · 2017-04-06T08:41:42Z

@joshuawscott any plans to merge/release this?

apurvis · 2017-07-19T01:43:20Z

@joshuawscott this has been open for over 5 months now... any plans to merge this? we have been running this streamlined version in production daily for that entire time.

joshuawscott · 2017-07-20T22:57:12Z

@mcmoyer can you take a look and give a 👍 / 👎

apurvis · 2017-08-30T09:50:35Z

@mcmoyer / @joshuawscott ?

apurvis · 2017-09-05T03:30:51Z

ping (sorry to be annoying, but it would be good if we could get this merged in the next 4 weeks)

apurvis · 2017-09-15T10:26:07Z

ping

mcmoyer · 2017-09-15T15:23:09Z

I’ll look at it this weekend…sorry for the delay

mcmoyer · 2017-09-18T15:09:38Z

had issues getting the tests to run this weekend. I believe it may be due to permissions with our sandbox login. Will check into that today

apurvis · 2017-09-18T17:20:27Z

thanks... are you trying to create new VCR files or just run the specs as is?

apurvis · 2017-09-18T22:01:16Z

maybe you have an old .env file that is out of date? you should be able to just delete it and run specs without it
https://github.com/lumoslabs/appnexusapi#writing-new-specs-or-updating-old-ones

mcmoyer · 2017-09-20T15:07:36Z

was able to get the tests running by renaming my .env file. I’m unable to get all the specs to pass

23 examples, 12 failures, 1 pending

It seems to be all permissions related so getting in touch with Appnexus to see if I can get a login with more permissions in the test system.

apurvis · 2017-09-20T18:47:41Z

i get

23 examples, 0 failures, 1 pending

but yeah the one spec i could not get to pass had to do with permissions on my account (that's the pending one).

however, the specs should not be hitting appnexus in any way, as long as you don't clear the VCR files. are you trying to regenerate the VCR files from scratch? if so you need the .env file:

APPNEXUS_USERNAME=me@yahooooo.com
APPNEXUS_PASSWORD=PaSsWoRd
APPNEXUS_URI=https://api-test.appnexus.com

apurvis · 2017-09-20T18:48:06Z

specifically the pending one is:

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) AppnexusApi::PlacementService default placement
     # seems to rely on a configured default_site_id
     Failure/Error: raise 'seems to rely on a configured default_site_id'

     RuntimeError:
       seems to rely on a configured default_site_id
     # ./spec/integration/placement_spec.rb:10:in `block (2 levels) in <top (required)>'

apurvis · 2017-09-20T18:51:57Z

FWIW i was able to get the specs to pass just with a fresh checkout and without hitting appnexus's actual test server in any way.

git clone git@github.com:lumoslabs/appnexusapi.git
cd appnexusapi
bundle install
bundle exec rspec

maybe it's an issue with some older version of some dependency gem that needs an update? my Gemfile.lock looks like this:

PATH
  remote: .
  specs:
    appnexusapi (1.1.0.pre.prerelease)
      faraday (~> 0.9)
      faraday_middleware
      multi_json
      retriable (>= 2.0)

GEM
  remote: https://rubygems.org/
  specs:
    addressable (2.5.2)
      public_suffix (>= 2.0.2, < 4.0)
    coderay (1.1.2)
    crack (0.4.3)
      safe_yaml (~> 1.0.0)
    diff-lcs (1.3)
    dotenv (2.2.1)
    faraday (0.13.1)
      multipart-post (>= 1.2, < 3)
    faraday_middleware (0.12.2)
      faraday (>= 0.7.4, < 1.0)
    hashdiff (0.3.6)
    method_source (0.8.2)
    multi_json (1.12.2)
    multipart-post (2.0.0)
    pry (0.11.0)
      coderay (~> 1.1.0)
      method_source (~> 0.8.1)
    public_suffix (3.0.0)
    retriable (3.1.1)
    rspec (3.6.0)
      rspec-core (~> 3.6.0)
      rspec-expectations (~> 3.6.0)
      rspec-mocks (~> 3.6.0)
    rspec-core (3.6.0)
      rspec-support (~> 3.6.0)
    rspec-expectations (3.6.0)
      diff-lcs (>= 1.2.0, < 2.0)
      rspec-support (~> 3.6.0)
    rspec-mocks (3.6.0)
      diff-lcs (>= 1.2.0, < 2.0)
      rspec-support (~> 3.6.0)
    rspec-support (3.6.0)
    safe_yaml (1.0.4)
    vcr (3.0.3)
    webmock (3.0.1)
      addressable (>= 2.3.6)
      crack (>= 0.3.2)
      hashdiff

PLATFORMS
  ruby

DEPENDENCIES
  appnexusapi!
  bundler (>= 1.2.0)
  dotenv
  pry
  rspec
  vcr
  webmock

BUNDLED WITH
   1.16.0.pre.2

apurvis · 2017-09-26T22:04:09Z

ping

mcmoyer · 2017-09-28T14:57:05Z

I’m able to run the specs without a .env file. It’s not ideal since our login is unable to run the specs against the actual appnexus sandbox. Since it’s taking a while to get the required permissions from appnexus, I’m going to merge these so you’re not held up any longer

apurvis · 2017-09-28T14:58:37Z

awesome, thanks.

apurvis · 2017-09-28T15:05:00Z

(i would definitely squash + merge, not just merge... all the tiny commits are not helpful)

apurvis and others added 30 commits February 8, 2017 19:48

Merge pull request #1 from simplifi/master

98c0de2

Merge master

Remove dependencies on Pester (use Retriable instead) and NullLogger;…

2cfaae6

… add CHANGELOG.md

README

46b4a34

README

36a47df

README

718f80d

Move integration specs to integration folder

5f209c3

Set the Resource classes dynamically

ef07b8e

Don't use class variable

4aea9a1

Ditch unnecessary resource_class methods

1ee5db8

Move method

287566e

Prerelease version - no benefit to actually releasing this

6d4b222

Better retries

c350521

Fix log issue while retrying

400823d

Merge branch 'use_retriable' into get_rid_of_resource_files

6019144

Unify the retries

6e9f07d

Refactor run_request method

dd0a792

Refactor

c76fbc6

Refactor read only service

f2061f2

Unify Resource class

3ca6c65

Merge get_rid_of_resource_files

fdf98da

Merge pull request #2 from lumoslabs/get_rid_of_resource_files

8cbc896

Get rid of resource files

Merge pull request #3 from lumoslabs/better_retries

de401b8

Better retries; move service files to their own directory

Constantize RATE_LIMIT_WAITER proc

1fbed14

Style

627afd9

Ditch unused :return_response option; default logger to INFO level (#4)

da5346a

* Add APPNEXUS_API_DEBUG env variable falg; Add respond_to_missing * Just use an accessor for raw_json * Get rid of :return_response flag; it's not used

response_headers not response.headers; add an extra 15s to retry wait…

b751b85

… time; set log level to debug if debug mode turned on

replace raw_json method

28518aa

Style

0f5ca29

Simplify requires in the service dir

e9c0c13

has_key? is deprecated in favor of key?

03e0f88

apurvis and others added 2 commits February 13, 2017 03:04

Refactor download service to a single service class; add a configurat…

7b7285c

…ion class with default logger (#6) * Consolidate download service * Fix final issues with log level data download * Real spec for log level data download; default feed

Adjust README

a927ee1

apurvis commented Feb 14, 2017

View reviewed changes

apurvis mentioned this pull request Feb 15, 2017

Refactor useless Resource classes; add VCR specs; make the retry strategy sane 33Across/appnexusapi#2

Closed

apurvis@lumoslabs.com added 3 commits February 20, 2017 22:38

Add Resource#save method to allow saving resources that have been mod…

10d3a96

…ified with

Add #save to README

457e467

Use require instead of require_relative

dc73f4e

apurvis mentioned this pull request Jul 19, 2017

Get rid of resource files; replace Pester with Retriable #17

Closed

apurvis@lumoslabs.com and others added 2 commits July 31, 2017 23:49

Changelog

1eb53d5

Some better style (#7)

127b1e1

mcmoyer merged commit 093dfc5 into simplifi:master Oct 3, 2017

guigs mentioned this pull request Oct 3, 2017

Evaluate/backport changes from appnexusapi gem ComboStrikeHQ/rocketfuel_api#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delete useless resource files; dramatically simplified retry strategies and response parsing #18

delete useless resource files; dramatically simplified retry strategies and response parsing #18

apurvis commented Feb 10, 2017 •

edited

Loading

apurvis Feb 14, 2017

apurvis commented Apr 6, 2017

apurvis commented Jul 19, 2017

joshuawscott commented Jul 20, 2017

apurvis commented Aug 30, 2017

apurvis commented Sep 5, 2017

apurvis commented Sep 15, 2017

mcmoyer commented Sep 15, 2017

mcmoyer commented Sep 18, 2017

apurvis commented Sep 18, 2017

apurvis commented Sep 18, 2017

mcmoyer commented Sep 20, 2017

apurvis commented Sep 20, 2017

apurvis commented Sep 20, 2017

apurvis commented Sep 20, 2017

apurvis commented Sep 26, 2017

mcmoyer commented Sep 28, 2017

apurvis commented Sep 28, 2017

apurvis commented Sep 28, 2017

delete useless resource files; dramatically simplified retry strategies and response parsing #18

delete useless resource files; dramatically simplified retry strategies and response parsing #18

Conversation

apurvis commented Feb 10, 2017 • edited Loading

apurvis Feb 14, 2017

Choose a reason for hiding this comment

apurvis commented Apr 6, 2017

apurvis commented Jul 19, 2017

joshuawscott commented Jul 20, 2017

apurvis commented Aug 30, 2017

apurvis commented Sep 5, 2017

apurvis commented Sep 15, 2017

mcmoyer commented Sep 15, 2017

mcmoyer commented Sep 18, 2017

apurvis commented Sep 18, 2017

apurvis commented Sep 18, 2017

mcmoyer commented Sep 20, 2017

apurvis commented Sep 20, 2017

apurvis commented Sep 20, 2017

apurvis commented Sep 20, 2017

apurvis commented Sep 26, 2017

mcmoyer commented Sep 28, 2017

apurvis commented Sep 28, 2017

apurvis commented Sep 28, 2017

apurvis commented Feb 10, 2017 •

edited

Loading