These are chat archives for spring-cloud/spring-cloud

16th
Apr 2015
wojtkiewicz
@wojtkiewicz
Apr 16 2015 11:52
Hi, could you take a look at spring-cloud/spring-cloud-config#124
Is there anything else that we should change?
Dave Syer
@dsyer
Apr 16 2015 12:36
```
```
```
public interface TextEncryptorLocator {
    TextEncryptor locate();
    TextEncryptor locate(Environment environment);
}
Why does it need 2 methods?
Isn't the point that in general the abstraction is needed precisely for the locate(Environment) method, so the other should never be used?
Dave Syer
@dsyer
Apr 16 2015 12:43
I see. It's a "temporary solution" because we need to continue to support the /decrypt and /encrypt endpoints in their current form
I'm not sure I like a method in an interface which is only there for legacy reasons
Maybe locate(String) would be better anyway?
What kind of implementation did you envisage for your own customization?
wojtkiewicz
@wojtkiewicz
Apr 16 2015 12:48
Generally speaking we return different TextEncryptor for different application-name and profile
Dave Syer
@dsyer
Apr 16 2015 12:49
I guess locate(String, String, String) would match the EnvironmentRepository then
But that leaves the question of what to do about the existing endpoints
wojtkiewicz
@wojtkiewicz
Apr 16 2015 12:50
you mean locate(applicationName, profiles, label) ?
those non-argument methods were designed to support notion of default TextEncryptor
which translate to existing single TextEncryptor per server
Dave Syer
@dsyer
Apr 16 2015 12:52
Yes. And I get that, but I don't know if I want the notion of a "default" to leak into the interface.
wojtkiewicz
@wojtkiewicz
Apr 16 2015 12:53
ok, I understand
Dave Syer
@dsyer
Apr 16 2015 12:53
applicationName, profiles, label all have defaults in other layers
unfortunately the "label" has different defaults depending on the EnvironmentRepository implementation
I hate that too, but it is already the case
Maybe locate(applicationName, profiles) is good enough
wojtkiewicz
@wojtkiewicz
Apr 16 2015 12:54
for our use case it is sufficient
Dave Syer
@dsyer
Apr 16 2015 12:54
Yeah. I'm trying to think beyond that.
Especially for the "default" case
I think we might need to expose applicationName, profiles everywhere a TextEncryptor is needed, and give them default values
I can hack on your code a bit when I have time. Probably not today though.
wojtkiewicz
@wojtkiewicz
Apr 16 2015 12:57
We can do majority of the work if you can demonstrate what you have in mind...
Dave Syer
@dsyer
Apr 16 2015 12:58
I think if you hack it down to that one method the compiler will tell you where you need to patch things
Normally there will be a default value available through ConfigServerProperties or similar
wojtkiewicz
@wojtkiewicz
Apr 16 2015 13:00
If I understood correctly the idea is to introduce default application-name and profile and use that for legacy /encrypt and /decrypt endpoints
application profiles should be an array of strings just like in Environment abstraction?
Dave Syer
@dsyer
Apr 16 2015 13:05
Yes. Comma-separated. Default is "default". But that should come from ConfigServerProperties I believe.
Probably we could make /encrypt and /decrypt match the new feature by optionally accepting parameters
/encrypt?name=foo&profiles=bar maybe
wojtkiewicz
@wojtkiewicz
Apr 16 2015 13:08
We can do that but is this consistent with REST API in EnvironmentController?
Dave Syer
@dsyer
Apr 16 2015 13:09
No. That's why I said "maybe".
wojtkiewicz
@wojtkiewicz
Apr 16 2015 13:09
Those endpoints take name, profiles and label as path variables...
Dave Syer
@dsyer
Apr 16 2015 13:09
It's not very RESTful anyway
Maybe /encrypt/foo/bar is not too crazy
I think on balance that's better. Do you?
This something we came up with in our PoC
this looks very similar to what you've suggested
Dave Syer
@dsyer
Apr 16 2015 13:12
Yes.
wojtkiewicz
@wojtkiewicz
Apr 16 2015 13:12
@RequestMapping(value = "/encrypt/{name}/{profiles}", method = RequestMethod.POST)
Dave Syer
@dsyer
Apr 16 2015 13:13
Can we use default values to get rid of the UnsupportedOperationExceptions?
wojtkiewicz
@wojtkiewicz
Apr 16 2015 13:13
sure
you mean 'UnsupportedOperationException' that is thrown in controller?
Dave Syer
@dsyer
Apr 16 2015 13:16
Yes
wojtkiewicz
@wojtkiewicz
Apr 16 2015 13:17
We didn't even considered doing that in production code :-)
That was just a demo, we won't do that in real PR
Dave Syer
@dsyer
Apr 16 2015 13:18
OK. Sounds good.
wojtkiewicz
@wojtkiewicz
Apr 16 2015 13:20
Thanks for your feedback. We should be done by tomorrow with these changes.