Fix defaults for immutability of MiddlewareServers

[cfcosta]

This fixes BZ https://bugzilla.redhat.com/show_bug.cgi?id=1443881 by providing a saner default for server mutability.

[abonas]

@miq-bot add_label bug, providers/hawkular

[abonas]

@miq-bot assign @Jiri-Kremser

[abonas]

@mtho11 @Jiri-Kremser please review

[jkremser]

Changing the hard-coded value from true to false will result in another issue. Running the power ops on dockerized servers. But agent doesn't allow that, so the MiQ ui will allow that but it fails. Why is the value hard-coded and not read from the properties? The idea is that all the hawkular servers running in the docker container have the immutable flag and they told it about themselves in the properties.

I don't get why there is always true or false, or is this method overriden somewhere?

[cfcosta]

It is overwritten here: https://github.com/ManageIQ/manageiq-providers-hawkular/blob/master/app/models/manageiq/providers/hawkular/middleware_manager/middleware_server.rb

As of now I'm tracking what might be the error where this class is not being instantiated on miq-core, so this change would only change the default if the provider does not have a different implementation.

[jkremser]

I see, I was doing the grep only over this repo

[abonas]

@miq-bot add_label fine/yes
for now lets wait for @durandom before merging, for the chance of getting to the bottom of the issue and potentially issuing a cleaner solution

[durandom]

As of now I'm tracking what might be the error where this class is not being instantiated on miq-core

Thats right, but actually that has never been the case 😟
Because the schema is missing a STI column

So while you can ManageIQ::Providers::Hawkular::MiddlewareManager::MiddlewareServer.new.save it wont be the subclass if you load it from the db again.
You'll have to include the NewWithTypeStiMixin in the base class and add a type column to the db.
Maybe this is also broken/missing for other models?

So, to fix this, you'll have to move the method implementation over here. like:

def immutable?
  if properties.has_key? 'Immutable'
   properties['Immutable'] == 'true'
  else
   true
  end
end

[durandom]

@cfcosta you should probably use the factory in the specs for immutable and reload the object from db. That should give you a failing spec

[abonas]

@durandom thanks a lot for investigating this.

Thats right, but actually that has never been the case 😟
Because the schema is missing a STI column

So while you can ManageIQ::Providers::Hawkular::MiddlewareManager::MiddlewareServer.new.save it wont be the subclass if you load it from the db again.
You'll have to include the NewWithTypeStiMixin in the base class and add a type column to the db.
Maybe this is also broken/missing for other models?

the above should be done after 'fine' release as the fine branch no longer receives db changes.

So, to fix this, you'll have to move the method implementation over here. like:

the below should be done now in order to fix it in 'fine'.
Looks like it should be in this PR.
@cfcosta wdyt?

def immutable?
if properties.has_key? 'Immutable'
properties['Immutable'] == 'true'
else
true
end
end

[durandom]

When you are doing the follow up PR, maybe you want to lead the way and namespace all middleware_ models under Middleware? So MiddlewareServer would become Middleware::Server

I see that could be of use for containers and storage and other models too.

[abonas]

@miq-bot assign @durandom
@cfcosta , @Jiri-Kremser @durandom please work on resolving this asap without the STI fix so it can enter the 'fine' branch. and we'll definitely need the "right" STI solution afterwards

[miq-bot]

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

[cfcosta]

@durandom @Jiri-Kremser @abonas did the changes you suggested. I'm creating a followup PR for doing the migration of the data and implementing STI correctly, will post later today and we can discuss how to proceed for the next version.

[durandom]

@miq-bot assign @blomquisg
@blomquisg please merge, follow up PRs will do this properly

[blomquisg]

The changed code in this method looks fine, but the comment here seems wrong.

It doesn't look like it's defaulting to mutable. It looks like it's defaulting to whatever the properties are.

[durandom]

too much boolean logic here.
the default is an empty {}, which does not have the property and hence returns false which is not immutable which is mutable

so the comment is right... once you meditate long enough 😄

maybe drop or rephrase it in a later pr

[blomquisg]

Ha! K, thanks for the logic test @durandom :)

[simaishi]

Fine backport details:

$ git log -1
commit d1f688853c1b4d697dcf95b8294d394a99e8332f
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Tue Apr 25 14:06:45 2017 -0400

    Merge pull request #14822 from cfcosta/fix-immutability-defaults-for-ms
    
    Fix defaults for immutability of MiddlewareServers
    (cherry picked from commit 16c43457b788ae12d49929ce1e696fd1f3e445cb)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1446387