Fix regression where URIs as input for the entrypoint no longer worked. #107

pilhuhn · 2016-07-08T10:46:31Z

Some normalization logic in 2.2.0 introduced a regression where URI:HTTP object were no longer accepted as input for entrypoints.
This pr checks for uri objects and converts them to string if needed.

@simon3z @yaacov

coveralls · 2016-07-08T10:55:07Z

Coverage increased (+0.1%) to 94.998% when pulling 09955b3 on pilhuhn:re-introduce-endpoint-param-as-uri into b52b1f8 on hawkular:master.

chessbyte · 2016-07-08T12:02:06Z

lib/hawkular/base_client.rb

@@ -139,10 +139,11 @@ def normalize_entrypoint_url(entrypoint, suffix_path)
      strip_path.nil? || suffix_path = strip_path
      strip_path = suffix_path.gsub(%r{^/}, '')
      strip_path.nil? || suffix_path = strip_path
-      strip_entrypoint = entrypoint.gsub(%r{/$}, '')
+      the_entry_point = (entrypoint.is_a? URI::HTTP) ? entrypoint.to_s : entrypoint


Why not the simpler entrypoint = entrypoint.to_s?

On 8 Jul 2016, at 14:02, Oleg Barenboim wrote:

Why not the simpler entrypoint = entrypoint.to_s?

Isn't a to_s on something that is already a string a bit odd? But sure I
can change that.

Idiomatic Ruby /cc @Fryguy

coveralls · 2016-07-08T12:59:05Z

Coverage increased (+0.2%) to 95.009% when pulling 0cf88f2 on pilhuhn:re-introduce-endpoint-param-as-uri into b52b1f8 on hawkular:master.

abonas · 2016-07-08T13:47:35Z

lib/hawkular/base_client.rb

-      strip_entrypoint = entrypoint.gsub(%r{/$}, '')
-      strip_path.nil? && strip_entrypoint = entrypoint
+      strip_entrypoint = entrypoint.to_s.gsub(%r{/$}, '')
+      strip_path.nil? && strip_entrypoint = entrypoint.to_s


what about empty strings? (I mean '')

The internal clients use that method to get to the correct base-endpoint, so I don't think this is an issue that could arise. I can add a guard on suffix_path being empty to warn the developer of a new sub-client - the nil case is easily caught by nil.gsubbailing out :-)

abonas · 2016-07-08T13:50:11Z

is this applying to all subclients or only main client and metrics?

pilhuhn · 2016-07-08T14:27:12Z

@abonas when you do Hawkular::Client.new then it applies to all sub-clients. As CM is calling it differently, I did the explicit check for their use case.

coveralls · 2016-07-08T14:29:49Z

Coverage increased (+0.2%) to 95.014% when pulling cd40a17 on pilhuhn:re-introduce-endpoint-param-as-uri into b52b1f8 on hawkular:master.

chessbyte · 2016-07-08T14:30:16Z

lib/hawkular/base_client.rb

      strip_path = suffix_path.gsub(%r{/$}, '')
      strip_path.nil? || suffix_path = strip_path
      strip_path = suffix_path.gsub(%r{^/}, '')
      strip_path.nil? || suffix_path = strip_path
-      strip_entrypoint = entrypoint.gsub(%r{/$}, '')
-      strip_path.nil? && strip_entrypoint = entrypoint
+      strip_entrypoint = entrypoint.to_s.gsub(%r{/$}, '')


I think doing entrypoint = entrypoint.to_s above here should eliminate need to do the to_s conversions below.

yaacov · 2016-07-08T14:32:18Z

@pilhuhn is is better to do client = Hawkular::Client.new ... and then do client.metrics... then the Hawkular::Metrics::Client.new ... we use now ?
I can add it to my todo list :-)

pilhuhn · 2016-07-08T14:44:22Z

@yaacov I don't think it matters too much in your use-case. Once you need more than one sub-client, it is better to use Hawkular::Client.new and getting the sub-clients from it.

@yaacov did you have a chance to test your code with this change?

coveralls · 2016-07-08T14:45:21Z

Coverage increased (+0.2%) to 95.016% when pulling cc8a98d on pilhuhn:re-introduce-endpoint-param-as-uri into b52b1f8 on hawkular:master.

yaacov · 2016-07-08T15:26:14Z

lib/hawkular/base_client.rb

      strip_entrypoint = entrypoint.gsub(%r{/$}, '')
-      strip_path.nil? && strip_entrypoint = entrypoint
+      strip_path.nil? && strip_entrypoint = entrypoint.to_s


in line 143 entrypoint = entrypoint.to_s is converted to string, do we need the .to_s here ?

I checked in CM and this works fine :-) with and without this .to_s

you need the to_s as the entrypoint could be a URI::HTTP (what you had originally) and without it the gsub() in line 144 will error out as you have reported :)

entrypoint could be a URI::HTTP

after line 143 entrypoint is string

Now I get what you mean - sorry

~~There is a to_s in line 143 and other in line 145. I guess the later isn't needed, right?~~

np. I checked it without the to_s in line 145 and CM metrics wroks :-)

pilhuhn · 2016-07-08T15:38:40Z

Ok, when Travis is happy, I will merge it and then release the 2.2.1 version of the gem and PR it on MiQ side

coveralls · 2016-07-08T15:40:55Z

Coverage increased (+0.2%) to 95.016% when pulling 709fe46 on pilhuhn:re-introduce-endpoint-param-as-uri into b52b1f8 on hawkular:master.

pilhuhn added the bug label Jul 8, 2016

pilhuhn added this to the 2.2.1 milestone Jul 8, 2016

pilhuhn self-assigned this Jul 8, 2016

pilhuhn mentioned this pull request Jul 8, 2016

Hawkular metrics reading regression ManageIQ/manageiq#9624

Closed

yaacov mentioned this pull request Jul 8, 2016

Fix hawkular metrics reading regression ManageIQ/manageiq#9625

Closed

pilhuhn force-pushed the re-introduce-endpoint-param-as-uri branch from d8fcb84 to 09955b3 Compare July 8, 2016 10:52

chessbyte reviewed Jul 8, 2016
View reviewed changes

pilhuhn force-pushed the re-introduce-endpoint-param-as-uri branch from 09955b3 to 0cf88f2 Compare July 8, 2016 12:56

abonas reviewed Jul 8, 2016
View reviewed changes

pilhuhn force-pushed the re-introduce-endpoint-param-as-uri branch from 0cf88f2 to 6336cf3 Compare July 8, 2016 14:19

Fix regression where URIs as input for the entrypoint no longer worked.

cd40a17

pilhuhn force-pushed the re-introduce-endpoint-param-as-uri branch from 6336cf3 to cd40a17 Compare July 8, 2016 14:26

chessbyte reviewed Jul 8, 2016
View reviewed changes

yaacov reviewed Jul 8, 2016
View reviewed changes

Stringify entrypoint only once.

709fe46

pilhuhn force-pushed the re-introduce-endpoint-param-as-uri branch from cc8a98d to 709fe46 Compare July 8, 2016 15:37

pilhuhn merged commit 238dec5 into hawkular:master Jul 8, 2016

pilhuhn mentioned this pull request Jul 8, 2016

Bump Hawkular-client gem to 2.2.1 to fix a regression. ManageIQ/manageiq#9686

Merged

rubenvp8510 mentioned this pull request Jul 18, 2016

Another gsub does not work on URL issue #116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression where URIs as input for the entrypoint no longer worked. #107

Fix regression where URIs as input for the entrypoint no longer worked. #107

pilhuhn commented Jul 8, 2016

coveralls commented Jul 8, 2016 •

edited

Loading

chessbyte Jul 8, 2016

pilhuhn Jul 8, 2016

chessbyte Jul 8, 2016

coveralls commented Jul 8, 2016 •

edited

Loading

abonas Jul 8, 2016

pilhuhn Jul 8, 2016

abonas commented Jul 8, 2016

pilhuhn commented Jul 8, 2016

coveralls commented Jul 8, 2016 •

edited

Loading

chessbyte Jul 8, 2016

yaacov commented Jul 8, 2016

pilhuhn commented Jul 8, 2016

coveralls commented Jul 8, 2016 •

edited

Loading

yaacov Jul 8, 2016

pilhuhn Jul 8, 2016

yaacov Jul 8, 2016

pilhuhn Jul 8, 2016

josejulio Jul 8, 2016 •

edited

Loading

yaacov Jul 8, 2016

pilhuhn commented Jul 8, 2016

coveralls commented Jul 8, 2016 •

edited

Loading

Fix regression where URIs as input for the entrypoint no longer worked. #107

Fix regression where URIs as input for the entrypoint no longer worked. #107

Conversation

pilhuhn commented Jul 8, 2016

coveralls commented Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonas commented Jul 8, 2016

pilhuhn commented Jul 8, 2016

coveralls commented Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

yaacov commented Jul 8, 2016

pilhuhn commented Jul 8, 2016

coveralls commented Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josejulio Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pilhuhn commented Jul 8, 2016

coveralls commented Jul 8, 2016 • edited Loading

coveralls commented Jul 8, 2016 •

edited

Loading

coveralls commented Jul 8, 2016 •

edited

Loading

coveralls commented Jul 8, 2016 •

edited

Loading

coveralls commented Jul 8, 2016 •

edited

Loading

josejulio Jul 8, 2016 •

edited

Loading

coveralls commented Jul 8, 2016 •

edited

Loading