Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Imgix Client Purge Method #37

Merged
merged 5 commits into from
Oct 29, 2018
Merged

Imgix Client Purge Method #37

merged 5 commits into from
Oct 29, 2018

Conversation

tyrauber
Copy link
Contributor

Implement basic token authentication api.imgix.com/v2/image/purger request using net:http and web mock for test suite

Implement basic token authentication api.imgix.com/v2/image/purger request using net:http and web mock for test suite
Copy link
Contributor

@jayeb jayeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @tyrauber. Purge support is something we've been talking about adding to this library (and all of our core libraries) for a long time, but haven't had the time to do it just yet.

There's a couple of specific problems with the implementation at the moment, which I've noted in comments below. Happy to discuss any of these further if you have questions.

I also have a feature request that I'd like you to add if possible--we currently track how often our libraries are used via the ixlib parameter, which we append to all URLs generated by each library unless the user turns the feature off. We'd like to do the same thing here, but with the user-agent header. Can you modify the code to add User-Agent: imgix rb-VERSION to each purge request?

lib/imgix/client.rb Outdated Show resolved Hide resolved
lib/imgix/client.rb Show resolved Hide resolved
- use @api_token in purge request
- remove full url option
- add ixlib parameter to purge requests
- update purge_test.rb and README
@tyrauber
Copy link
Contributor Author

@jayeb Sorry for jumping the roadmap. Unfortunately, purge support is something we require in production ASAP. When I saw the curl purge example, I thought I might as well take the extra hour and fork and update the gem.

This last commit should resolve the outstanding concerns, but let me know.

FYI - I added the user agent as requested - imgix @library-@version - but it appears that the rest of the gem only uses @library-@version. Can you confirm imgix should also be included in the user agent? If so, the core path method should probably also be updated.

Copy link
Contributor

@jayeb jayeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more fixes, @tyrauber. Thanks for sticking with it.

lib/imgix/client.rb Outdated Show resolved Hide resolved
lib/imgix/client.rb Outdated Show resolved Hide resolved
test/units/purge_test.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
- remove ability to disable library tracking
- test should test user-agent
- update README content
@tyrauber
Copy link
Contributor Author

Of course, @jayeb, Glad to help. Let me know if you need anything else.

@jayeb jayeb merged commit fcb0023 into imgix:master Oct 29, 2018
@@ -20,4 +20,6 @@ Gem::Specification.new do |spec|

spec.required_ruby_version = '>= 1.9.0'
spec.add_dependency 'addressable'
spec.add_dependency 'http'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry if I'm being dense, but is this gem actually being used here? AFAIK, it uses an HTTP class, and this PR only uses Net::HTTP, not HTTP. It'd be great to not require it if we don't really need it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benhutton You are absolutely correct. Addressed in #38. Thanks for catching that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants