Do not use empty system properties for endpoint #135
Conversation
When configuring the endpoint, jclouds should use the default endpoint value if an empty string is passed via system properties.
| putAllAsString(propertiesPrefixedWithJcloudsApiOrProviderId(getSystemProperties(), apiMetadata.getId(), providerId), defaults); | ||
| Map<String, Object> system = propertiesPrefixedWithJcloudsApiOrProviderId(getSystemProperties(), apiMetadata.getId(), providerId); | ||
| if (Strings.isNullOrEmpty((String) system.get(PROPERTY_ENDPOINT))) { | ||
| system.remove(PROPERTY_ENDPOINT); |
There was a problem hiding this comment.
I don't strongly oppose this but why does the caller set an empty string? Should we check all keys to see if they are empty? Should we do other input validation like checking whether the endpoint is a URL?
There was a problem hiding this comment.
I think storing endpoint as a URL rather than a string would be preferable. I think the confusing part is that setting a system property like -Djclouds.endpoint= means an empty endpoint, rather than one that is not set, like here: https://github.com/gaul/s3proxy/blob/master/Dockerfile#L35
There was a problem hiding this comment.
Does it make more sense for the application not to set a value when it doesn't have a value? This seems like a weird quirk of an application's configuration (environment variables) that makes more sense to address in the application than in jclouds itself.
Also I believe that System.getProperties().remove() is thread-safe but can mess up iteration order and anyway modifying global state is a bad idea.
There was a problem hiding this comment.
Maybe. I'll submit an s3proxy change for the same.
The above doesn't mutate System.getProperties() -- the map is populated with the values from system properties, API metadata, and provider metadata and then the key is removed from that map. Maybe I should've used a better variable name, but I don't see how the global state is mutated.
There was a problem hiding this comment.
Sorry I did not realize that propertiesPrefixedWithJcloudsApiOrProviderId returned a new Map instead of mutating the existing one.
|
Closing since I'm not comfortable with this change. |
When configuring the endpoint, jclouds should use the default endpoint
value if an empty string is passed via system properties.