Adding the user_agent when is creating the connection #96

CharlleDaniel · 2017-10-30T12:39:52Z

This PR is able to:

Add the User-Agent when is creating the connection.

CharlleDaniel · 2017-10-30T13:51:01Z

AndreyMenezes · 2017-10-31T19:08:04Z

app/models/manageiq/providers/lenovo/manager_mixin.rb

@@ -20,10 +20,11 @@ def connect(options = {})
    host       = options[:host] || address
    port       = options[:port] || self.port
    auth_type  = AUTH_TYPES[options[:auth_type]]
+    user_agent_label = "ManageIQ/#{MiqServer.first.version}"


@rodneyhbrown7 @ManasaRaoK using cloudforms MiqServer.first.version will show the version of the cloudforms? Doesn't make sense to me expose the branch in this label. Do you have any particular request for this?

It doesn't make any sense for me...

The goal is to be able to identify what client was communicating with LXCA. The version is just a way to identify what version of the client was used.

Oooh, now it's clear for me! Thanks @rodneyhbrown7 :D

So, @AndreyMenezes, @walteraa and @rodneyhbrown7 Could you review this PR, please? 👍

@rodneyhbrown7 I understand this, but for ManageIQ this call will get fine-3, fine-4, etc, depends on the branch of manageIQ. In cloudforms this call will return 4.5, 4.6, etc? Just to understand the expectancy.

We should change the user_agent_label to "CFME/#{MiqServer.first.version}"

rodneyhbrown7 · 2017-11-01T17:35:03Z

@miq-bot add_label gaprindashvili/yes

walteraa · 2017-11-01T18:21:16Z

app/models/manageiq/providers/lenovo/manager_mixin.rb

@@ -20,10 +20,11 @@ def connect(options = {})
    host       = options[:host] || address
    port       = options[:port] || self.port
    auth_type  = AUTH_TYPES[options[:auth_type]]
+    user_agent_label = "ManageIQ/#{MiqServer.first.version}"


Once it always will be the same value and it isn't defined through options, I recommend it should be a constant.

Ok, I can accept the constant. But it shouldn't be the first.version, because when the build change MIQ create another entry to this table. So, it should be last.version.

I disagree with you @AndreyMenezes. This value is coming from a file named VERSION which is in the ManageIQ root, the point is: how to know what ManageIQ is instantiating the client, it could be the first, the last or another between both of them.

It is coming from this stack:

manageiq/app/models/miq_server.rb

class MiqServer < ApplicationRecord (...) self.version = Vmdb::Appliance.VERSION end (...)

manageiq/lib/vmdb/appliance.rb

module Vmdb module Appliance def self.VERSION @EVM_VERSION ||= File.read(File.join(File.expand_path(Rails.root), "VERSION")).strip end (...) end end

manageiq/lib/vmdb/appliance.rb

@rodneyhbrown7 @CharlleDaniel is there a way to know what server is instantiating the client and then perform a kind of MiqServer.find and then getting the .version variable?

@walteraa I got it! the find should be perfect! I'm thinking we only can have the last build server, but I'm wrong!

walteraa · 2017-11-01T18:57:59Z

spec/models/manageiq/providers/lenovo/physical_infra_manager_spec.rb

@@ -4,6 +4,7 @@
 describe ManageIQ::Providers::Lenovo::PhysicalInfraManager do
  before :all do
    @auth = { :user => 'admin', :pass => 'smartvm', :host => 'localhost', :port => '3000' }
+    FactoryGirl.create(:miq_server, :version=>'master')
  end



Isn't interesting add a test to check the :user_agent_label, I mean, is it possible?

We should make sure that the :user_agent_label matches the :version added by FactoryGirl. Is that possible Charlle?

walteraa · 2017-11-06T13:58:32Z

LGTM

walteraa · 2017-11-06T20:34:09Z

@miq-bot assign @juliancheal

rodneyhbrown7

Charlle the spec test for this seems to have gone away? Is there anything we can do to do some spec testing here?

rodneyhbrown7 · 2017-11-03T14:22:28Z

spec/models/manageiq/providers/lenovo/physical_infra_manager_spec.rb

@@ -4,6 +4,7 @@
 describe ManageIQ::Providers::Lenovo::PhysicalInfraManager do
  before :all do
    @auth = { :user => 'admin', :pass => 'smartvm', :host => 'localhost', :port => '3000' }
+    FactoryGirl.create(:miq_server, :version=>'master')
  end



We should make sure that the :user_agent_label matches the :version added by FactoryGirl. Is that possible Charlle?

Fryguy · 2017-11-07T15:34:08Z

app/models/manageiq/providers/lenovo/manager_mixin.rb

@@ -8,6 +8,8 @@ module ManageIQ::Providers::Lenovo::ManagerMixin
    nil       => 'basic_auth'
  }.freeze

+  USER_AGENT = "CFME/#{MiqServer.last&.version}".freeze


Do not use the string "CFME" as part of the user agent string. CFME is a downstream Red Hat thing only, and upstream you should use ManageIQ. You can get the correct string by using I18n.t("product.name"). That being said, you probably should not access the i18n catalog at the class-level context, so I recommend putting this into a method instead. On that note, this seems like something that might be useful for every provider, so I would be OK with a class method in the core repo to get the user agent string.

Additionally, don't use MiqServer.last as you are technically then getting a random server object. Instead, if you are going for the version, prefer Vmdb::Appliance::VERSION or if you want the build number, Vmdb::Appliance::BUILD.

@Fryguy and @juliancheal thanks for help me, I will work on this.

juliancheal · 2017-11-07T16:41:29Z

I've created two PRs that we could use (once they're merged)

Added Product name to VMDB::Appliance manageiq#16409 - creates Product Name
Added User Agent to VMDB::Appliance manageiq#16410 - creates a User Agent

It would be called by Vmdb::Appliance.USER_AGENT

gberginc · 2017-11-08T04:48:17Z

@juliancheal should you label them for the G-release because of this PR?

miq-bot · 2017-11-08T06:18:00Z

This pull request is not mergeable. Please rebase and repush.

juliancheal · 2017-11-07T13:30:39Z

app/models/manageiq/providers/lenovo/manager_mixin.rb

@@ -8,6 +8,8 @@ module ManageIQ::Providers::Lenovo::ManagerMixin
    nil       => 'basic_auth'
  }.freeze

+  USER_AGENT = "CFME/#{MiqServer.last&.version}".freeze


This shouldn't be CFME, we should change this to a variable configurable from the settings. I think there might be something already like that.

maybe build would be ok to use?

I've posted a question in gitter.

juliancheal · 2017-11-27T16:59:04Z

ManageIQ/manageiq#16410 is merged now. You can use Vmdb::Appliance.USER_AGENT

Vmdb::Appliance.USER_AGENT was called with ::USER_AGENT not .USER_AGENT

miq-bot · 2017-11-29T16:23:14Z

Checked commits CharlleDaniel/manageiq-providers-lenovo@940987a~...8f00e70 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

juliancheal · 2017-11-29T16:41:57Z

My bad, waiting on ManageIQ/manageiq#16556 to be merged. THEN we'll be good to go.

CharlleDaniel force-pushed the add_agent_label branch 3 times, most recently from b02ea0d to 5f1df73 Compare October 30, 2017 13:37

CharlleDaniel force-pushed the add_agent_label branch 3 times, most recently from 704290a to 680d30e Compare October 30, 2017 20:39

CharlleDaniel changed the title ~~Added the user_agent when is creating the connection~~ Adding the user_agent when is creating the connection Oct 30, 2017

CharlleDaniel force-pushed the add_agent_label branch 2 times, most recently from 0139819 to 3b3cbc3 Compare October 31, 2017 14:10

AndreyMenezes reviewed Oct 31, 2017

View reviewed changes

miq-bot added the gaprindashvili/yes label Nov 1, 2017

walteraa suggested changes Nov 1, 2017

View reviewed changes

CharlleDaniel force-pushed the add_agent_label branch from 3b3cbc3 to ac18ab4 Compare November 2, 2017 20:49

rodneyhbrown7 self-assigned this Nov 3, 2017

CharlleDaniel force-pushed the add_agent_label branch from ac18ab4 to 26fdd51 Compare November 6, 2017 13:42

miq-bot assigned juliancheal and unassigned rodneyhbrown7 Nov 6, 2017

rodneyhbrown7 reviewed Nov 7, 2017

View reviewed changes

Fryguy reviewed Nov 7, 2017

View reviewed changes

CharlleDaniel changed the title ~~Adding the user_agent when is creating the connection~~ [WIP]Adding the user_agent when is creating the connection Nov 7, 2017

miq-bot added the wip label Nov 7, 2017

miq-bot added the unmergeable label Nov 8, 2017

juliancheal reviewed Nov 8, 2017

View reviewed changes

juliancheal added blocker gaprindashvili/no and removed gaprindashvili/yes blocker labels Nov 27, 2017

CharlleDaniel force-pushed the add_agent_label branch 2 times, most recently from b1f155a to 6557415 Compare November 27, 2017 20:57

miq-bot removed the unmergeable label Nov 27, 2017

CharlleDaniel force-pushed the add_agent_label branch from 6557415 to 940987a Compare November 27, 2017 21:03

CharlleDaniel changed the title ~~[WIP]Adding the user_agent when is creating the connection~~ Adding the user_agent when is creating the connection Nov 27, 2017

Added the user_agent when is creating the connection

940987a

miq-bot removed the wip label Nov 27, 2017

Syntax error

8f00e70

Vmdb::Appliance.USER_AGENT was called with ::USER_AGENT not .USER_AGENT

juliancheal force-pushed the add_agent_label branch from 9419175 to 8f00e70 Compare November 29, 2017 16:15

juliancheal mentioned this pull request Nov 29, 2017

Looks like self is needed on PRODUCT_NAME and VERSION calls ManageIQ/manageiq#16556

Merged

juliancheal merged commit b3ab7f7 into ManageIQ:master Nov 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the user_agent when is creating the connection #96

Adding the user_agent when is creating the connection #96

CharlleDaniel commented Oct 30, 2017 •

edited

Loading

CharlleDaniel commented Oct 30, 2017 •

edited

Loading

AndreyMenezes Oct 31, 2017

walteraa Nov 1, 2017

rodneyhbrown7 Nov 1, 2017

walteraa Nov 1, 2017

CharlleDaniel Nov 1, 2017

AndreyMenezes Nov 1, 2017

rodneyhbrown7 Nov 2, 2017

rodneyhbrown7 commented Nov 1, 2017

walteraa Nov 1, 2017

AndreyMenezes Nov 1, 2017

walteraa Nov 1, 2017

AndreyMenezes Nov 1, 2017

walteraa Nov 1, 2017

rodneyhbrown7 Nov 3, 2017

walteraa commented Nov 6, 2017

walteraa commented Nov 6, 2017

rodneyhbrown7 left a comment

rodneyhbrown7 Nov 3, 2017

Fryguy Nov 7, 2017

CharlleDaniel Nov 7, 2017

juliancheal commented Nov 7, 2017 •

edited

Loading

gberginc commented Nov 8, 2017

miq-bot commented Nov 8, 2017

juliancheal Nov 7, 2017

juliancheal commented Nov 27, 2017 •

edited

Loading

miq-bot commented Nov 29, 2017

juliancheal commented Nov 29, 2017

Adding the user_agent when is creating the connection #96

Adding the user_agent when is creating the connection #96

Conversation

CharlleDaniel commented Oct 30, 2017 • edited Loading

CharlleDaniel commented Oct 30, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodneyhbrown7 commented Nov 1, 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

walteraa commented Nov 6, 2017

walteraa commented Nov 6, 2017

rodneyhbrown7 left a 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

juliancheal commented Nov 7, 2017 • edited Loading

gberginc commented Nov 8, 2017

miq-bot commented Nov 8, 2017

Choose a reason for hiding this comment

juliancheal commented Nov 27, 2017 • edited Loading

miq-bot commented Nov 29, 2017

juliancheal commented Nov 29, 2017

CharlleDaniel commented Oct 30, 2017 •

edited

Loading

CharlleDaniel commented Oct 30, 2017 •

edited

Loading

juliancheal commented Nov 7, 2017 •

edited

Loading

juliancheal commented Nov 27, 2017 •

edited

Loading