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

Support placeholders in @FeignClient definitions #414

Closed
CanD42 opened this issue Jun 29, 2015 · 8 comments
Closed

Support placeholders in @FeignClient definitions #414

CanD42 opened this issue Jun 29, 2015 · 8 comments

Comments

@CanD42
Copy link

CanD42 commented Jun 29, 2015

The com.netflix.loadbalancer.LoadBalancerContext takes the original protocol from the @FeignClient definition so if you want to use https you can specify that with (as discussed in #337)

@FeignClient("https://users")

It would be very helpful to support placeholder here so that you can have different schemes in various landscapes:

@FeignClient("${feign.url.scheme}://users")
@beku8
Copy link

beku8 commented Jul 1, 2015

This feature would be nice, not just to resolve scheme, but for use cases like plug local app to cloud dev environment with prefixes etc.

@CanD42
Copy link
Author

CanD42 commented Jul 22, 2015

Any change to get this into the next release? Thanks

@czolek
Copy link

czolek commented Sep 28, 2015

I have stumbled upon this issue myself and thought it would be very helpful to have this resolved. That's why I have implemented the fix (I call it a fix as it is clearly an omission and I categorise it as a bug).
I will submit a pull request in a day or two when I learn how the process works as I have never done so before.
I'll keep you posted.

@dsyer
Copy link
Contributor

dsyer commented Sep 29, 2015

I believe you can set <service>.ribbon.IsSecure (or omit <service> for all). Feign hasn't yet been spruced up to support the new ServerIntrospector abstraction (commit aa86267), but it probably should be. So I don't think placeholders is necessarily the way to solve this problem (however handy they would be).

@beku8
Copy link

beku8 commented Sep 29, 2015

@dsyer I would like to use this feature not to support ssl, but to add prefixes to the services. E.g I would set -Dfoo.servicePrefix=dev_ & feign would connect to dev_foo instead of foo etc.

dsyer pushed a commit that referenced this issue Sep 29, 2015
Other Ribbon clients in Spring Cloud use a ServerIntrospector
to check if the Server wanted to be secure. This change levels things
up (and makes the Spring Feign Client a lit easier to customize).

See gh-414
@dsyer
Copy link
Contributor

dsyer commented Sep 29, 2015

I don't think that's a great use case either. The whole point of service discovery is so you don't have to do this kind of thing.

@czolek
Copy link

czolek commented Sep 29, 2015

I agree with Dave that placeholders have little use in service discovery. However they are quite useful when using url attribute without service discovery enabled. Also I found it troublesome not to be able to use placeholders in @EnableFeignClients basePackages attribute.
The use-case is that we are developing our own starter project which enables feign clients and by default registers some clients, interceptors etc. Users, of course, can provide their own clients and the idea is to provide an easy to use lib where clients location is specified via a property without the need to provide any additional configuration class. By default we scan at root level /**, but someone may wish to limit scan scope.
I won't argue that the whole idea is vital but it is just nice to have such an option.

jkschneider pushed a commit to jkschneider/spring-cloud-netflix that referenced this issue Oct 8, 2015
Other Ribbon clients in Spring Cloud use a ServerIntrospector
to check if the Server wanted to be secure. This change levels things
up (and makes the Spring Feign Client a lit easier to customize).

See spring-cloudgh-414
@kflorence
Copy link

👍 only because this seems like expected behavior and it would be beneficial to pull values from a config -- otherwise, be cautious about disrupting behavior between environments.

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

No branches or pull requests

6 participants