Configure monasca-ui plugin by samirjorina · Pull Request #6 · FujitsuEnablingSoftwareTechnologyGmbH/crowbar-openstack

samirjorina · 2017-01-12T12:30:18Z

No description provided.

houndci-bot · 2017-01-16T06:42:41Z

chef/cookbooks/horizon/recipes/server.rb

  end
 end

+


Style/EmptyLines: Extra blank line detected.

houndci-bot · 2017-01-16T06:42:41Z

chef/cookbooks/horizon/recipes/server.rb

+    config["kibana_host"] = monasca_public_host
+    config["grafana_url"] = "#{monasca_protocol}://#{monasca_public_host}:#{grafana_port}/"
+    config["os_region"] = keystone_settings["endpoint_region"]
+    template "/srv/www/openstack-dashboard/openstack_dashboard/local/local_settings.d/_80_monasca_ui_settings.py" do


Metrics/LineLength: Line is too long. [116/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

houndci-bot · 2017-01-16T06:42:42Z

chef/cookbooks/horizon/recipes/server.rb

    end
+
+    monasca_server = monasca_servers[0]
+    #TODO get the grafana port from data bag?


Style/LeadingCommentSpace: Missing space after #. (https://github.com/bbatsov/ruby-style-guide#hash-space)
Style/CommentAnnotation: Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem. (https://github.com/bbatsov/ruby-style-guide#annotate-keywords)

houndci-bot · 2017-01-20T07:58:31Z

chef/cookbooks/horizon/recipes/server.rb

+      variables config
+      owner 'root'
+      group 'root'
+      mode '0644'


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

houndci-bot · 2017-01-20T07:58:31Z

chef/cookbooks/horizon/recipes/server.rb

+      source "monasca_ui_local_settings.py.erb"
+      variables config
+      owner 'root'
+      group 'root'


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

houndci-bot · 2017-01-20T07:58:31Z

chef/cookbooks/horizon/recipes/server.rb

+    template "/srv/www/openstack-dashboard/openstack_dashboard/local/local_settings.d/_80_monasca_ui_settings.py" do
+      source "monasca_ui_local_settings.py.erb"
+      variables config
+      owner 'root'


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

houndci-bot · 2017-01-20T10:18:25Z

chef/cookbooks/horizon/recipes/server.rb

+      variables config
+      owner 'root'
+      group 'root'
+      mode '0644'


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

houndci-bot · 2017-01-20T10:18:26Z

chef/cookbooks/horizon/recipes/server.rb

+      source "_80_monasca_ui_settings.py.erb"
+      variables config
+      owner 'root'
+      group 'root'


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

houndci-bot · 2017-01-20T10:18:26Z

chef/cookbooks/horizon/recipes/server.rb

+    template "/srv/www/openstack-dashboard/openstack_dashboard/local/local_settings.d/_80_monasca_ui_settings.py" do
+      source "_80_monasca_ui_settings.py.erb"
+      variables config
+      owner 'root'


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

houndci-bot · 2017-01-20T10:18:26Z

chef/cookbooks/horizon/recipes/server.rb

+    config["kibana_enabled"] = true
+    config["kibana_host"] = monasca_public_host
+
+    template "/srv/www/openstack-dashboard/openstack_dashboard/local/local_settings.d/_80_monasca_ui_settings.py" do


Metrics/LineLength: Line is too long. [116/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

houndci-bot · 2017-01-20T10:36:22Z

chef/cookbooks/horizon/recipes/server.rb

  end
 end

+#TODO extract this all to a separate reciepe


Style/LeadingCommentSpace: Missing space after #. (https://github.com/bbatsov/ruby-style-guide#hash-space)
Style/CommentAnnotation: Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem. (https://github.com/bbatsov/ruby-style-guide#annotate-keywords)

houndci-bot · 2017-01-31T06:40:13Z

chef/cookbooks/monasca/libraries/helper.rb

+    protocol = node[:monasca][:grafana][:ssl] ? "https" : "http"
+    port = node[:monasca][:grafana][:bind_port]
+    "#{protocol}://#{host}:#{port}/grafana"
+  end 


Style/TrailingWhitespace: Trailing whitespace detected. (https://github.com/bbatsov/ruby-style-guide#no-trailing-whitespace)

houndci-bot · 2017-01-31T06:40:13Z

chef/cookbooks/horizon/recipes/monasca_ui.rb

+monasca_servers = search(:node, "roles:monasca-server")
+monasca_server = monasca_servers[0]
+grafana_available = true
+grafana_url = MonascaUiHelper.grafana_public_url(monasca_server)


Lint/UselessAssignment: Useless assignment to variable - grafana_url. (https://github.com/bbatsov/ruby-style-guide#underscore-unused-vars)

houndci-bot · 2017-01-31T06:40:13Z

chef/cookbooks/horizon/recipes/monasca_ui.rb

+
+monasca_servers = search(:node, "roles:monasca-server")
+monasca_server = monasca_servers[0]
+grafana_available = true


Lint/UselessAssignment: Useless assignment to variable - grafana_available. (https://github.com/bbatsov/ruby-style-guide#underscore-unused-vars)

houndci-bot · 2017-01-31T06:40:13Z

chef/cookbooks/horizon/libraries/helper.rb

+    protocol = node[:monasca][:grafana][:ssl] ? "https" : "http"
+    port = node[:monasca][:grafana][:bind_port]
+    "#{protocol}://#{host}:#{port}/grafana"
+  end 


Style/TrailingWhitespace: Trailing whitespace detected. (https://github.com/bbatsov/ruby-style-guide#no-trailing-whitespace)

matrixik · 2017-01-31T12:23:39Z

chef/cookbooks/horizon/libraries/helper.rb

@@ -0,0 +1,14 @@
+module MonascaUiHelper


matrixik · 2017-01-31T12:24:11Z

chef/cookbooks/horizon/recipes/monasca_ui.rb

@@ -0,0 +1,18 @@
+include_recipe "apache2::mod_proxy"


matrixik · 2017-01-31T12:29:53Z

chef/cookbooks/horizon/templates/suse/openstack-dashboard.conf.erb

    DocumentRoot <%= @horizon_dir %>
    Alias /media <%= @horizon_dir %>/media
    Alias /static <%= @horizon_dir %>/static
+    <% if @grafana_available %>


I would do it the same way that is already done at line 33:
<% unless @ssl_crt_chain_file.nil? or @ssl_crt_chain_file.empty? %>

matrixik · 2017-01-31T12:30:03Z

chef/cookbooks/horizon/templates/suse/openstack-dashboard.conf.erb

        ExpiresActive on
        ExpiresDefault "access plus 1 month"
    </Location>
+    <% if @grafana_available %>


I would do it the same way that is already done at line 33:
<% unless @ssl_crt_chain_file.nil? or @ssl_crt_chain_file.empty? %>

matrixik · 2017-01-31T12:35:30Z

chef/cookbooks/horizon/recipes/server.rb

      action :install
      notifies :reload, resources(service: "apache2")
    end
+    grafana_available = true


Do you actually need grafana_available variable?
You just need checking if grafana_url is not empty to include into template.

Where is grafana_url coming from? Maybe we can use it, but monasca_servers.empty? is a very reliable test. In doubt I'd go for using that as a drop-in replacement for grafana_available rather than the contents of grafana_url (I agree that grafana_available itself is rather redundant).

matrixik · 2017-01-31T12:36:50Z

chef/cookbooks/horizon/templates/default/_80_monasca_ui_settings.py.erb

@@ -0,0 +1,52 @@
+from django.conf import settings


houndci-bot · 2017-02-09T07:38:48Z

chef/cookbooks/horizon/recipes/monasca_ui.rb

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+


Style/EmptyLines: Extra blank line detected.

jgrassler

Apart from the two small inline comments this looks good as far as I can tell right now. Once they're fixed we can merge this and fix anything that's still broken once we have all the ingredients for testing this (i.e. the Grafana package and Kibana host it can talk to).

jgrassler · 2017-02-10T16:07:59Z

chef/cookbooks/horizon/recipes/monasca_ui.rb

+
+
+monasca_servers = search(:node, "roles:monasca-server")
+monasca_server = monasca_servers[0]


Shouldn't this point to the load balancer the Monasca cluster is behind?

I'm sorry, but I simply can't make much sense from the github review process...

It seems to me that your comments refer to some older version of this PR - I have already updated the code, so that right now grafana is deployed on the dashboard node(s). And, as a result, there is no more need to have any URLs to grafana

Github sometimes does that, yeah. I directly checked out your repository and there it looks fine. So ignore these two comments and feel free to merge as far as I'm concerned :-)

jgrassler · 2017-02-10T16:16:16Z

chef/cookbooks/horizon/recipes/server.rb

      action :install
      notifies :reload, resources(service: "apache2")
    end
+    grafana_available = true


Where is grafana_url coming from? Maybe we can use it, but monasca_servers.empty? is a very reliable test. In doubt I'd go for using that as a drop-in replacement for grafana_available rather than the contents of grafana_url (I agree that grafana_available itself is rather redundant).

* Configure monasca-ui plugin * Refactor the code * Addresing review comments * Deploy grafana alongside horizon * Remove obsolete changes to monasca schema * Removing extra blank line

samirjorina added the work in progress label Jan 12, 2017

samirjorina self-assigned this Jan 12, 2017

samirjorina requested a review from matrixik January 12, 2017 12:30

samirjorina force-pushed the monasca_ui_config branch from e7d45b3 to 671b1a5 Compare January 16, 2017 06:42

houndci-bot reviewed Jan 16, 2017

View reviewed changes

chef/cookbooks/horizon/recipes/server.rb Outdated

end

end

Copy link

houndci-bot Jan 16, 2017

Choose a reason for hiding this comment

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

Style/EmptyLines: Extra blank line detected.

houndci-bot reviewed Jan 16, 2017

View reviewed changes

samirjorina force-pushed the monasca_ui_config branch from 671b1a5 to 1fabee8 Compare January 20, 2017 07:58

houndci-bot reviewed Jan 20, 2017

View reviewed changes

samirjorina force-pushed the monasca_ui_config branch from 1fabee8 to 153681d Compare January 20, 2017 10:18

houndci-bot reviewed Jan 20, 2017

View reviewed changes

samirjorina force-pushed the monasca_ui_config branch 2 times, most recently from 481c31b to d560cc8 Compare January 20, 2017 10:36

houndci-bot reviewed Jan 20, 2017

View reviewed changes

samirjorina force-pushed the monasca_ui_config branch 3 times, most recently from fc01b00 to 93598f5 Compare January 23, 2017 09:31

kornicameister force-pushed the monasca branch 2 times, most recently from d78d8db to 9208ba5 Compare January 25, 2017 06:32

samirjorina force-pushed the monasca_ui_config branch from 93598f5 to 07ffd6f Compare January 25, 2017 06:52

houndci-bot reviewed Jan 31, 2017

View reviewed changes

samirjorina force-pushed the monasca_ui_config branch from 1b9cf7f to 61931fd Compare January 31, 2017 06:46

samirjorina removed the work in progress label Jan 31, 2017

matrixik suggested changes Jan 31, 2017

View reviewed changes

samirjorina added the work in progress label Feb 9, 2017

samirjorina added 4 commits February 9, 2017 08:14

Configure monasca-ui plugin

24cbc6c

Refactor the code

ce66ae1

Addresing review comments

cfcf993

Deploy grafana alongside horizon

c02f9aa

samirjorina force-pushed the monasca_ui_config branch from 6a7c685 to c02f9aa Compare February 9, 2017 07:38

houndci-bot reviewed Feb 9, 2017

View reviewed changes

Remove obsolete changes to monasca schema

bd32dfe

samirjorina removed the work in progress label Feb 9, 2017

jgrassler suggested changes Feb 10, 2017

View reviewed changes

Removing extra blank line

dda2df2

samirjorina merged commit 3f03764 into FujitsuEnablingSoftwareTechnologyGmbH:monasca Feb 20, 2017

samirjorina deleted the monasca_ui_config branch February 28, 2017 10:09

		# See the License for the specific language governing permissions and
		# limitations under the License.



		monasca_servers = search(:node, "roles:monasca-server")
		monasca_server = monasca_servers[0]

Conversation

samirjorina commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrassler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants