Skip to content

Support ports as integers#315

Merged
truthbk merged 1 commit intoDataDog:masterfrom
alex-harvey-z3q:also_support_ports_as_integers
Mar 22, 2017
Merged

Support ports as integers#315
truthbk merged 1 commit intoDataDog:masterfrom
alex-harvey-z3q:also_support_ports_as_integers

Conversation

@alex-harvey-z3q
Copy link
Copy Markdown
Contributor

Without this patch applied, compilation fails if integer data for ports
(proxy_port, pup_port etc) is passed in.

This is a problem, because it requires us to quote ports in Hiera, and
is a surprising and unexpected behaviour.

After this patch is applied, it is possible to specify these ports
either as a string or an integer.

Tests are added for each port showing that integer or string inputs both
lead to the same configuration in the catalog.

@alex-harvey-z3q alex-harvey-z3q force-pushed the also_support_ports_as_integers branch from 1de3090 to edc7acd Compare March 21, 2017 14:39
@truthbk
Copy link
Copy Markdown
Member

truthbk commented Mar 21, 2017

Thanks for the contribution @alexharv074. Indeed having to specify ports as strings may be a bit misleading and unexpected. I do appreciate that you have implemented a backward compatible approach. The PR looks good to me, the tests are currently failing not totally sure why at a quick glance. I'll take a better look in a bit.

Copy link
Copy Markdown
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

Found a couple of things. I tried these changes locally and the tests passed with PUPPET_VERSION=4.2.1 (didn't try the full matrix however).

Comment thread manifests/init.pp Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will probably have to look like this to pass the linter:

  # lint:ignore:only_variable_string
  $_statsd_forward_port = "${statsd_forward_port}"
  $_proxy_port = "${proxy_port}"
  $_graphite_listen_port = "${graphite_listen_port}"
  $_listen_port = "${listen_port}"
  $_pup_port = "${pup_port}"
  $_syslog_port = "${syslog_port}"
  # lint:endignore

Comment thread manifests/init.pp Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo here (two underscores). This should be:
validate_re($_graphite_listen_port, '^\d*$')

@alex-harvey-z3q alex-harvey-z3q force-pushed the also_support_ports_as_integers branch 2 times, most recently from ddee3ed to b85e9ae Compare March 22, 2017 02:00
@alex-harvey-z3q
Copy link
Copy Markdown
Contributor Author

Great, I updated the PR with your changes.

Without this patch applied, compilation fails if integer data for ports
(proxy_port, pup_port etc) is passed in.

This is a problem, because it requires us to quote ports in Hiera, and
is a surprising and unexpected behaviour.

After this patch is applied, it is possible to specify these ports
either as a string or an integer.

Tests are added for each port showing that integer or string inputs both
lead to the same configuration in the catalog.
@alex-harvey-z3q alex-harvey-z3q force-pushed the also_support_ports_as_integers branch from b85e9ae to 554796b Compare March 22, 2017 02:13
@alex-harvey-z3q
Copy link
Copy Markdown
Contributor Author

I also fixed up some whitespace issues (spaces at end of line, unwanted spaces between blocks etc).

Copy link
Copy Markdown
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

Looks good! Merging!

@truthbk truthbk merged commit 2b3248a into DataDog:master Mar 22, 2017
@alex-harvey-z3q alex-harvey-z3q deleted the also_support_ports_as_integers branch March 23, 2017 07:34
@truthbk truthbk added this to the 1.10.0 milestone Apr 20, 2017
cegeka-jenkins pushed a commit to cegeka/puppet-datadog_agent that referenced this pull request Jan 31, 2018
Without this patch applied, compilation fails if integer data for ports
(proxy_port, pup_port etc) is passed in.

This is a problem, because it requires us to quote ports in Hiera, and
is a surprising and unexpected behaviour.

After this patch is applied, it is possible to specify these ports
either as a string or an integer.

Tests are added for each port showing that integer or string inputs both
lead to the same configuration in the catalog.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants