For middleware UI, only allow operations on mutable servers. #636

mtho11 · 2017-03-08T23:59:40Z

The Hawkular agent now reads the properties from managed servers. These properties can be used to indicate the immutability of the environment (such as an EAP server running in OpenShift). The property being set here is: -Dhawkular.agent.immutable=true

An Immutable server:

A mutable server:

Testing

Start a hawkular server (requires Hawkular Services >= 0.32)
Add a Hawkular Provider
Add manger EAP 7.0 server (with Hawkular Agent) to Hawkular Provider with parameter: -Dhawkular.agent.immutable=true where machineA can be any string. For example:

bin/standalone.sh -Djboss.socket.binding.port-offset=100 -Dhawkular.agent.in-container=true -Dhawkular.agent.immutable=true

Links

mtho11 · 2017-03-09T00:00:45Z

@miq-bot add_label middleware, enhancement, ui

miq-bot · 2017-03-09T00:01:08Z

@mtho11 unrecognized command 'middleware', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

mtho11 · 2017-03-09T00:05:21Z

@miq-bot add_label middleware, enhancement, ui

abonas · 2017-03-15T09:40:40Z

Does the hawkular server ("the provider") also have this flag?

mtho11 · 2017-03-15T14:22:50Z

@abonas yes,
Jira link here

abonas · 2017-03-16T14:06:53Z

app/helpers/application_helper/button/middleware_server_action.rb

-    [email protected]? ||
-      (@record.try(:product) != 'Hawkular' && @record.try(:middleware_server).try(:product) != 'Hawkular')
+    if @record.present?
+      hawkular_product? ? false : !immutable?


Can this part be simplified?
perhaps something like "return record.present? && hawkular_product? && immutable?"

ok thank for the tip, this works: @record.present? && !hawkular_product? && !immutable?

cfcosta

The case for when the record is a hawkular instance should be tested accordingly. The other changes I suggested are more code-quality related, so while they are completely optional in my view, they would increase the code quality by making stuff more encapsulated.

cfcosta · 2017-03-17T00:41:46Z

app/helpers/application_helper/button/middleware_server_action.rb

@@ -1,7 +1,18 @@
 class ApplicationHelper::Button::MiddlewareServerAction < ApplicationHelper::Button::Basic

  def visible?
-    [email protected]? ||
-      (@record.try(:product) != 'Hawkular' && @record.try(:middleware_server).try(:product) != 'Hawkular')
+    @record.present? && !hawkular_product? && !immutable?


I think this class would be better written as:

class ApplicationHelper::Button::MiddlewareServerAction < ApplicationHelper::Button::Basic def NullableRecord delegate :present?, to: :@record def initialize(record) @record = record end def properties @record.try(:properties) end def product @record.try(:product).presence || @record.try(:middleware_server).try(:product) end end def visible? nullable_record = NullableRecord.new(@record) @record.present? && !nullable_record.properties['Immutable'] && nullable_record.product != 'Hawkular' end end

That way you gain a more testable interface and make the visible? method clearer about its implementation.

Also, wouldn't EAP non-immutable servers have the Hawkular product if they are registered into miq by Hawkular? Also, what happens if the created provider for Hawkular is not called Hawkular, but something else? Does this field is unrelated to the created name?

@cfcosta I agree with all of your changes, just not sure it should be part of this PR. I'm just adding an immutable field to existing infrastructure. And we need to get this merged quickly as this is a blocker issue for this release that the window is closing rapidly. Changing more lines of code will subject the PR to more scrutiny which could cause it to miss the release window. You or I can add these changes to a follow-on refactoring PR immediately after this is merged. Besides the fact that, these changes need to be applied consistently to all of the middleware application helper classes and tests.

cfcosta · 2017-03-17T00:44:57Z

spec/helpers/application_helper/buttons/middleware_standalone_server_action_spec.rb

+      end
+
+      context 'when server.properties.immutable? == true' do
+        let(:immutable_server) { FactoryGirl.create(:middleware_server, :properties => create_immutable(true)) }


For those tests you can do something like this:

let(:record) { double(:properties => create_immutable(true)) }

I'll have to read more about double, I'm not familiar.

cfcosta · 2017-03-17T00:45:09Z

spec/helpers/application_helper/buttons/middleware_standalone_server_action_spec.rb

+      end
+
+      context 'when server.properties.immutable? == false' do
+        let(:mutable_server) { FactoryGirl.create(:middleware_server, :properties => create_immutable(false)) }


let(:record) { double(:properties => create_immutable(false) }

cfcosta · 2017-03-17T00:45:47Z

spec/helpers/application_helper/buttons/middleware_standalone_server_action_spec.rb

+        subject { described_class.new(setup_view_context_with_sandbox({}), {}, {'record' => record}, {}) }
+        it { expect(subject).not_to be_visible }
+      end
+    end


These test cases are not testing if the product is hawkular.

thanks @cfcosta, I'm done for the night, will address tomorrow.

I did think about the not testing 'Hawkular' product but it was already not part of the test suite and I didn't want to add permutation explosion. Not sure how we should handle this?

cfcosta · 2017-03-17T17:57:11Z

Sure, I'd be up for that. I'll create a follow-up today then.

…

On Mar 17, 2017 14:39, "Mike Thompson" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/helpers/application_helper/button/middleware_server_action.rb <#636 (comment)> : > @@ -1,7 +1,18 @@ class ApplicationHelper::Button::MiddlewareServerAction < ApplicationHelper::Button::Basic def visible? - ***@***.***? || - ***@***.***(:product) != 'Hawkular' && @record.try(:middleware_server).try(:product) != 'Hawkular') + @record.present? && !hawkular_product? && !immutable? @cfcosta <https://github.com/cfcosta> I agree with all of your changes, just not sure it should be part of this PR. I'm just adding an immutable field to existing infrastructure. And we need to get this merged quickly as this is a blocker issue for this release that the window is closing rapidly. Changing more lines of code will subject the PR to more scrutiny which could cause it to miss the release window. You or I can add these changes to a follow-on refactoring PR immediately after this is merged. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#636 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA5oGdE-IAEPXuabiZu0GfWbwxgVbQBks5rmsVNgaJpZM4MXeIG> .

miq-bot · 2017-03-18T01:58:10Z

Checked commits https://github.com/mtho11/manageiq-ui-classic/compare/768982921884e69ad47a8ac324f89f9ab3597db6~...2e48b198305e19f602d11c1de0c1f9551a26b603 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🍰

mtho11 · 2017-03-20T01:19:28Z

@abonas is this good to go?

abonas · 2017-03-20T10:01:04Z

LGTM
@miq-bot assign @bronaghs

bronaghs · 2017-03-20T13:44:01Z

@miq-bot assign @martinpovolny

abonas · 2017-03-23T12:04:46Z

@martinpovolny @bronaghs @chessbyte can someone please merge it ? thank you

jkremser · 2017-03-29T11:56:08Z

app/helpers/application_helper/button/middleware_server_action.rb

+
+  def hawkular?
+    @record.try(:product) == 'Hawkular' ||
+      @record.try(:middleware_server).try(:product) == 'Hawkular'


It has been already merged, but it may be handy to have those methods also on the server entity itself

cfcosta · 2017-03-29T22:21:24Z

@Jiri-Kremser check ManageIQ/manageiq#14565 and #839

miq-bot added enhancement middleware ui labels Mar 9, 2017

mtho11 changed the title ~~For middleware UI, only allow operations on mutable servers.~~ [WIP]For middleware UI, only allow operations on mutable servers. Mar 9, 2017

miq-bot added the wip label Mar 9, 2017

chessbyte changed the title ~~[WIP]For middleware UI, only allow operations on mutable servers.~~ [WIP] For middleware UI, only allow operations on mutable servers. Mar 9, 2017

dclarizio removed the ui label Mar 13, 2017

mtho11 closed this Mar 15, 2017

mtho11 reopened this Mar 15, 2017

abonas suggested changes Mar 16, 2017

View reviewed changes

cfcosta suggested changes Mar 17, 2017

View reviewed changes

mtho11 added 2 commits March 17, 2017 18:54

For middleware servers, only allow operations on mutable servers.

7689829

Add Rspec examples for immutable middleware servers.

2e48b19

mtho11 changed the title ~~[WIP] For middleware UI, only allow operations on mutable servers.~~ For middleware UI, only allow operations on mutable servers. Mar 18, 2017

miq-bot removed the wip label Mar 18, 2017

miq-bot assigned bronaghs and martinpovolny and unassigned bronaghs Mar 20, 2017

h-kataria added the euwe/no label Mar 23, 2017

h-kataria added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 23, 2017

h-kataria merged commit 064885c into ManageIQ:master Mar 23, 2017

mtho11 deleted the mw-os-disable-cmds2 branch March 23, 2017 14:37

jkremser reviewed Mar 29, 2017

View reviewed changes

cfcosta mentioned this pull request Mar 29, 2017

Refactor MiddlewareServerAction#visible? #839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For middleware UI, only allow operations on mutable servers. #636

For middleware UI, only allow operations on mutable servers. #636

mtho11 commented Mar 8, 2017

mtho11 commented Mar 9, 2017 •

edited

Loading

miq-bot commented Mar 9, 2017

mtho11 commented Mar 9, 2017

abonas commented Mar 15, 2017

mtho11 commented Mar 15, 2017

abonas Mar 16, 2017

mtho11 Mar 16, 2017 •

edited

Loading

cfcosta left a comment

cfcosta Mar 17, 2017

cfcosta Mar 17, 2017

mtho11 Mar 17, 2017 •

edited

Loading

cfcosta Mar 17, 2017

mtho11 Mar 17, 2017

cfcosta Mar 17, 2017

cfcosta Mar 17, 2017

mtho11 Mar 17, 2017 •

edited

Loading

mtho11 Mar 17, 2017

cfcosta commented Mar 17, 2017 via email

miq-bot commented Mar 18, 2017

mtho11 commented Mar 20, 2017

abonas commented Mar 20, 2017

bronaghs commented Mar 20, 2017

abonas commented Mar 23, 2017

jkremser Mar 29, 2017

cfcosta commented Mar 29, 2017

For middleware UI, only allow operations on mutable servers. #636

For middleware UI, only allow operations on mutable servers. #636

Conversation

mtho11 commented Mar 8, 2017

Testing

Links

mtho11 commented Mar 9, 2017 • edited Loading

miq-bot commented Mar 9, 2017

mtho11 commented Mar 9, 2017

abonas commented Mar 15, 2017

mtho11 commented Mar 15, 2017

Choose a reason for hiding this comment

mtho11 Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

cfcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtho11 Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtho11 Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfcosta commented Mar 17, 2017 via email

miq-bot commented Mar 18, 2017

mtho11 commented Mar 20, 2017

abonas commented Mar 20, 2017

bronaghs commented Mar 20, 2017

abonas commented Mar 23, 2017

Choose a reason for hiding this comment

cfcosta commented Mar 29, 2017

mtho11 commented Mar 9, 2017 •

edited

Loading

mtho11 Mar 16, 2017 •

edited

Loading

mtho11 Mar 17, 2017 •

edited

Loading

mtho11 Mar 17, 2017 •

edited

Loading