New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minor refactor of mailhost.pp #1397

Merged

alexjfisher merged 1 commit into voxpupuli:master from alexjfisher:mailhost_cleanups

Aug 29, 2020

Member

alexjfisher commented Jul 17, 2020

Remove what looks like copy-n-pasted lines (copied from resource/server.pp ?)
Use Stdlib::Port instead of Integer

puppet-community-rangefinder bot commented Jul 17, 2020

nginx::resource::mailhost is a type

that may have no external impact to Forge modules.

This module is declared in 11 of 575 indexed public `Puppetfiles`.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

alexjfisher commented

View reviewed changes

manifests/resource/mailhost.pp

@@ @@ -1,7 +1,5 @@ @@
               # define: nginx::resource::mailhost
               #
-              # This definition creates a virtual host

Member Author

alexjfisher Jul 17, 2020

No it doesn't...

alexjfisher commented

View reviewed changes

manifests/resource/mailhost.pp Outdated

@@ @@ -122,22 +120,10 @@ @@
                 String $proxy_pass_error_message               = 'off',
                 Array $server_name                             = [$name]
               ) {

Member Author

alexjfisher Jul 17, 2020

I think the new whitespace lint plugin complains about this line

alexjfisher commented

View reviewed changes

manifests/resource/mailhost.pp

                 if ! defined(Class['nginx']) {
                   fail('You must include the nginx base class before using any defined resources')
                 }
-                $root_group = $nginx::root_group

Member Author

alexjfisher Jul 17, 2020

No need to create a local variable here.

alexjfisher commented

View reviewed changes

manifests/resource/mailhost.pp

                 if ! defined(Class['nginx']) {
                   fail('You must include the nginx base class before using any defined resources')
                 }
-                $root_group = $nginx::root_group
-                File {

Member Author

alexjfisher Jul 17, 2020

There are no File resources in this manifest (and resource defaults like this are discouraged anyway)

alexjfisher commented

View reviewed changes

manifests/resource/mailhost.pp

-                  mode  => $nginx::global_mode,
-                }
-                $config_dir  = "${nginx::conf_dir}/conf.mail.d"

Member Author

alexjfisher Jul 17, 2020

Moved these lines to after the other validation stuff.

alexjfisher force-pushed the mailhost_cleanups branch from 8418c11 to 760f45e Compare

July 17, 2020 11:11

ekohl reviewed

View reviewed changes

Member

ekohl left a comment

I noticed similar things but held back refactoring :)

manifests/resource/mailhost.pp

@@ @@ -77,13 +75,13 @@ @@
               #  }
               #
               define nginx::resource::mailhost (
-                Integer $listen_port,
+                Stdlib::Port $listen_port,

Member

ekohl Jul 17, 2020

Since these are ports, you can drop the + 0 later on in the code. That used to be needed when it could have been a String.

Member Author

alexjfisher Jul 17, 2020

Done!

manifests/resource/mailhost.pp

@@ @@ -77,13 +75,13 @@ @@
               #  }
               #
               define nginx::resource::mailhost (
-                Integer $listen_port,
+                Stdlib::Port $listen_port,
                 Enum['absent', 'present'] $ensure              = 'present',
                 Variant[Array[String], String] $listen_ip      = '*',
                 Optional[String] $listen_options               = undef,
                 Boolean $ipv6_enable                           = false,
                 Variant[Array[String], String] $ipv6_listen_ip = '::',

Member

ekohl Jul 17, 2020

I don't know if this should also be tightened. Not sure about the nginx spec for listening.

alexjfisher force-pushed the mailhost_cleanups branch from 760f45e to 1319e15 Compare

July 17, 2020 11:26

ekohl reviewed

View reviewed changes

manifests/resource/mailhost.pp

@@ @@ -169,7 +158,7 @@ @@
                 }
                 # Create SSL File Stubs if SSL is enabled
-                if ($ssl) {
+                if $ssl {

Member

ekohl Jul 17, 2020

If you're removing these braces, do we also remove them around line 143?

ekohl approved these changes

View reviewed changes

Member

ekohl left a comment

LGTM.

vox-pupuli-tasks bot commented Jul 29, 2020

Dear @alexjfisher, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

vox-pupuli-tasks bot added the tests-fail label

alexjfisher force-pushed the mailhost_cleanups branch from 1319e15 to aeea592 Compare

August 29, 2020 12:04


          Minor refactor of mailhost.pp

9a2b890

* Remove what looks like copy-n-pasted lines (copied from resource/server.pp ?)
* Use `Stdlib::Port` instead of `Integer`
* Get rid of unneeded `+ 0` (which was needed to cast String's to
Integers)

alexjfisher force-pushed the mailhost_cleanups branch from aeea592 to 9a2b890 Compare

August 29, 2020 16:45

alexjfisher removed the tests-fail label

alexjfisher merged commit 9e06714 into voxpupuli:master

alexjfisher deleted the mailhost_cleanups branch

August 29, 2020 17:28

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request


          Merge pull request voxpupuli#1397 from alexjfisher/mailhost_cleanups

1e1cd6c

Minor refactor of mailhost.pp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet