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 address validation for ConversionHost model #18381

Merged
merged 3 commits into from
Jan 23, 2019
Merged

Fix address validation for ConversionHost model #18381

merged 3 commits into from
Jan 23, 2019

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Jan 22, 2019 •
edited
Loading

The address validation for the ConversionHost model is busted. It mistakenly does not pass in a parameter, which causes it to blow up with an ArgumentError when called.

Inadvertently introduced in #18277

@djberg96
Copy link
Contributor Author

@miq-bot add_reviewer @agrare

@djberg96
Copy link
Contributor Author

@miq-bot add_label hammer/no

@miq-bot miq-bot requested a review from agrare January 22, 2019 20:30
@agrare agrare self-assigned this Jan 22, 2019
jrafanie
jrafanie requested changes Jan 22, 2019
View reviewed changes
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM other than the minor change to add a test case.

@miq-bot
Copy link
Member

miq-bot commented Jan 23, 2019

Checked commits https://github.com/djberg96/manageiq/compare/2534c795313441f5d6468bbeef3e0a8ca1f7ea3e~...2b15547d5bdd86637bee8fc4345c870ce769356c 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. ⭐

@djberg96
Copy link
Contributor Author

@jrafanie @agrare Ok, updated.

@jrafanie
Copy link
Member

@agrare LGTM, merge at will

@agrare agrare merged commit 45d836b into ManageIQ:master Jan 23, 2019
@agrare agrare modified the milestones: Roadmap, Sprint 104 Ending Feb 4, 2019 Feb 14, 2019
@simaishi
Copy link
Contributor

@djberg96 Is this not needed in hammer branch? #18277 was hammer/yes(now hammer/backported), so double-checking...

@djberg96
Copy link
Contributor Author

@miq-bot remove_label hammer/no

@djberg96
Copy link
Contributor Author

@miq-bot add_label hammer/yes

@djberg96
Copy link
Contributor Author

djberg96 commented Apr 23, 2019 •
edited
Loading

@simaishi updated. Can we use the same BZ as #18277 since the bug was introduced there? (https://bugzilla.redhat.com/show_bug.cgi?id=1695797)

@simaishi
Copy link
Contributor

@djberg96 yes, the same BZ is ok. Thanks!

@mturley mturley mentioned this pull request Apr 23, 2019
Merged
simaishi pushed a commit that referenced this pull request Apr 25, 2019
@agrare @simaishi
Merge pull request #18381 from djberg96/conversion_host_validations_fix
564d170 
Fix address validation for ConversionHost model

(cherry picked from commit 45d836b)

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

Hammer backport details:

$ git log -1
commit 564d170024abf429bfd394f5fa9c61572de1c5ed
Author: Adam Grare <[email protected]>
Date:   Wed Jan 23 11:19:02 2019 -0500

    Merge pull request #18381 from djberg96/conversion_host_validations_fix
    
    Fix address validation for ConversionHost model
    
    (cherry picked from commit 45d836b9a62836781bce7af50f5cb059b934b1d5)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1696437

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

@agrare agrare agrare approved these changes

@jrafanie jrafanie jrafanie approved these changes

Assignees

@agrare agrare

Labels
hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@djberg96 @miq-bot @jrafanie @simaishi @agrare