Skip to content
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.

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

Add stricter parameter types #1413

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Aug 30, 2020

This includes a rebase of #1189. That adds a type Nginx::Duration which looks similar to Nginx::Time (added in f0bf83a). We likely want to unify those, but right now I'm not sharp enough to see the exact differences.

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

👀

$service_restart = undef,
$service_name = 'nginx',
$service_manage = true,
Enum['running', 'absent', 'stopped', 'undef'] $service_ensure = running,
Copy link
Contributor

Choose a reason for hiding this comment

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

what's undef doing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from the previous PR. Haven't looked too closely at the content yet.

$service_flags = $nginx::service_flags,
$service_manage = $nginx::service_manage,
Optional[String[1]] $service_restart = $nginx::service_restart,
Enum['running', 'absent', 'stopped', 'undef'] $service_ensure = $nginx::service_ensure,
Copy link
Contributor

Choose a reason for hiding this comment

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

what's undef doing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In #1414 I chose Stdlib::Ensure::Service

@ekohl
Copy link
Member Author

ekohl commented Aug 31, 2020

#1414 should probably be merged first.

String $proxy_pass_error_message = 'off',
Array $server_name = [$name]
Enum['absent', 'present'] $ensure = 'present',
Variant[Enum['*'], Array[Stdlib::Ipv4], Stdlib::Ipv4] $listen_ip = '*',
Copy link
Member Author

Choose a reason for hiding this comment

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

These should be Stdlib::IP::Address::V4::Nosubnet. Same below for IPv4

@ekohl ekohl force-pushed the add_parameter_types branch 2 times, most recently from b4ad1dd to 726aaa8 Compare August 31, 2020 15:48
@ekohl ekohl marked this pull request as draft August 31, 2020 16:17
@ekohl
Copy link
Member Author

ekohl commented Aug 31, 2020

Converting to draft since I'm using Travis instead of local tests. Feel free to point out errors or mistakes, but no need to approve until it's green.

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

🙋🏻‍♀️

@@ -0,0 +1 @@
type Nginx::Duration = Pattern[/^((\d+y)?\s?(\d+M)?\s?(\d+w)?\s?(\d+d)?\s?(\d+h)?\s?(\d+m)?\s?(\d+s)?\s?(\d+ms)?\s?|\d+)$/]
Copy link
Contributor

Choose a reason for hiding this comment

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

why is there a trailing \s??

Copy link
Member Author

Choose a reason for hiding this comment

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

diffing this regex with the stricter pattern that's Nginx::Time is still on my todo list.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a commit that merges it into Nginx::Time. That also copies the Nginx docs. While it now allows a lot of additional whitespace, it's probably easier to read this way. The various suffixes can be combined and I can't see how the regex can be really improved.

@ekohl
Copy link
Member Author

ekohl commented Aug 31, 2020

Note to self:

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

Successfully merging this pull request may close these issues.

4 participants