Ref #9134 - gb plugin sets configs for foreman-gutterball #58

dustints · 2015-01-28T06:39:53Z

komidore64 · 2015-01-30T22:57:33Z

templates/foreman_gutterball.yaml.erb

@@ -0,0 +1,2 @@
+<%# for foreman_gutterball plugin %>
+:url: <%= @fqdn %>


this should have :8443/gutterball on the end, i believe. other than that, i will see if i can test this on monday!

ehelms · 2015-01-30T23:15:01Z

A more tpyical convention would be to create manifests/plugin/gutterball/config.pp and perhaps to mimic the same for templates to help with separation and locating things, e.g. templates/plugin/gutterball/foreman_gutterball.yaml.erb

ehelms · 2015-01-30T23:18:50Z

manifests/plugin/foreman_gutterball_config.pp

+class katello::plugin::foreman_gutterball_config(
+    $foreman_user  = 'foreman',
+    $foreman_group = 'foreman',
+    $foreman_plugins_dir = '/usr/share/foreman/plugins',


I don't think this is the right location for the file to get laid down. I believe it should be /etc/foreman/settings.plugins.d/ . See https://github.com/theforeman/foreman/blob/develop/config/settings.rb#L14

dustints · 2015-02-02T05:15:52Z

@ehelms, @komidore64 , @ekohl
addressed commends.

template is now @ templates/plugin/gutterball/foreman_gutterball_addressable.yaml.erb
does not concat with the gem's settings into one config. instead it creates a new config in /usr/foreman..settings.plugin.d/foreman_gutterball_auxiliary.yaml

ekohl · 2015-02-02T11:15:36Z

manifests/plugin/gutterball.pp

@@ -1,9 +1,26 @@
 # gutterball plugin
 class katello::plugin::gutterball{
+  $foreman_gutterball_gem_config_tmp = '/tmp/foreman_gutterball.yaml'
+
+  $package = "${foreman::plugin_prefix}gutterball"


This whole block appears to be unused.

dustints · 2015-02-02T12:52:48Z

@ekohl updated.

ehelms · 2015-02-02T14:41:39Z

manifests/plugin/gutterball/config.pp

+    $foreman_plugins_dir = '/etc/foreman/plugins',
+  ){
+
+  file { "${foreman_plugins_dir}/foreman_gutterball_auxiliary.yaml":


Why 'auxiliary' ?

what's a good name...forman_gutterball_url?

How about just foreman_gutterball.yaml ? I don't see why it needs a special name when it is simply the config file for foreman_gutterball.

I'd use gutterball.yaml since the plugin is also named gutterball. In the PR I mentioned I will also default to that if you don't specify a config filename.

dustints · 2015-02-02T15:57:06Z

@ehelms, @ekohl updated

rename foreman_gutterball_aux => foreman_gutterball_url.yaml
parameterize the url supplied in template.

ekohl · 2015-02-02T16:03:12Z

templates/plugin/gutterball/foreman_gutterball_url.yaml.erb

@@ -0,0 +1,6 @@
+:foreman_gutterball_url:
+  :protocol: <%= @protocol %>


Does the plugin use these variables? I'd only parameterize the configuration parameters that the plugin actually uses.

@komidore64 do you think there is any value for foreman-gutterball to have discrete portions of the url?

we could go either way. i was doing similar to what is found in katello's config

for example:

common: # ... candlepin: url: https://localhost:8443/candlepin

dustints · 2015-02-02T17:39:40Z

@komidore64, @ehelms, @ekohl
updated

just using the same format as katello's config for url
let this module manage foreman_gutterball.yaml so renaming templates and destination config file to foreman_gutterball.yaml

ehelms · 2015-02-02T17:52:23Z

manifests/plugin/gutterball/config.pp

+    $foreman_plugins_dir = '/etc/foreman/plugins',
+    $protocol = 'https',
+    $port = '8443',
+    $path = 'gutterball/',


I think we typically defer to leading slashes instead of trailing slashes for URL paths.

dustints · 2015-02-02T17:57:20Z

@ehelms updated

ehelms · 2015-02-02T18:00:34Z

APT

komidore64 · 2015-02-02T18:25:04Z

templates/plugin/gutterball/foreman_gutterball.yaml.erb

@@ -0,0 +1,2 @@
+:foreman_gutterball:
+  :url: <%= @protocol %>://<%= @fqdn %>:<%= @port %><%= @path %>


one last thing...

let's stick with the keys being strings instead of symbols, so we can stay uniform with other configs.

so

foreman_gutterball: url: <%= @protocol %>://<%= @fqdn %>:<%= @port %><%= @path %>

dustints · 2015-02-02T18:31:39Z

@komidore64 updated

komidore64 · 2015-02-02T18:32:36Z

awesome!

ack pending tests

ekohl · 2015-02-02T22:34:00Z

manifests/plugin/gutterball/config.pp

+    $foreman_user  = 'foreman',
+    $foreman_group = 'foreman',
+    $foreman_plugins_dir = '/etc/foreman/plugins',
+    $protocol = 'https',


I still think a single URL parameter is easier than all the separate components. Now you're still forced to use the fqdn which a user might not want.

dustints · 2015-02-03T12:48:11Z

@ekohl updated to a single url param

ekohl · 2015-02-03T12:58:35Z

manifests/plugin/gutterball/config.pp

+    $url = "https://${fqdn}:8443/gutterball",
+  ){
+
+  file { "${foreman_plugins_dir}/foreman_gutterball.yaml":


I'd name this simply gutterball.yaml since foreman is implied.

I prefer things are explicit and match the plugin name. That way there is
no translation or discrepancy whenever you are looking for something
related. I think that the namespace of the plugins config and the file
containing it should remain the same.
On Feb 3, 2015 7:58 AM, "Ewoud Kohl van Wijngaarden" <
notifications@github.com> wrote:

In manifests/plugin/gutterball/config.pp
#58 (comment):

@@ -0,0 +1,15 @@
+# gutterball config
+class katello::plugin::gutterball::config(

$foreman_user = 'foreman',

$foreman_group = 'foreman',

$foreman_plugins_dir = '/etc/foreman/plugins',

$url = "https://${fqdn}:8443/gutterball",

){

file { "${foreman_plugins_dir}/foreman_gutterball.yaml":

I'd name this simply gutterball.yaml since foreman is implied.

—
Reply to this email directly or view it on GitHub
https://github.com/Katello/puppet-katello/pull/58/files#r24001396.

That's also a good point. I'll think about this and get back to you.

I agree with @ehelms. the name of the plugin is foreman_gutterball so the plugin's config file should have the same name.

dustints · 2015-02-03T13:45:26Z

@ekohl, updated renamed the source template & destination config file

Ref #9134 - gb plugin sets configs for foreman-gutterball

dustints · 2015-02-04T15:34:09Z

merging! too much time spent on this 10 liner

dustints force-pushed the foreman_gutterball_config branch 5 times, most recently from b0b8f48 to 27015e3 Compare January 30, 2015 22:52

dustints mentioned this pull request Jan 30, 2015

Ref #9134 - foreman-gb plugin sets configs for foreman-gutterball theforeman/puppet-katello_devel#30

Merged

komidore64 reviewed Jan 30, 2015
View reviewed changes

ehelms reviewed Jan 30, 2015
View reviewed changes

dustints force-pushed the foreman_gutterball_config branch 2 times, most recently from c81439d to 324ed5a Compare February 2, 2015 05:14

ekohl reviewed Feb 2, 2015
View reviewed changes

dustints force-pushed the foreman_gutterball_config branch 2 times, most recently from 6573d9f to b95683b Compare February 2, 2015 12:50

ehelms reviewed Feb 2, 2015
View reviewed changes

dustints force-pushed the foreman_gutterball_config branch from b95683b to b83f77a Compare February 2, 2015 15:55

ekohl reviewed Feb 2, 2015
View reviewed changes

dustints force-pushed the foreman_gutterball_config branch 2 times, most recently from ffd5a89 to e21bfb0 Compare February 2, 2015 17:22

ehelms reviewed Feb 2, 2015
View reviewed changes

dustints force-pushed the foreman_gutterball_config branch from e21bfb0 to e840e4a Compare February 2, 2015 17:55

dustints force-pushed the foreman_gutterball_config branch from e840e4a to d92512d Compare February 2, 2015 17:57

komidore64 reviewed Feb 2, 2015
View reviewed changes

dustints force-pushed the foreman_gutterball_config branch 2 times, most recently from 5b3e5ae to 11b15d4 Compare February 2, 2015 18:30

ekohl reviewed Feb 2, 2015
View reviewed changes

dustints force-pushed the foreman_gutterball_config branch from 11b15d4 to aefb5e8 Compare February 3, 2015 12:47

ekohl reviewed Feb 3, 2015
View reviewed changes

dustints force-pushed the foreman_gutterball_config branch from aefb5e8 to 3d07b65 Compare February 3, 2015 13:43

Ref #9134 - gb plugin sets configs for foreman-gutterball

ccaf98d

dustints force-pushed the foreman_gutterball_config branch from 3d07b65 to ccaf98d Compare February 3, 2015 14:02

dustints pushed a commit that referenced this pull request Feb 4, 2015

Merge pull request #58 from dustints/foreman_gutterball_config

b0d65ee

Ref #9134 - gb plugin sets configs for foreman-gutterball

dustints merged commit b0d65ee into theforeman:master Feb 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ref #9134 - gb plugin sets configs for foreman-gutterball #58

Ref #9134 - gb plugin sets configs for foreman-gutterball #58

dustints commented Jan 28, 2015

komidore64 Jan 30, 2015

dustints Feb 2, 2015

ehelms commented Jan 30, 2015

ehelms Jan 30, 2015

dustints commented Feb 2, 2015

ekohl Feb 2, 2015

dustints commented Feb 2, 2015

ehelms Feb 2, 2015

dustints Feb 2, 2015

ehelms Feb 2, 2015

ekohl Feb 2, 2015

dustints commented Feb 2, 2015

ekohl Feb 2, 2015

dustints Feb 2, 2015

komidore64 Feb 2, 2015

dustints commented Feb 2, 2015

ehelms Feb 2, 2015

dustints commented Feb 2, 2015

ehelms commented Feb 2, 2015

komidore64 Feb 2, 2015

dustints commented Feb 2, 2015

komidore64 commented Feb 2, 2015

ekohl Feb 2, 2015

dustints commented Feb 3, 2015

ekohl Feb 3, 2015

ehelms Feb 3, 2015

ekohl Feb 3, 2015

komidore64 Feb 3, 2015

dustints commented Feb 3, 2015

dustints commented Feb 4, 2015

		@@ -0,0 +1,2 @@
		<%# for foreman_gutterball plugin %>
		:url: <%= @fqdn %>

		@@ -0,0 +1,6 @@
		:foreman_gutterball_url:
		:protocol: <%= @protocol %>

		@@ -0,0 +1,2 @@
		:foreman_gutterball:
		:url: <%= @protocol %>://<%= @fqdn %>:<%= @port %><%= @path %>

Ref #9134 - gb plugin sets configs for foreman-gutterball #58

Ref #9134 - gb plugin sets configs for foreman-gutterball #58

Conversation

dustints commented Jan 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Jan 30, 2015

Choose a reason for hiding this comment

dustints commented Feb 2, 2015

Choose a reason for hiding this comment

dustints commented Feb 2, 2015

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

dustints commented Feb 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustints commented Feb 2, 2015

Choose a reason for hiding this comment

dustints commented Feb 2, 2015

ehelms commented Feb 2, 2015

Choose a reason for hiding this comment

dustints commented Feb 2, 2015

komidore64 commented Feb 2, 2015

Choose a reason for hiding this comment

dustints commented Feb 3, 2015

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

dustints commented Feb 3, 2015

dustints commented Feb 4, 2015