Closed iMacTia closed 7 years ago
@kjg did you have a chance to take a look? Unfortunately tests are failing because Faraday 0.10 was release without support for ruby 1.8.7, so not related to my changes 😄
Thanks so much for taking the time to make a PR for this! I've been going through a job transition and haven't had time to look at this yet, but hopefully I'll have some time as soon as things start to settle down.
On December 6, 2016 at 4:20:28 AM, Mattia (notifications@github.com) wrote:
@kjg https://github.com/kjg did you have a chance to take a look? Unfortunately tests are failing because Faraday 0.10 was release without support for ruby 1.8.7, so not related to my changes 😄
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mgomes/api_auth/pull/137#issuecomment-265113087, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAInR7hZpTr9p_6z0EOCyBnEDP3a8Tpks5rFTbsgaJpZM4K-GGC .
Hi @kjg no worries I'm a maintainer as well (of Faraday, btw) so I know how it is :) If you need any help with fixing the tests please do let me know. Basically the choice is yours between:
In Faraday, we decided to go for the 2nd choice as always more dependencies were dropping the support as well and to embrace the syntax improvements available in new versions. Whatever your choice is, let me know when you've updated master and I'll pull changes in my branch as well.
Thanks @iMacTia. I vote for dropping Ruby 1.8.7 support. What do you think @kjg?
I think with the next major release it might make sense to drop support for 1.8.7
By major do you mean 3.0?
@mgomes @kjg I noticed you decided to change Faraday version according to the ruby version (which fixes the issue I was having with tests) and released a new version of the gem, but since you didn't tell me my PR remained behind 😞
I've now pulled changes from master into my branch and pushed again. Tests are green everywhere apart for one specific test, where Travis couldn't install nokogiri successfully (so unrelated to my changes).
I think we're there and at this point it really takes nothing to review and merge my branch in. I'm sure people lie @elliot-nelson from #95 will be happy to have it in the next release 👍
can you please either rebase this against master or give me Push access on the PR by clicking the "Allow edits from maintainers" checkbox? I think I've fixed the travis failure.
Thanks for your patients and for the PR!
Thanks @kjg I'll try to rebase now and push the changes 👍
@kjg looks like your fix worked 😃! Tests are green now
This addresses the issue highlighted in #95 . I went for the header
X-ORIGINAL-URI
as this is a standard for proxy servers (in primis Nginx). I've also added a test to show that the new test is working (plus, I've tested it with our application and is now working fine 👍 ).If you need to configure Nginx to make this working, you just need to add:
@kjg On a sidenote, while working on the code I realised that the
request_drivers
folder could be refactored as most of the implementation are just a copy-and-paste of the same methods. Maybe it would be a good idea to introduce a superclass likeBaseDriver
and make all drivers inheriting from it? Im sure it would save a good amount of code