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

Only store the hostname if the hostname is valid #16913

Merged
merged 1 commit into from
Jan 31, 2018
Merged

Only store the hostname if the hostname is valid #16913

merged 1 commit into from
Jan 31, 2018

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Jan 30, 2018

@bdunne bdunne requested a review from carbonin January 30, 2018 19:07
carbonin
carbonin suggested changes Jan 31, 2018
View reviewed changes
Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

Given the comment on the validation, it might be better to fix this in two separate steps.

For this bug fix (which can't rely on a migration for backport reasons) I think we should fix the issue closer to where it is happening. So check for hostname validity in embedded ansible, central admin and wherever else we use it.

Then in a separate effort, only meant for master, we can add a migration and a validation to fix the issue in the table.

app/models/miq_server.rb Outdated
@@ -33,6 +33,8 @@ class MiqServer < ApplicationRecord

scope :with_zone_id, ->(zone_id) { where(:zone_id => zone_id) }

validates :hostname, :allow_nil => true, :format => {:with => MoreCoreExtensions::StringFormats::RE_HOSTNAME, :multiline => true}
Copy link
Member

Choose a reason for hiding this comment

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

I think this will fail save calls for unrelated changes if we have a record in the database with an invalid hostname already.

I would rather have a migration that removes invalid hostnames from the miq_servers table.

@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2018

Checked commit bdunne@287f25d with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

carbonin
carbonin approved these changes Jan 31, 2018
View reviewed changes
Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Can you also create a migration to remove invalid hostnames (only for master, of course).

@carbonin carbonin self-assigned this Jan 31, 2018
@carbonin carbonin merged commit 27e646c into ManageIQ:master Jan 31, 2018
@carbonin carbonin added this to the Sprint 79 Ending Feb 12, 2018 milestone Jan 31, 2018
@bdunne bdunne deleted the server_hostname branch January 31, 2018 16:23
@bdunne bdunne mentioned this pull request Feb 1, 2018
Merged
@bdunne bdunne added the fine/yes label Feb 7, 2018
simaishi pushed a commit that referenced this pull request Mar 1, 2018
@carbonin @simaishi
Merge pull request #16913 from bdunne/server_hostname
d5131ce 
Only store the hostname if the hostname is valid
(cherry picked from commit 27e646c)

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

simaishi commented Mar 1, 2018

Fine backport details:

$ git log -1
commit d5131ce50029ffd3a02d12f4fb3e0b844157a031
Author: Nick Carboni <[email protected]>
Date:   Wed Jan 31 11:22:11 2018 -0500

    Merge pull request #16913 from bdunne/server_hostname
    
    Only store the hostname if the hostname is valid
    (cherry picked from commit 27e646cb092817c4112c6b5d1d4510b5091a51a3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1550732

simaishi pushed a commit that referenced this pull request Mar 6, 2018
@carbonin @simaishi
Merge pull request #16913 from bdunne/server_hostname
f8524f2 
Only store the hostname if the hostname is valid
(cherry picked from commit 27e646c)

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

simaishi commented Mar 6, 2018

Gaprindashvili backport details:

$ git log -1
commit f8524f2edcb2974a9c2ee47661aafb4a154dcd69
Author: Nick Carboni <[email protected]>
Date:   Wed Jan 31 11:22:11 2018 -0500

    Merge pull request #16913 from bdunne/server_hostname
    
    Only store the hostname if the hostname is valid
    (cherry picked from commit 27e646cb092817c4112c6b5d1d4510b5091a51a3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1550730

simaishi added a commit to simaishi/manageiq that referenced this pull request Apr 13, 2018
@simaishi
Bump more_core_extensions to 3.5
3bc169b 
Needed for ManageIQ#16913
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1550732
@simaishi simaishi mentioned this pull request Apr 13, 2018
Merged
d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
@carbonin @d-m-u
Merge pull request ManageIQ#16913 from bdunne/server_hostname
358452c 
Only store the hostname if the hostname is valid
(cherry picked from commit 27e646c)

https://bugzilla.redhat.com/show_bug.cgi?id=1550732
d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
@simaishi @d-m-u
Bump more_core_extensions to 3.5
f7f0f91 
Needed for ManageIQ#16913
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1550732
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbonin carbonin carbonin approved these changes

Assignees

@carbonin carbonin

Labels
core fine/backported gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bdunne @miq-bot @simaishi @carbonin