Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix authentication of metrics credentials in RHV #13981

Merged
merged 1 commit into from
Feb 27, 2017

Conversation

borod108
Copy link

When the database name was not the default one the RHV metrics credentials
would not use the right database name during authentication and fail.

@borod108
Copy link
Author

@masayag please review.

@@ -134,7 +134,7 @@ def rhevm_metrics_connect_options(options = {})
server = options[:hostname] || metrics_hostname || hostname
username = options[:user] || authentication_userid(:metrics)
password = options[:pass] || authentication_password(:metrics)
database = options[:database]
database = options[:database] || history_database_name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it require a test ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it does, added them.

@borod108 borod108 force-pushed the bugs/metrics_database_auth branch 3 times, most recently from 449c7e8 to eee15ed Compare February 20, 2017 13:20
When the database name was not the default one the RHV metrics credentials
would not use the right database name during authentication and fail.

Removed some code that supported RHV 3.0 and needed to connect to the RHV
api to find its version, so it caused some tests to fail.
@miq-bot
Copy link
Member

miq-bot commented Feb 20, 2017

Checked commit borod108@1313a33 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🏆

@borod108 borod108 changed the title Fix authentication of metrics credentials in RHV [wip] Fix authentication of metrics credentials in RHV Feb 20, 2017
@miq-bot miq-bot added the wip label Feb 20, 2017
@borod108
Copy link
Author

borod108 commented Feb 20, 2017

@Fryguy Fryguy changed the title [wip] Fix authentication of metrics credentials in RHV [WIP] Fix authentication of metrics credentials in RHV Feb 20, 2017
@borod108 borod108 changed the title [WIP] Fix authentication of metrics credentials in RHV Fix authentication of metrics credentials in RHV Feb 23, 2017
@borod108
Copy link
Author

@miq-bot assign @durandom

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about v3? DEFAULT_HISTORY_DATABASE_NAME_3_0

Now this default is not being picked up anymore.

  • Will this require doc updates?
  • What about users having v3 and relying on the default?

@borod108
Copy link
Author

borod108 commented Feb 27, 2017

@durandom I think it should be ok since the 3_0 thing is relevant up to 3.1 version of RHV which we do not support.
Shirly found a patch that shows it was changed in 3_1: http://git.engineering.redhat.com/git/users/ydary/3.1/rhevm-dwh/.git/commit/?id=3295e6f82ecc42806f6a82db1a1bd46e3f87de4d

For >3.2 it should work fine.

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for clarifiying @borod108

@miq-bot assign @agrare
@agrare please merge

@durandom
Copy link
Member

@miq-bot add_labels providers/rhvem, bug, euwe/no

@borod108 I guess its euwe/no ?!

@miq-bot
Copy link
Member

miq-bot commented Feb 27, 2017

@durandom Cannot apply the following label because they are not recognized: providers/rhvem

@borod108
Copy link
Author

@durandom actually its a yes...

@miq-bot add_labels euwe/yes

@agrare
Copy link
Member

agrare commented Feb 27, 2017

Looks good thanks @borod108 !

@agrare agrare added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 27, 2017
@agrare agrare merged commit 1f93c45 into ManageIQ:master Feb 27, 2017
@durandom
Copy link
Member

durandom commented Mar 2, 2017

@miq-bot remove_label euwe/yes

@miq-bot miq-bot removed the euwe/yes label Mar 2, 2017
@durandom
Copy link
Member

durandom commented Mar 2, 2017

@miq-bot remove_label euwe/no
@miq-bot add_label euwe/yes

https://bugzilla.redhat.com/show_bug.cgi?id=1414077

@miq-bot miq-bot added euwe/yes and removed euwe/no labels Mar 2, 2017
simaishi pushed a commit that referenced this pull request Mar 8, 2017
@simaishi
Copy link
Contributor

simaishi commented Mar 8, 2017

Euwe backport details:

$ git log -1
commit ffed336460065ede09408e54d5ae6ecada36ef50
Author: Adam Grare <agrare@redhat.com>
Date:   Mon Feb 27 11:00:22 2017 -0500

    Merge pull request #13981 from borod108/bugs/metrics_database_auth
    
    Fix authentication of metrics credentials in RHV
    (cherry picked from commit 1f93c451219cc757374473726e9344a857ae3a14)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1429649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants