Add parameters to upstream and upstreammembers #1233

SaschaDoering · 2018-07-16T14:00:18Z

Pull Request (PR) description

Add parameters to upstream and upstreammembers

Add parameters to nginx::resource::upstream and
nginx::resource::upstream::member which allows more configuration on
upstreams as before. The only thing that we broke is that the
members of an upstream must now be passed as a hash rather than an
array. This also makes the sorting of keepalive to the end no
longer necessary because there is now a parameter for it. And
values for a nginx::resource::upstream::member can now be set as
default for all members of an upstream or individually for each
member inside the members hash. Of course, the explicit
specification overrides the defaults. In general the changes have
made more parameters available to nginx::resource::upstream and
nginx::resource::upstream::member. In addition, one of the two
templates for nginx::resource::upstream::member was disposed since
it is no longer needed.

This Pull Request (PR) fixes the following issues

Fixes GH-1222

SaschaDoering · 2018-08-13T12:41:35Z

@juniorsysadmin Which kind of feedback do you need from me?

juniorsysadmin · 2018-08-13T23:18:17Z

@SaschaDoering Nothing required from you just yet, but given this is a breaking change it would good to get some approval on the design from other users before merging.

SaschaDoering · 2018-08-14T08:07:40Z

Thanks for the feedback, I thought something was missing from my side :)

baurmatt · 2018-08-29T08:29:09Z

LGTM! :)

bastelfreak · 2018-08-29T12:32:21Z

Is the only breaking change the new datatype for $members? Then it should be possible to implement this without backwards incompatible changes?

SaschaDoering · 2018-08-29T15:18:16Z

Yes, this should be the only breaking change. In addition, it is possible to set different values for members of the upstream which is necessary for things like weight. I also don't see a reason for adding extra code for the old method given the README.md states Please note: This module is undergoing some structural maintenance. You may experience breaking changes between minor versions. Especially since the changes to the configuration will be very minimal. On the other hand, the changes in the code would not be so minimal. Would it help to expand the README.md with a migration example?

bastelfreak · 2018-08-29T15:29:21Z

sure this is mentioned in the readme and we still didn't do a 1.0.0 release, but still backwards compatibility is always nice. Can you add backwards support or update the READMe.md?

bastelfreak

I did some inline comments, please have a look. Can you add some acceptance tests so we now this works?

bastelfreak · 2018-08-29T15:31:26Z

manifests/resource/upstream.pp

-  Enum['http', 'stream'] $upstream_context  = 'http',
+  Enum['present', 'absent']                 $ensure                 = 'present',
+  Enum['http', 'stream']                    $context                = 'http',
+  Optional[Hash]                            $members                = undef,


can this default to an empty hash instead of optional? We have some guidelines for datatypes at https://voxpupuli.org/docs/#reviewing-a-module-pr

Are there new params with datatype Hash or Array? If possible, they should default to empty Hash/Array instead of undef. You can also enforce the datastructure like Array[String]

and:

If you can supply one or multiple values for an attribute it’s common practice to enforce the datatype for one value and an array of that datatype. An example for string is Variant[String[1],Array[String[1]]]. This can be used in the Puppet code as [$var].flatten()

can you also define the datatypes within the hash?

You're right, I'll fix that.

bastelfreak · 2018-08-29T15:32:42Z

manifests/resource/upstream.pp

+  Optional[String]                          $members_tag            = undef,
+  Optional[Nginx::UpstreamMemberDefaults]   $member_defaults        = undef,
+  Optional[String]                          $hash                   = undef,
+  Optional[Boolean]                         $ip_hash                = undef,


Please check all parameters with the Boolean dataype. If it makes sense, remove the Optional and let them default to True or False

You're right, again, I'll fix that, too.

bastelfreak · 2018-08-29T15:35:07Z

templates/upstream/upstream_footer.erb

-  @upstream_cfg_append['keepalive'] = @upstream_cfg_append.delete('keepalive')
-%>
-  <%- @upstream_cfg_append.each do |key,value| -%>
+<% if @hash -%>


are you interested in turning this into an epp template? From our docs:

Is a new template added? The preferred language is epp, not erb

it isn't required to turn this into an epp template, but it would be nice to have.

If I convert upstream_member.erb I will convert this too. It makes no sense to use different templatetypes for one file I think.

bastelfreak · 2018-08-29T15:37:01Z

templates/upstream/upstream_member.erb

@@ -1 +1,12 @@
-  server     <%= @server %>:<%= @port %>  fail_timeout=<%= @upstream_fail_timeout %><% if @upstream_max_fails -%> max_fails=<%=@upstream_max_fails %><% end %>;
+  server <%= @_server %><% if @params_prepend -%> <%= @params_prepend %><% end -%>


please convert this into an epp template.

I'll convert the template.

SaschaDoering · 2018-09-05T14:35:15Z

I pushed a new version a few moments ago which should fix the issues you commented. Please let me know if I missed something, thanks.

bastelfreak · 2018-09-05T15:50:03Z

manifests/resource/upstream.pp

+  Optional[Nginx::UpstreamSticky]     $sticky                 = undef,
+  Optional[Nginx::UpstreamZone]       $zone                   = undef,
+  Hash                                $cfg_append             = {},
+  Hash                                $cfg_prepend            = {},


Can you enforce the datatypes within the hash please?

I've added a datatype for this.

bastelfreak · 2018-09-05T15:50:41Z

manifests/resource/upstream.pp

+  Boolean                             $ntlm                   = false,
+  Optional[Integer]                   $queue_max              = undef,
+  Optional[Nginx::Time]               $queue_timeout          = undef,
+  Optional[String]                    $random                 = undef,


For strings, please always use String[1], it will enforce a minimal string length of 1.

Should be fixed.

bastelfreak · 2018-09-05T15:51:13Z

spec/acceptance/nginx_upstream_spec.rb

@@ -0,0 +1,113 @@
+require 'spec_helper_acceptance'
+
+describe 'nginx::resource::upstream define:' do


acceptance tests \o/

spec/type_aliases/size.rb

Add parameters to nginx::resource::upstream and nginx::resource::upstream::member which allows more configuration on upstreams as before. The only thing that we broke is that the members of an upstream must now be passed as a hash rather than an array. This also makes the sorting of keepalive to the end no longer necessary because there is now a parameter for it. And values for a nginx::resource::upstream::member can now be set as default for all members of an upstream or individually for each member inside the members hash. Of course, the explicit specification overrides the defaults. In general the changes have made more parameters available to nginx::resource::upstream and nginx::resource::upstream::member. In addition, one of the two templates for nginx::resource::upstream::member was disposed since it is no longer needed. Fixes voxpupuliGH-1222

SaschaDoering · 2018-09-06T14:26:00Z

I pushed an updated version which fix the datatype issues. So it's your turn again ;) Thanks.

bastelfreak · 2018-09-06T15:26:33Z

looks good to me, thanks to the massive PR! I will keep this open so we can get another review from somebody else.

SaschaDoering · 2018-09-06T15:42:11Z

Ok, thanks for the review.

dhollinger

Just a couple questions surrounding the changes to the $members parameter, specifically around how the datatype parses that parameter and if there's another way to do it.

dhollinger · 2018-09-06T17:23:51Z

README.md

+  members => {
+    'localhost:3000':
+      server => 'localhost'
+      port   => 3000


At first glance this can be a little confusing. My first question would be "Why not parse the localhost:3000 into server and port?"

Could we potentially make it an array of hashes so that it looks more like:

nginx::resource::upstream { 'puppet_rack_app': members => [ { server => 'localhost', port => 3000, }, { server => 'localhost', port => 3001, }, { server => 'localhost', port => 3002, }, ]

Does that work or does the parameter rely on each member to have a unique name?

Mainly cause Unix sockets are accepted too. We would first have to see if it is a valid Unix socket and only if not split at the colon. After that we need to check the parts if they are valid. We almost have to implement the used datatype in code. And at the end we have the problem to name the resources for the defined type which is necessary for exporting upstream members. That's why we need a unique name here.
Should I change the README.md file and use keys that do not consist of server and port? Maybe it's a good idea to use the keys as default for the comment.

dhollinger · 2018-09-06T17:24:41Z

manifests/resource/upstream.pp

-  Enum['http', 'stream'] $upstream_context  = 'http',
+  Enum['present', 'absent']           $ensure                 = 'present',
+  Enum['http', 'stream']              $context                = 'http',
+  Nginx::UpstreamMembers              $members                = {},


Same comment as above

SaschaDoering · 2018-09-27T08:22:48Z

Is there anything left or any open points to discuss?

bastelfreak · 2018-10-10T15:27:20Z

Hey @voxpupuli/collaborators, could I get your feedback here?

baurmatt · 2018-10-19T14:20:25Z

Can we please get this merged or an update what changes should be made?

juniorsysadmin · 2018-10-20T10:14:12Z

manifests/resource/upstream.pp

    order   => '90',
-    content => template('nginx/upstream/upstream_footer.erb'),
+    content => epp('nginx/upstream/upstream_footer.epp', {


My personal preference is for no parameters for epp(), where possible.

I highly prefer them, because that allows scoped variables with proper datatypes.

voxpupuli#1233 was a breaking change that amongst other things, renamed this parameter. Fixes voxpupuli#1316

Add parameters to upstream and upstreammembers

voxpupuli#1233 was a breaking change that amongst other things, renamed this parameter. Fixes voxpupuli#1316

Add parameters to upstream and upstreammembers

voxpupuli#1233 was a breaking change that amongst other things, renamed this parameter. Fixes voxpupuli#1316

ghost force-pushed the add_upstream_parameters branch from cf49d4c to 91a8a7b Compare July 16, 2018 15:00

juniorsysadmin added backwards-incompatible needs-feedback Further information is requested labels Aug 5, 2018

juniorsysadmin requested a review from bastelfreak August 29, 2018 12:27

bastelfreak requested changes Aug 29, 2018

View reviewed changes

bastelfreak added enhancement New feature or request needs-tests needs-work not ready to merge just yet and removed needs-feedback Further information is requested labels Aug 29, 2018

ghost force-pushed the add_upstream_parameters branch 5 times, most recently from 41ccfe2 to bf8febd Compare September 5, 2018 14:07

bastelfreak reviewed Sep 5, 2018

View reviewed changes

spec/type_aliases/size.rb Show resolved Hide resolved

bastelfreak removed the needs-tests label Sep 5, 2018

ghost force-pushed the add_upstream_parameters branch 3 times, most recently from 9d4b426 to 4272747 Compare September 6, 2018 13:36

ghost force-pushed the add_upstream_parameters branch from 4272747 to aa529c7 Compare September 6, 2018 13:44

ghost force-pushed the add_upstream_parameters branch from aa529c7 to f0bf83a Compare September 6, 2018 13:58

bastelfreak approved these changes Sep 6, 2018

View reviewed changes

dhollinger reviewed Sep 6, 2018

View reviewed changes

juniorsysadmin reviewed Oct 20, 2018

View reviewed changes

juniorsysadmin approved these changes Oct 20, 2018

View reviewed changes

juniorsysadmin removed the needs-work not ready to merge just yet label Oct 20, 2018

juniorsysadmin merged commit 74703e3 into voxpupuli:master Oct 20, 2018

SaschaDoering mentioned this pull request Nov 19, 2018

Fix the condition for upstream members #1276

Merged

alexjfisher mentioned this pull request Feb 26, 2019

Hiera: Resource Statement, Nginx::Resource::Upstream[radius]: has no parameter named 'upstream_context' #1316

Closed

alexjfisher added a commit to alexjfisher/puppet-nginx that referenced this pull request Feb 26, 2019

Fix upstream_context parameter in README

a779cfe

voxpupuli#1233 was a breaking change that amongst other things, renamed this parameter. Fixes voxpupuli#1316

alexjfisher mentioned this pull request Feb 26, 2019

Fix upstream_context parameter in README #1317

Merged

cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019

Merge pull request voxpupuli#1233 from syseleven/add_upstream_parameters

7f746ed

Add parameters to upstream and upstreammembers

cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019

Fix upstream_context parameter in README

e5f7103

voxpupuli#1233 was a breaking change that amongst other things, renamed this parameter. Fixes voxpupuli#1316

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020

Merge pull request voxpupuli#1233 from syseleven/add_upstream_parameters

bb6d444

Add parameters to upstream and upstreammembers

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020

Fix upstream_context parameter in README

59b290a

voxpupuli#1233 was a breaking change that amongst other things, renamed this parameter. Fixes voxpupuli#1316

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parameters to upstream and upstreammembers #1233

Add parameters to upstream and upstreammembers #1233

SaschaDoering commented Jul 16, 2018 •

edited

Loading

SaschaDoering commented Aug 13, 2018

juniorsysadmin commented Aug 13, 2018

SaschaDoering commented Aug 14, 2018

baurmatt commented Aug 29, 2018

bastelfreak commented Aug 29, 2018

SaschaDoering commented Aug 29, 2018

bastelfreak commented Aug 29, 2018

bastelfreak left a comment

bastelfreak Aug 29, 2018

SaschaDoering Sep 4, 2018

bastelfreak Aug 29, 2018

SaschaDoering Sep 4, 2018

bastelfreak Aug 29, 2018

SaschaDoering Sep 4, 2018

bastelfreak Aug 29, 2018

SaschaDoering Sep 4, 2018

SaschaDoering commented Sep 5, 2018

bastelfreak Sep 5, 2018

SaschaDoering Sep 6, 2018

bastelfreak Sep 5, 2018

SaschaDoering Sep 6, 2018

bastelfreak Sep 5, 2018

SaschaDoering commented Sep 6, 2018

bastelfreak commented Sep 6, 2018

SaschaDoering commented Sep 6, 2018

dhollinger left a comment

dhollinger Sep 6, 2018

SaschaDoering Sep 7, 2018

dhollinger Sep 6, 2018

SaschaDoering commented Sep 27, 2018

bastelfreak commented Oct 10, 2018

baurmatt commented Oct 19, 2018

juniorsysadmin Oct 20, 2018

bastelfreak Oct 20, 2018

		@@ -1 +1,12 @@
		server <%= @server %>:<%= @port %> fail_timeout=<%= @upstream_fail_timeout %><% if @upstream_max_fails -%> max_fails=<%=@upstream_max_fails %><% end %>;
		server <%= @_server %><% if @params_prepend -%> <%= @params_prepend %><% end -%>

		@@ -0,0 +1,113 @@
		require 'spec_helper_acceptance'

		describe 'nginx::resource::upstream define:' do

Add parameters to upstream and upstreammembers #1233

Add parameters to upstream and upstreammembers #1233

Conversation

SaschaDoering commented Jul 16, 2018 • edited Loading

Pull Request (PR) description

Add parameters to upstream and upstreammembers

This Pull Request (PR) fixes the following issues

SaschaDoering commented Aug 13, 2018

juniorsysadmin commented Aug 13, 2018

SaschaDoering commented Aug 14, 2018

baurmatt commented Aug 29, 2018

bastelfreak commented Aug 29, 2018

SaschaDoering commented Aug 29, 2018

bastelfreak commented Aug 29, 2018

bastelfreak left a 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

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

SaschaDoering commented Sep 5, 2018

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

SaschaDoering commented Sep 6, 2018

bastelfreak commented Sep 6, 2018

SaschaDoering commented Sep 6, 2018

dhollinger left a 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

SaschaDoering commented Sep 27, 2018

bastelfreak commented Oct 10, 2018

baurmatt commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaDoering commented Jul 16, 2018 •

edited

Loading