Create cert_store if it's not created before validation #101

aljesusg · 2017-11-07T14:34:08Z

This fix the cert store if it's not created before call the validation method, create the endpoint and assign the certificate_authority

Related with : #100
Related with: UI ManageIQ/manageiq-ui-classic#2577
Related with: UI ManageIQ/manageiq-ui-classic#2660

cc @tumido , @israel-hdez

tumido · 2017-11-07T14:47:57Z

@aljesusg Please link the dependent/related PRs in the descriptions.

aljesusg · 2017-11-07T14:52:45Z

@miq-bot assign @abonas

aljesusg · 2017-11-07T14:54:44Z

Tested with:

ManageIQ/manageiq-ui-classic#2577
#100
#101

and revert

ManageIQ/manageiq-ui-classic#2643

and it's working with hawkular server, ssl and CA

tumido · 2017-11-07T15:06:07Z

@israel-hdez please review

tumido · 2017-11-07T15:11:37Z

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

@@ -102,7 +102,11 @@ def self.raw_connect(host, port, username, password, security_protocol, cert_sto
      options = {
        :tenant         => 'hawkular',
        :verify_ssl     => verify_ssl_mode(security_protocol),
-        :ssl_cert_store => cert_store
+        :ssl_cert_store => if cert_store.kind_of?(String)


Functional-wise I'm glad the change is that simple, though stylistically I would prefer a bit different way, something like:

if cert_store.kind_of?(String) cert_store = Endpoint.new(:certificate_authority => cert_store).ssl_cert_store options = { ... :ssl_cert_store => cert_store ... }

I prefer cert_store.kind_of?(String) ? Endpoint.new(:certificate_authority => cert_store).ssl_cert_store : cert_store ^^ Vote community !!!

Ternary operators before if

I like the ternary operator, but I don't like that the line will become loooong. So, I would vote for @tumido suggestion to have a balance between style and readability.

@tumido wins...I'll change the code ^^ thanks

abonas · 2017-11-07T18:40:51Z

is this ready for merge? or does it have to be merged with the other related PRs?
should/can we have this in gaprindashvili?

tumido

Please adjust it as we agreed on above.

tumido · 2017-11-07T19:34:00Z

@abonas, not yet, there's a change suggestion which was promised to be adjusted by @aljesusg.

tumido · 2017-11-07T19:37:45Z

@abonas, it should be merged together with the other PRs.

The gaprindashvili thing - I'm not sure. Is there a plan to have the new queue validation available for Hawkular in this release @agrare? If yes, it needs to land there, if not it doesn't matter.

agrare · 2017-11-07T19:40:18Z

Is there a plan to have the new queue validation available for Hawkular in this release @agrare? If yes, it needs to land there, if not it doesn't matter.

Not that I know of but it could always happen. I'd recommend putting a WIP PR in to ui-classic that adds it to the list of ems_types that we do validate as an umbrella PR that can link to all the dependencies. That way if we do backport that we make sure to include everything it depends on.

tumido · 2017-11-07T20:19:30Z

@agrare, good idea. Here it is ;) ManageIQ/manageiq-ui-classic#2660

agrare · 2017-11-07T20:19:45Z

❤️ thanks @tumido

aljesusg · 2017-11-08T08:32:08Z

Sorry @abonas I forgot update this PR ^^ is ready now, review @tumido . Thanks

cfcosta · 2017-11-09T13:32:48Z

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

@@ -99,6 +99,9 @@ def self.raw_connect(host, port, username, password, security_protocol, cert_sto
        :username => username,
        :password => password
      }
+      if cert_store.kind_of?(String)


Please add line breaks before and after the block.

Also, is there a way this can be tested? Since it broke something, I think we should have regression tests for it.

Also, what do you think on moving this to Endpoint and have it default to cert_store if provided? I prefer to have it there, so we could remove conditionals on foreign data.

@cfcosta linre breaks throw a rubocop error :(

It's not the line break that throws the rubocop errors, it's the whitespaces in the blank line :)

I was cleaning whitespaces and still got error It was confused ^^Fixed !!!

abonas · 2017-12-03T08:57:53Z

Hi, what's the status of this PR? I see a related PR was merged a few days ago, but here some review changes were requested?

aljesusg · 2017-12-03T09:10:58Z

It was pending from the PR merged yesterday and make this changes but I was thinking about make the move of the function to endpoint or not like @cfcosta requested I am gonna make the changes today and add a test for merge this but I think that is not a good idea to move this to the endpoint because this is only happening to this provider.

abonas · 2017-12-03T11:20:53Z

It was pending from the PR merged yesterday and make this changes but I was thinking about make the move of the function to endpoint or not like @cfcosta requested I am gonna make the changes today and add a test for merge this but I think that is not a good idea to move this to the endpoint because this is only happening to this provider.

it can wait for tomorrow, and it sounds like there is some more discussion needed if certain things sound like not a good idea to you :)

aljesusg · 2017-12-03T11:23:00Z

I think so, maybe we can merge it after me changes and after we can discuss about move it to Endpoint , What do you think @cfcosta?

tumido · 2017-12-04T10:03:50Z

@aljesusg I agree with @cfcosta, it makes a much more sense to have this handled on the Endpoint part and I would suggest to change this PR to do so instead of merging and fixing later... This PR is not intended for gaprindashvili so we have plenty of time to do it right. Also just a reminder this PR belongs under changes in ManageIQ/manageiq-ui-classic#2660 (please add the link into the description).

agrare · 2017-12-04T13:50:20Z

The gaprindashvili thing - I'm not sure. Is there a plan to have the new queue validation available for Hawkular in this release

Not necessary, and I'd argue stability is more important at this point.

aljesusg · 2017-12-04T14:13:42Z

I updated the description @tumido, ok so I'll do the PR for the Endpoint like @cfcosta requested. Thanks

abonas · 2017-12-04T14:24:48Z

@agrare yes, I agree, I asked the gaprindashvili question a month ago. it has been marked as "no" for quite some time now.

israel-hdez · 2017-12-04T20:49:51Z

@aljesusg I agree with @cfcosta, it makes a much more sense to have this handled on the Endpoint part and I would suggest to change this PR to do so instead of merging and fixing later...

I don't agree "too much"... Even when I suggested this code to fix the related issue, Endpoint is being used as a helper and an instance is created just to re-use ssl_cert_store method. Perhaps the code to create certificate stores should be extracted to a module under lib/ in core-repo. But given that Endpoint is part if the core repo, you could try to place a PR and see if the core team accepts.

aljesusg · 2017-12-04T21:36:25Z

Yes...In my experience, they don't merge in core something for a specific provider but we can try it.

aljesusg · 2017-12-06T11:20:42Z

So @cfcosta, @tumido, @israel-hdez What should I do? I can try to move this to the Endpoint or me can merge this.

israel-hdez · 2017-12-06T17:19:59Z

@aljesusg Since Endpoint is in core repo, my vote is to merge now while you check if core team accepts you PR. If they do, just place another PR here. If not... well, we've already made progress.
Just fix the rubocop warning ;)

miq-bot · 2017-12-06T21:19:31Z

Checked commit aljesusg@4fc514b 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. 👍

aljesusg · 2017-12-06T21:20:18Z

@israel-hdez is ready now. Thanks

miq-bot assigned abonas Nov 7, 2017

This was referenced Nov 7, 2017

Add optional validate param to raw_connect #100

Closed

Fix credential validation in Hawkular provider about number of arguments ManageIQ/manageiq-ui-classic#2577

Merged

tumido mentioned this pull request Nov 7, 2017

Queue authentication check only for ems_cloud and ems_infra providers. ManageIQ/manageiq-ui-classic#2643

Merged

tumido reviewed Nov 7, 2017

View reviewed changes

israel-hdez approved these changes Nov 7, 2017

View reviewed changes

abonas added the bug label Nov 7, 2017

tumido suggested changes Nov 7, 2017

View reviewed changes

tumido mentioned this pull request Nov 7, 2017

Enable queued credentials validation for middleware ManageIQ/manageiq-ui-classic#2660

Closed

aljesusg force-pushed the cert_store_fix branch from c8ff15b to 95ad312 Compare November 8, 2017 08:31

tumido approved these changes Nov 8, 2017

View reviewed changes

cfcosta suggested changes Nov 9, 2017

View reviewed changes

abonas added the gaprindashvili/no label Nov 9, 2017

aljesusg force-pushed the cert_store_fix branch from 95ad312 to 066af83 Compare December 5, 2017 16:10

aljesusg force-pushed the cert_store_fix branch from 066af83 to 12c24bd Compare December 6, 2017 21:14

Create cert_store if it's not created before validation

4fc514b

aljesusg force-pushed the cert_store_fix branch from 12c24bd to 4fc514b Compare December 6, 2017 21:19

israel-hdez merged commit 458b461 into ManageIQ:master Dec 7, 2017

israel-hdez added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 7, 2017

israel-hdez added enhancement technical debt and removed bug enhancement labels Dec 7, 2017

Create cert_store if it's not created before validation #101

Create cert_store if it's not created before validation #101

Conversation

aljesusg commented Nov 7, 2017 • edited Loading

tumido commented Nov 7, 2017

aljesusg commented Nov 7, 2017

aljesusg commented Nov 7, 2017

tumido commented Nov 7, 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

abonas commented Nov 7, 2017

tumido left a comment

Choose a reason for hiding this comment

tumido commented Nov 7, 2017

tumido commented Nov 7, 2017

agrare commented Nov 7, 2017

tumido commented Nov 7, 2017

agrare commented Nov 7, 2017

aljesusg commented Nov 8, 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

abonas commented Dec 3, 2017

aljesusg commented Dec 3, 2017

abonas commented Dec 3, 2017

aljesusg commented Dec 3, 2017

tumido commented Dec 4, 2017

agrare commented Dec 4, 2017

aljesusg commented Dec 4, 2017

abonas commented Dec 4, 2017

israel-hdez commented Dec 4, 2017

aljesusg commented Dec 4, 2017

aljesusg commented Dec 6, 2017

israel-hdez commented Dec 6, 2017

miq-bot commented Dec 6, 2017

aljesusg commented Dec 6, 2017

aljesusg commented Nov 7, 2017 •

edited

Loading