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

Not working with ruby 2.5 #171

Closed
EugenMayer opened this issue Dec 23, 2018 · 8 comments · Fixed by #181
Closed

Not working with ruby 2.5 #171

EugenMayer opened this issue Dec 23, 2018 · 8 comments · Fixed by #181
Labels

Comments

@EugenMayer
Copy link
Contributor

When working with 2.5 ( e.g. ruby:2.5-slim-stretch official docker image )
One gets a 301 moved permanently on each KV access

/usr/local/bundle/gems/diplomat-2.0.2/lib/diplomat/kv.rb:88:in `get': status 301: <a href="/v1/kv/tiller/globals/all?dc=stable&amp;keys">Moved Permanently</a>. (Diplomat::UnknownStatus)
@pierresouchay
Copy link
Member

Weird, what is your call exactly? How did you configure the lib?
Only when listing keys?

@rockpapergoat
Copy link

rockpapergoat commented Feb 1, 2019

oh, huh… i see that now with ruby 2.5, too.

passing faraday middleware options for which middleware to load doesn't seem to work for me, but that will solve it. either that, or require 'faraday_middleware' and then using conn.use FaradayMiddleware::FollowRedirects when setting up the connection should get it going.

when setting up a connection manually with faraday under ruby 2.5.1 here, this works just fine. i haven't finished making these changes and testing in a branch yet, but i'd start here.

@pierresouchay
Copy link
Member

@EugenMayer @rockpapergoat How?

Can you please try this?

Create file test.rb (executable):

#!/usr/bin/env ruby
require 'diplomat'

raise "USAGE: <KV_PATH_TO_GET>" unless ARGV.count > 0

Diplomat.configure do |config|
  config.url = ENV['CONSUL_HTTP_ADDR'] || "http://localhost:8500"
end
foo = Diplomat::Kv.get(ARGV[0])
puts "#{ARGV[0]}: #{foo}"

Run it with CONSUL_HTTP_ADDR=http://localhost:8500 ./test.rb PATH_TO_KV

(You can replace CONSUL_HTTP_ADDR with the value of the consul agent you are targeting)

On my side, with ruby-2.5.3, working quite well...
What OS? Docker/Alpine?

@rockpapergoat
Copy link

sorry for the delay. my issue was not specifying the full path to the key. ubuntu 16.04/ruby 2.5.1 worked/works fine, as far as i can tell.

@EugenMayer
Copy link
Contributor Author

@pierresouchay its all up there

When working with 2.5 ( e.g. ruby:2.5-slim-stretch official docker image )

@pierresouchay
Copy link
Member

@EugenMayer So, I suspect you are using :

Diplomat::Kv.get('/tiller/globals/all', ...)

right?

Using:

Diplomat::Kv.get('tiller/globals/all', ...)

Would probably fix your issue (note that I removed the first '/')

@EugenMayer Can you confirm this?

@EugenMayer
Copy link
Contributor Author

Well @pierresouchay i am not sure you are right about that, since in this case, the request is wrapped by https://github.com/markround/tiller - but i remember having issues with my direct ruby diplomat implementation.

Still looking at the error and the cause it seem viable - still why did this happen with ruby 2.5 but not with < 2.5? Does not make sense this redirect is only triggered then. Probably in 2.5 the redirect is not followed automatically, similar to curls -L ?

Thanks

@pierresouchay
Copy link
Member

@EugenMayer might be. I definitely have a 302 if using /anykey for a KV.get(). This is a problem anyway, because I am not sure of behavior if using put for instance.

There might be a change in ruby to default following 302 from 2.4 to 2.5, but we might simply fix this globally in diplomat if self to avoid this behavior

pierresouchay added a commit that referenced this issue Mar 11, 2019
pierresouchay added a commit that referenced this issue Mar 15, 2019
* Avoid having a HTTP 302 if key requested starts with '/'

This might fix: #171

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

Successfully merging a pull request may close this issue.

3 participants