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

Pass correct credential parameters depending on the credential type #3913

Conversation

AparnaKarve
Copy link
Contributor

@AparnaKarve AparnaKarve commented May 8, 2018

For VMWare Infra VMRC Console, correct credential parameters were not being passed while doing the validation on the queue.

Other than the above case that was reported in the BZ, the PR also fixes the following cases -
OpenStack cloud AMQP
OpenStack infra AMQP
VMWare cloud AMQP

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

@AparnaKarve
Copy link
Contributor Author

/cc @h-kataria

@miq-bot
Copy link
Member

miq-bot commented May 8, 2018

Checked commits AparnaKarve/manageiq-ui-classic@1b10a16~...0abf186 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@h-kataria
Copy link
Contributor

looks good.

@h-kataria h-kataria self-assigned this May 10, 2018
@h-kataria h-kataria added this to the Sprint 86 Ending May 21, 2018 milestone May 10, 2018
@h-kataria h-kataria merged commit e16363f into ManageIQ:master May 10, 2018
@AparnaKarve AparnaKarve deleted the bz1574000_fix_creds_for_vmware_console_tab branch May 10, 2018 14:39
simaishi pushed a commit that referenced this pull request Jul 19, 2018
…are_console_tab

Pass correct credential parameters depending on the credential type
(cherry picked from commit e16363f)

https://bugzilla.redhat.com/show_bug.cgi?id=1591497
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit c305329943b75d1abb79be28495ccba12e53de3e
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Thu May 10 10:35:15 2018 -0400

    Merge pull request #3913 from AparnaKarve/bz1574000_fix_creds_for_vmware_console_tab
    
    Pass correct credential parameters depending on the credential type
    (cherry picked from commit e16363f5ab1f888b1aa43985536537faef01021c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1591497

@simaishi
Copy link
Contributor

simaishi commented Jul 19, 2018

@AparnaKarve Please take a look at Travis failure in Gaprindashvili. This is the one I was asking you on gitter about spec possibly needing a change in G-branch because of missing use_broker.

https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/405802359

@AparnaKarve
Copy link
Contributor Author

Thanks @simaishi. I had verified this locally and it did not fail for some reason - but will look into it now.

@AparnaKarve
Copy link
Contributor Author

@simaishi I can make a PR to fix the spec but was wondering why the use_broker changes did not get carried over here?
https://github.com/ManageIQ/manageiq-ui-classic/blob/gaprindashvili/app/controllers/mixins/ems_common_angular.rb#L161

@simaishi
Copy link
Contributor

@AparnaKarve That's because the hash didn't get modified in this PR. Before the PR, the line in 'master' branch was:

[{:pass => password, :user => user, :ip => params[:default_hostname], :use_broker => false}]

This PR changed to:

connect_opts = [{:pass => password, :user => user, :ip => params[:default_hostname], :use_broker => false}] if params[:cred_type] == "default"

So in this PR, you've added a few things before/after the hash without changing the hash contents, and that's what I've done when cherry-picking too.

use_broker was added in #2770. So if I add use_broker during backport of this PR, that will mean I'm partially backporting #2770 as well and that would be wrong especially because that PR is marked as gaprindashvili/no.

@AparnaKarve
Copy link
Contributor Author

So in this PR, you've added a few things before/after the hash without changing the hash contents, and that's what I've done when cherry-picking too.

Nice!
Thanks for the explanation @simaishi !!

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.

None yet

4 participants