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

Add logic for checking if a middleware server is immutable #14565

Merged
merged 1 commit into from
Apr 13, 2017
Merged

Add logic for checking if a middleware server is immutable #14565

merged 1 commit into from
Apr 13, 2017

Conversation

cfcosta
Copy link

@cfcosta cfcosta commented Mar 29, 2017 •
edited
Loading

This is a companion to another commit on manageiq-ui-classic, which
moves the logic to check if a server is mutable or not from an action on
the UI to the MiddlewareServer itself. It is a refactor of manageiq-ui-classic#636, and should be merged together with ManageIQ/manageiq-ui-classic#839

@cfcosta cfcosta mentioned this pull request Mar 29, 2017
Merged
@cfcosta cfcosta force-pushed the mw-os-disable-cmds2-refactor branch from c86224b to 2db2b8e Compare March 29, 2017 20:33
@cfcosta cfcosta mentioned this pull request Mar 29, 2017
Merged
abonas
abonas reviewed Mar 30, 2017
View reviewed changes
app/models/middleware_server.rb
@@ -33,6 +37,21 @@ def evaluate_alert(alert_id, event)
end

def in_domain?
!middleware_server_group.nil?
middleware_server_group
Copy link
Member

Choose a reason for hiding this comment

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

I'm finding it hard to follow (maybe it's just me :)) - how does this return boolean?

Copy link
Author

Choose a reason for hiding this comment

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

nil == false, and everything else besides false is truthy. Predicate methods do not need to return true/false explicitly, just a truthy and a falsey value. This is common pratice in ruby.

app/models/middleware_server.rb Outdated

def immutable?
hawkular? ||
['Immutable', 'Is Immutable'].any? do |key|
Copy link
Member

Choose a reason for hiding this comment

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

where does the 'Is Immutable' property come from?

Copy link
Author

Choose a reason for hiding this comment

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

Wildfly/EAP agents present this key, while Hawkular servers use the 'Immutable' key.

Copy link
Member

Choose a reason for hiding this comment

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

please add it as a comment in code for easier understanding by anyone who reads it

Copy link
Author

Choose a reason for hiding this comment

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

done

@abonas
Copy link
Member

abonas commented Mar 30, 2017

@miq-bot assign @mtho11
@mtho11 this is related to your recent patch, please review

@miq-bot miq-bot assigned mtho11 and unassigned abonas Mar 30, 2017
@cfcosta cfcosta force-pushed the mw-os-disable-cmds2-refactor branch from 2db2b8e to 35a2146 Compare March 30, 2017 12:27
mtho11
mtho11 reviewed Mar 30, 2017
View reviewed changes
Copy link
Contributor

@mtho11 mtho11 left a comment

Choose a reason for hiding this comment

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

LGTM, I learned a few things too ;)

Just need to check on other comment with: 'Immutable' and 'Is Immutable' but its covered either way now so that would just be an optimization.

app/models/middleware_server.rb Outdated
# 'Immutable' key is used in Hawkular servers, while EAP/WildFly servers
# use 'Is Immutable' as the key on properties.
hawkular? ||
['Immutable', 'Is Immutable'].any? do |key|
Copy link
Contributor

Choose a reason for hiding this comment

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

I will confirm that we need to check for both: 'Immutable' and 'Is Immutable' . It has a bad smell (not from anything you have coded but just in general design consistency).

Copy link
Member

Choose a reason for hiding this comment

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

@mtho11 is this PR pending your check regarding the different Immutable variables?

Copy link
Contributor

Choose a reason for hiding this comment

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

@abonas let's check with @mazz first.

@mazz do we need to check for both 'Immutable' and 'Is Immutable' ?
CC: @pilhuhn in case he has input as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, confirmed second property is not necessary. I didn't think that looked right. 'Is Immutable' is an older artefact from a hawkfly build.

@mazz
Copy link

mazz commented Apr 4, 2017

Hi, I've been added to this thread but not actually involved with development.

@cfcosta cfcosta force-pushed the mw-os-disable-cmds2-refactor branch from 35a2146 to 3b91e5d Compare April 4, 2017 19:00
mtho11
mtho11 approved these changes Apr 4, 2017
View reviewed changes
Copy link
Contributor

@mtho11 mtho11 left a comment

Choose a reason for hiding this comment

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

After fixes LGTM!

@cfcosta
Copy link
Author

cfcosta commented Apr 4, 2017

@abonas seems like it is mergeable, who should I assign?

@cfcosta cfcosta force-pushed the mw-os-disable-cmds2-refactor branch from 3b91e5d to 9abda96 Compare April 4, 2017 21:15
@abonas
Copy link
Member

abonas commented Apr 5, 2017

@cfcosta the tests looks red, please have a look

@cfcosta
Copy link
Author

cfcosta commented Apr 5, 2017

@abonas the failure is related to a migration, and there's none on this PR :(

@abonas
Copy link
Member

abonas commented Apr 5, 2017

hmmm... please rekick the tests then

@cfcosta cfcosta closed this Apr 5, 2017
@cfcosta cfcosta reopened this Apr 5, 2017
@abonas
Copy link
Member

abonas commented Apr 5, 2017

@mtho11 if you're good with this PR, please reassign to miq core for merging

@mtho11
Copy link
Contributor

mtho11 commented Apr 5, 2017

@miq-bot add_label fine/yes

@mtho11
Copy link
Contributor

mtho11 commented Apr 5, 2017

LGTM
@miq-bot assign @bronaghs

@miq-bot miq-bot assigned bronaghs and unassigned mtho11 Apr 5, 2017
@abonas
Copy link
Member

abonas commented Apr 6, 2017

@bronaghs @blomquisg will be nice to have this merged before repo split
thanks

@bronaghs
Copy link

bronaghs commented Apr 6, 2017

@abonas - this PR states that it should be merged at the same time as ManageIQ/manageiq-ui-classic#839 which appears not be ready for merging.

@cfcosta
Copy link
Author

cfcosta commented Apr 6, 2017

@bronaghs it is not dependant though. This one could be merged without ManageIQ/manageiq-ui-classic#839, but not the other way around.

@bronaghs
Copy link

bronaghs commented Apr 6, 2017

@miq-bot assign @agrare

@miq-bot miq-bot assigned agrare and unassigned bronaghs Apr 6, 2017
@mtho11
Copy link
Contributor

mtho11 commented Apr 7, 2017

@cfcosta looks like this needs to be moved to new provider repo: https://github.com/ManageIQ/manageiq-providers-hawkular and new PR opened up there.

agrare
agrare reviewed Apr 10, 2017
View reviewed changes
app/models/middleware_server.rb Outdated
end

def immutable?
hawkular? || properties['Immutable'] == 'true'
Copy link
Member

Choose a reason for hiding this comment

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

This should really be implemented by the subclass instead of checking hawkular? here no? Seeing if hawkular? on a base model feels like it is breaking the pluggability (/cc @durandom )

Why not have immutable? return false on the base model and have ManageIQ::Providers::Hawkular::MiddlewareManager::MiddlewareServer#immutable? return properties['Immutable'] == 'true'

@cfcosta cfcosta force-pushed the mw-os-disable-cmds2-refactor branch from 9abda96 to cbcf23c Compare April 11, 2017 00:49
@cfcosta cfcosta mentioned this pull request Apr 11, 2017
Merged
@cfcosta cfcosta force-pushed the mw-os-disable-cmds2-refactor branch from cbcf23c to 4a43f7a Compare April 11, 2017 13:32
durandom
durandom reviewed Apr 12, 2017
View reviewed changes
app/models/middleware_deployment.rb Outdated
@@ -3,4 +3,6 @@ class MiddlewareDeployment < ApplicationRecord
belongs_to :middleware_server, :foreign_key => "server_id"
belongs_to :middleware_server_group, :foreign_key => "server_group_id", :optional => true
acts_as_miq_taggable

delegate :hawkular?, :mutable?, :in_domain?, :to => :middleware_server
Copy link
Member

Choose a reason for hiding this comment

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

where is hawkular? used?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, thanks!

Add logic for checking if a middleware server is immutable
81e2d38 
This is a companion to another commit on manageiq-ui-classic, which
moves the logic to check if a server is mutable or not from an action on
the UI to the MiddlewareServer itself.
@cfcosta cfcosta force-pushed the mw-os-disable-cmds2-refactor branch from 4a43f7a to 81e2d38 Compare April 12, 2017 18:47
@miq-bot
Copy link
Member

miq-bot commented Apr 12, 2017

Checked commit cfcosta@81e2d38 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

@abonas
Copy link
Member

abonas commented Apr 13, 2017

@agrare @durandom please merge if you have no more review comments. thank you

agrare
agrare reviewed Apr 13, 2017
View reviewed changes
app/models/middleware_server.rb
#
# Just a convenience method over #immutable.
def mutable?
!immutable?
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of this since we already have if/unless to avoid negating a conditional but I won't hold this up because of it

@agrare agrare merged commit 3619952 into ManageIQ:master Apr 13, 2017
@agrare agrare added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 13, 2017
simaishi pushed a commit that referenced this pull request Apr 13, 2017
@agrare @simaishi
Merge pull request #14565 from cfcosta/mw-os-disable-cmds2-refactor
b979a0b 
Add logic for checking if a middleware server is immutable
(cherry picked from commit 3619952)
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit b979a0b6219d7546717d6adaf9b12abe772805d2
Author: Adam Grare <[email protected]>
Date:   Thu Apr 13 09:06:28 2017 -0400

    Merge pull request #14565 from cfcosta/mw-os-disable-cmds2-refactor
    
    Add logic for checking if a middleware server is immutable
    (cherry picked from commit 36199523979022ba75aeba2732c1808d7a8e7536)

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

@agrare agrare agrare left review comments

@abonas abonas abonas left review comments

@durandom durandom durandom left review comments

@bronaghs bronaghs bronaghs approved these changes

@mtho11 mtho11 mtho11 approved these changes

Assignees

@agrare agrare

Labels
fine/backported providers/middleware refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@cfcosta @abonas @mazz @mtho11 @bronaghs @miq-bot @simaishi @durandom @agrare @chessbyte