Stop using deprecated names of hawkular-client gem #14543

israel-hdez · 2017-03-28T17:32:47Z

Version 2.9.0 of hawkular-client has deprecated some names. This is
replacing the deprecated names with the current ones.

NOTE: This must be merged after ManageIQ/manageiq-gems-pending#102

@miq-bot add_labels providers/hawkular

BACKPORT UPDATE: https://bugzilla.redhat.com/show_bug.cgi?id=1446286

Version 2.9.0 of hawkular-client has deprecated some names. This is replacing the deprecated names with the current ones.

miq-bot · 2017-03-28T17:42:24Z

Checked commit israel-hdez@71a7d66 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

abonas · 2017-03-29T08:43:48Z

@chessbyte what is the syntax for me to assign/reassign a reviewer?
Also, I am concerned on our ability to sync well on changes like this where a gem is bumped and some changes are required to be done due to that, but the gem bump must be in a separate pull request and in another repo.
Can can this be reviewed, tested and merged smoothly?

abonas · 2017-03-29T08:44:51Z

@simon3z @yaacov @moolitayer please check if this affects containers provider as well

yaacov · 2017-03-29T08:49:34Z

@abonas thanks 👍 , the metrics client works with this changes.

moolitayer · 2017-03-29T09:36:50Z

@abonas In case this helps:

assigning a reviewer:
https://github.com/ManageIQ/miq_bot#requested-tasks

However the user needs to be in the reviewers list per repo and I'm not sure who handles that

As for dependencies, what I would do in a case like this to be on the safe side:
Mark this PR as WIP (both label and title) until the dependent PR merges

Of curse that is just my opinion and not an official guideline

moolitayer · 2017-03-29T09:47:15Z

app/models/manageiq/providers/hawkular/middleware_manager.rb

@@ -279,7 +279,7 @@ def self.raw_alerts_connect(hostname, port, username, password)
 :username => username,
 :password => password
 }
- ::Hawkular::Alerts::AlertsClient.new(url, credentials)
+ ::Hawkular::Alerts::Client.new(url, credentials)


Can you have a problem in middleware since you are not setting the tenant explicitly?

I'm referring to:
https://github.com/hawkular/hawkular-client-ruby/blob/master/api_breaking_changes.rdoc#200

good point, @Jiri-Kremser and @lucasponce can you please reply?

If the code worked before, this change shouldn't break it. Nor the version bump to 2.9.0 @lucasponce it works because alerts don't enforce the hawkular tenant header, right?

@Jiri-Kremser yes, the Alerts client expects the tenant from the options, the default 'hawkular' tenant used by Middleware is set in the upper raw_client as far as I remember. Inside the Alerts rb there is not tenant manipulation.

@lucasponce I'm not sure I'm seeing it set:

manageiq/app/models/manageiq/providers/hawkular/middleware_manager.rb

Line 275 in b7933dc

def self.raw_alerts_connect(hostname, port, username, password)

ok, so the alerts server side does require the header being set:

$curl -s -u jdoe:password 'http://localhost:8080/hawkular/alerts' {"errorMsg":"The HTTP header Hawkular-Tenant has to be provided."}

So now I don't understand how this could have worked before.

I think that the AlertsClient is generated from the with_provider_connection which should invoke the connect()
https://github.com/israel-hdez/manageiq/blob/71a7d668b9b5a88aec3e460efa46949d2d450197/app/models/manageiq/providers/hawkular/middleware_manager.rb#L197
and invoke the raw_connect with the default tenant.
I did a grep and I can't see where raw_alerts_connect is invoked, perhaps it is on a separate PR just for the Datawarehouse use cases ?

I also grep'ed managui-ui-classic repo and found no references to it.

Anyway... The code using raw_alerts_connect will fail with an exception because we have this: https://github.com/hawkular/hawkular-client-ruby/blob/master/lib/hawkular/base_client.rb#L32

I think it's dead code and I can safely remove it.

abonas · 2017-03-29T11:11:14Z

assigning a reviewer:
https://github.com/ManageIQ/miq_bot#requested-tasks

However the user needs to be in the reviewers list per repo and I'm not sure who handles that

@moolitayer thanks. I think that we are talking about different things.
There's a reviewer field (one or more) and an assignee field. See in this PR in the top right corner - there are both reviewers and an assignee.
Assigning an assignee is possible via the assign keyword. However I am l looking for a way to add/change the reviewer.

moolitayer · 2017-03-29T11:29:32Z

Ahh right @abonas
So the reviewers filed was added at some point by github. I don't know if a non administrator can request review ATM. maybe @chrisarcand knows.

Testing something:
@yaacov please review

abonas · 2017-03-30T11:00:48Z

ok so where do we stand with this PR? all reviewers acked?

jkremser

lgtm

yaacov · 2017-03-30T11:38:12Z

all reviewers acked?

I'm ok with the containers metrics side.

lucasponce · 2017-03-30T12:10:54Z

lgtm but travis is showing some errors related. So perhaps good to take a look to check if are derivated to the changes.

abonas · 2017-03-30T12:16:12Z

lgtm but travis is showing some errors related. So perhaps good to take a look to check if are derivated to the changes.

I believe the tests fail as this patch actually needs the gem version to be bumped (which is done in another PR in another repo but not yet merged as they must be merged at the same time)

israel-hdez · 2017-03-30T15:11:08Z

lgtm but travis is showing some errors related. So perhaps good to take a look to check if are derivated to the changes.

In travis log:
bundler: failed to load command: rspec (/home/travis/build/ManageIQ/manageiq/vendor/bundle/ruby/2.3.0/bin/rspec) NameError: uninitialized constant Hawkular::ClientUtils

And that's because of the gem version, as @abonas said.

israel-hdez · 2017-04-27T14:53:23Z

@miq-bot add_label fine/yes, blocker

Please see ManageIQ/manageiq-gems-pending#138 (comment) for backport notes.

…_constants Stop using deprecated names of hawkular-client gem (cherry picked from commit f15a504) https://bugzilla.redhat.com/show_bug.cgi?id=1446329

simaishi · 2017-04-27T20:26:58Z

Fine backport details:

$ git log -1
commit c02266a9ebb932a42d84be69545eed3fe343c268
Author: Oleg Barenboim <chessbyte@gmail.com>
Date:   Thu Mar 30 11:29:35 2017 -0400

    Merge pull request #14543 from israel-hdez/remove_usage_of_deprecated_constants
    
    Stop using deprecated names of hawkular-client gem
    (cherry picked from commit f15a504cb08f09cb82ce43cd5664253690af75a3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1446329

chrisarcand · 2017-05-01T14:01:21Z

@moolitayer @abonas Correct, reviewers are different from assignees. Currently the bot only deals with assignees (reviewers are a relatively new Github feature).

Stop using deprecated names of hawkular-client gem.

71a7d66

Version 2.9.0 of hawkular-client has deprecated some names. This is replacing the deprecated names with the current ones.

miq-bot added the providers/hawkular label Mar 28, 2017

chessbyte requested a review from abonas March 28, 2017 17:36

chessbyte self-assigned this Mar 28, 2017

moolitayer reviewed Mar 29, 2017

View reviewed changes

jkremser approved these changes Mar 30, 2017

View reviewed changes

chessbyte merged commit f15a504 into ManageIQ:master Mar 30, 2017

chessbyte added this to the Sprint 58 Ending Apr 10, 2017 milestone Mar 30, 2017

chessbyte added the enhancement label Mar 30, 2017

israel-hdez deleted the remove_usage_of_deprecated_constants branch March 31, 2017 18:03

abonas mentioned this pull request Apr 2, 2017

[RFE] PRs should support add_reviewer action ManageIQ/miq_bot#337

Closed

abonas mentioned this pull request Apr 24, 2017

[FINE] Fix to operation callbacks not being called correctly. #14851

Closed

israel-hdez mentioned this pull request Apr 27, 2017

Bump version of hawkular-client gem to 3.0.1 ManageIQ/manageiq-gems-pending#138

Merged

miq-bot added blocker fine/yes labels Apr 27, 2017

simaishi added fine/backported and removed fine/yes labels Apr 27, 2017

abonas mentioned this pull request Jun 5, 2017

bin/rake evm:status: NameError: uninitialized constant Hawkular::ClientUtils #15267

Closed

chessbyte added providers/middleware and removed providers/hawkular labels Nov 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using deprecated names of hawkular-client gem #14543

Stop using deprecated names of hawkular-client gem #14543

israel-hdez commented Mar 28, 2017 •

edited

Loading

miq-bot commented Mar 28, 2017

abonas commented Mar 29, 2017 •

edited

Loading

abonas commented Mar 29, 2017

yaacov commented Mar 29, 2017

moolitayer commented Mar 29, 2017

moolitayer Mar 29, 2017

abonas Mar 29, 2017

jkremser Mar 29, 2017

lucasponce Mar 29, 2017

moolitayer Mar 29, 2017

jkremser Mar 29, 2017

lucasponce Mar 29, 2017

israel-hdez Mar 29, 2017

abonas commented Mar 29, 2017

moolitayer commented Mar 29, 2017

abonas commented Mar 30, 2017

jkremser left a comment

yaacov commented Mar 30, 2017

lucasponce commented Mar 30, 2017

abonas commented Mar 30, 2017

israel-hdez commented Mar 30, 2017

israel-hdez commented Apr 27, 2017

simaishi commented Apr 27, 2017

chrisarcand commented May 1, 2017

Stop using deprecated names of hawkular-client gem #14543

Stop using deprecated names of hawkular-client gem #14543

Conversation

israel-hdez commented Mar 28, 2017 • edited Loading

miq-bot commented Mar 28, 2017

abonas commented Mar 29, 2017 • edited Loading

abonas commented Mar 29, 2017

yaacov commented Mar 29, 2017

moolitayer commented Mar 29, 2017

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

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

abonas commented Mar 29, 2017

moolitayer commented Mar 29, 2017

abonas commented Mar 30, 2017

jkremser left a comment

Choose a reason for hiding this comment

yaacov commented Mar 30, 2017

lucasponce commented Mar 30, 2017

abonas commented Mar 30, 2017

israel-hdez commented Mar 30, 2017

israel-hdez commented Apr 27, 2017

simaishi commented Apr 27, 2017

chrisarcand commented May 1, 2017

israel-hdez commented Mar 28, 2017 •

edited

Loading

abonas commented Mar 29, 2017 •

edited

Loading