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

use Enum instead of validate_re for $sendfile #1664

Merged
merged 1 commit into from Aug 25, 2017
Merged

use Enum instead of validate_re for $sendfile #1664

merged 1 commit into from Aug 25, 2017

Conversation

mmoll
Copy link
Contributor

@mmoll mmoll commented Aug 6, 2017

No description provided.

@ekohl
Copy link
Collaborator

ekohl commented Aug 15, 2017

I don't see why the tests failed.

@mmoll
Copy link
Contributor Author

mmoll commented Aug 15, 2017

parallel_tests maybe? 😹

@@ -636,7 +636,7 @@
it "should fail" do
expect do
catalogue
end.to raise_error(Puppet::Error, /"foo" does not match/)
end.to raise_error(Puppet::PreformattedError, /Evaluation Error: Error while evaluating a Resource Statement, Class\[Apache\]: parameter 'sendfile' expects a match for Enum\['Off', 'On', 'off', 'on'\], got 'foo'/)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it still needed to test against those cases? In the Vox Pupuli namespace we dropped them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can delete this for sure, if wanted...

@@ -38,7 +38,7 @@
$purge_vhost_dir = undef,
$purge_vdir = false,
$serveradmin = 'root@localhost',
$sendfile = 'On',
Enum['On', 'Off', 'on', 'off'] $sendfile = 'On',

Choose a reason for hiding this comment

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

why not map On/Off, on/off to bool in puppet?

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, overly complicated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For simplification you could add a type Apache::OnOff since this validation is used in a lot of places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, if wanted.

However, that would be a separate PR, which also changes all occurrences of it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're right they shouldn't be in this PR.

@ekohl
Copy link
Collaborator

ekohl commented Aug 18, 2017

@mmoll if you rebase, do the tests pass then?

@mmoll
Copy link
Contributor Author

mmoll commented Aug 18, 2017

Tests are green now.

@mmoll
Copy link
Contributor Author

mmoll commented Aug 22, 2017

anything needed to get this in?

@eputnam eputnam merged commit 83b6b25 into puppetlabs:master Aug 25, 2017
@mmoll mmoll deleted the sendfile_re branch August 25, 2017 22:35
cegeka-jenkins pushed a commit to cegeka/puppet-apache that referenced this pull request Jul 15, 2020
use Enum instead of validate_re for $sendfile
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.

None yet

7 participants