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.

Sign up for GitHub

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

Jump to bottom

Fix wrong validation error #1126

Merged
merged 1 commit into from
Apr 24, 2017
Merged

Fix wrong validation error #1126

merged 1 commit into from
Apr 24, 2017

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Apr 24, 2017

Description

Fix validation success when port is wrong

Screenshots
Bug
screenshot-localhost 3000-2017-04-24-13-57-45

FIx
screenshot-localhost 3000-2017-04-24-13-56-54

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

@yaacov
Copy link
Member Author

yaacov commented Apr 24, 2017

@miq-bot add_label compute/containers, bug

@simon3z @cben @moolitayer @himdel please review

@yaacov
Copy link
Member Author

yaacov commented Apr 24, 2017

@miq-bot add_label fine/yes

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

@cben cben mentioned this pull request Apr 24, 2017
Closed
@yaacov
Copy link
Member Author

yaacov commented Apr 24, 2017

@miq-bot add_label blocker

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

cben
cben reviewed Apr 24, 2017
View reviewed changes
Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

👍 Closed my #1120 in favor of this.
@moolitayer originally said to catch just the known OpenSSLError, but you're seeing yet another exception Errno::ECONNREFUSED, so now we all agreed we should rescue any exception.

app/controllers/mixins/ems_common_angular.rb Outdated
@@ -478,6 +478,9 @@ def get_hostname_from_routes(ems, endpoint_hash, token)
rescue KubeException, OpenSSL::SSL::SSLError => e
$log.warn("MIQ(#{controller_name}_controller-#{action_name}): get_hostname_from_routes error: #{e}")
nil
rescue StandardError
Copy link
Contributor

Choose a reason for hiding this comment

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

you can write just rescue, it default to StandardError.
and you'll need => e to log the error.

app/controllers/mixins/ems_common_angular.rb Outdated
@@ -478,6 +478,9 @@ def get_hostname_from_routes(ems, endpoint_hash, token)
rescue KubeException, OpenSSL::SSL::SSLError => e
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're keeping this separate to log "error" vs "unexpected error", consider upgrading to OpenSSL::OpenSSLError (see #1120 explanation).

@yaacov yaacov force-pushed the fix-false-validation-error branch from e580346 to 64d5985 Compare April 24, 2017 11:34
@yaacov
Copy link
Member Author

yaacov commented Apr 24, 2017

@cben Thanks 👍 fixed

@miq-bot
Copy link
Member

miq-bot commented Apr 24, 2017

Checked commit yaacov@64d5985 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 👍

moolitayer
moolitayer approved these changes Apr 24, 2017
View reviewed changes
Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

I'm working on a related fix for:
https://bugzilla.redhat.com/show_bug.cgi?id=1437138

However this solves:
https://bugzilla.redhat.com/show_bug.cgi?id=1441670
https://bugzilla.redhat.com/show_bug.cgi?id=1443520

@moolitayer
Copy link

@simon3z please review

@himdel
Copy link
Contributor

himdel commented Apr 24, 2017

Agreed, makes sense to catch any errors, not just the expected ones :).

Tested in the UI, the usual ones (timeout, refused) still work as expected.

@himdel himdel merged commit d63ee8f into ManageIQ:master Apr 24, 2017
@himdel himdel self-assigned this Apr 24, 2017
@himdel himdel added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 24, 2017
@cben cben mentioned this pull request Apr 24, 2017
Closed
simaishi pushed a commit that referenced this pull request Apr 24, 2017
@himdel @simaishi
Merge pull request #1126 from yaacov/fix-false-validation-error
a44e911 
Fix wrong validation error
(cherry picked from commit d63ee8f)

https://bugzilla.redhat.com/show_bug.cgi?id=1445002
https://bugzilla.redhat.com/show_bug.cgi?id=1441670
@cben
Copy link
Contributor

cben commented Apr 24, 2017

@simaishi We'd like this backported for tomorrow build (BZ 1441670 is 5.8 blocker+ clone of BZ 1436221)

@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit a44e91167f2f44332649a3a55f68f99e194c5371
Author: Martin Hradil <[email protected]>
Date:   Mon Apr 24 14:25:55 2017 +0000

    Merge pull request #1126 from yaacov/fix-false-validation-error
    
    Fix wrong validation error
    (cherry picked from commit d63ee8fe6eb6907fbf57202029248bfba64704f5)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1445002
    https://bugzilla.redhat.com/show_bug.cgi?id=1441670

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moolitayer moolitayer moolitayer approved these changes

@cben cben cben approved these changes

Assignees

@himdel himdel

Labels
blocker bug compute/containers fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yaacov @miq-bot @moolitayer @himdel @cben @simaishi