You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by GitBox <gi...@apache.org> on 2022/03/03 18:57:05 UTC
[GitHub] [jclouds] timuralp opened a new pull request #135: Do not use empty system properties for endpoint
timuralp opened a new pull request #135:
URL: https://github.com/apache/jclouds/pull/135
When configuring the endpoint, jclouds should use the default endpoint
value if an empty string is passed via system properties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@jclouds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] timuralp commented on a change in pull request #135: Do not use empty system properties for endpoint
Posted by GitBox <gi...@apache.org>.
timuralp commented on a change in pull request #135:
URL: https://github.com/apache/jclouds/pull/135#discussion_r820248676
##########
File path: core/src/main/java/org/jclouds/ContextBuilder.java
##########
@@ -370,7 +371,11 @@ private Properties currentStateToUnexpandedProperties() {
defaults.setProperty(PROPERTY_CREDENTIAL, credential);
if (overrides.isPresent())
putAllAsString(overrides.get(), defaults);
- 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);
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@jclouds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul commented on a change in pull request #135: Do not use empty system properties for endpoint
Posted by GitBox <gi...@apache.org>.
gaul commented on a change in pull request #135:
URL: https://github.com/apache/jclouds/pull/135#discussion_r820088521
##########
File path: core/src/main/java/org/jclouds/ContextBuilder.java
##########
@@ -370,7 +371,11 @@ private Properties currentStateToUnexpandedProperties() {
defaults.setProperty(PROPERTY_CREDENTIAL, credential);
if (overrides.isPresent())
putAllAsString(overrides.get(), defaults);
- 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);
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@jclouds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul commented on a change in pull request #135: Do not use empty system properties for endpoint
Posted by GitBox <gi...@apache.org>.
gaul commented on a change in pull request #135:
URL: https://github.com/apache/jclouds/pull/135#discussion_r820301900
##########
File path: core/src/main/java/org/jclouds/ContextBuilder.java
##########
@@ -370,7 +371,11 @@ private Properties currentStateToUnexpandedProperties() {
defaults.setProperty(PROPERTY_CREDENTIAL, credential);
if (overrides.isPresent())
putAllAsString(overrides.get(), defaults);
- 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);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@jclouds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] timuralp commented on a change in pull request #135: Do not use empty system properties for endpoint
Posted by GitBox <gi...@apache.org>.
timuralp commented on a change in pull request #135:
URL: https://github.com/apache/jclouds/pull/135#discussion_r820367119
##########
File path: core/src/main/java/org/jclouds/ContextBuilder.java
##########
@@ -370,7 +371,11 @@ private Properties currentStateToUnexpandedProperties() {
defaults.setProperty(PROPERTY_CREDENTIAL, credential);
if (overrides.isPresent())
putAllAsString(overrides.get(), defaults);
- 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);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@jclouds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul commented on a change in pull request #135: Do not use empty system properties for endpoint
Posted by GitBox <gi...@apache.org>.
gaul commented on a change in pull request #135:
URL: https://github.com/apache/jclouds/pull/135#discussion_r820593576
##########
File path: core/src/main/java/org/jclouds/ContextBuilder.java
##########
@@ -370,7 +371,11 @@ private Properties currentStateToUnexpandedProperties() {
defaults.setProperty(PROPERTY_CREDENTIAL, credential);
if (overrides.isPresent())
putAllAsString(overrides.get(), defaults);
- 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);
Review comment:
Sorry I did not realize that `propertiesPrefixedWithJcloudsApiOrProviderId` returned a new `Map` instead of mutating the existing one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@jclouds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org