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

rotate must be an integer #46

Open
parhamr opened this issue Oct 20, 2014 · 17 comments
Open

rotate must be an integer #46

parhamr opened this issue Oct 20, 2014 · 17 comments

Comments

@parhamr
Copy link

parhamr commented Oct 20, 2014

The parsing of rotate appears to differ from the official documentation. In the examples, rotate is presented as a Ruby Fixnum. In the code, the match criteria appears to only work for numeric characters from a Ruby String.

This is working for me:

  logrotate::rule { 'redis_6384':
    path          => '/var/log/redis_6384.log',
    rotate        => '4',
    mail          => '',
    rotate_every  => 'week'
  }

This block returns an error:

  logrotate::rule { 'redis_6384':
    path          => '/var/log/redis_6384.log',
    rotate        => 4,
    mail          => '',
    rotate_every  => 'week'
  }

==> data: Error: Logrotate::Rule[redis_6384]: rotate must be an integer on node db01.hostname.dev

Am I misunderstanding something? Is the documentation wrong? Is the parser incorrect?

Thanks!

@CpuID
Copy link

CpuID commented Nov 18, 2014

Just for clarity here - are you using the future parser? Getting the same thing right after changing over here. Puppet 3.7.3

@CpuID
Copy link

CpuID commented Nov 18, 2014

I am seeing this specifically with the entries in manifests/defaults/debian.pp, if I change the Logrotate::rule definition to use rotate => '1' instead of rotate => 1, it works fine.

@CpuID
Copy link

CpuID commented Nov 18, 2014

just a note - d569bce fixes this issue. use master instead of 1.1.1, or @rodjek may be nice enough to arrange a version bump? :)

@jowrjowr
Copy link

jowrjowr commented Dec 4, 2014

+1 for this, I had to manually fix this. Please get this updated in forge, it would be appreciated.

Future parser stops being future Q1 2015 (aka soon).

@bastelfreak
Copy link

Hey, any progress here? could you bump the version to release the fix?

tohuwabohu added a commit to tohuwabohu/puppet-duplicity that referenced this issue Jun 27, 2015
The current implementation of the logrotate::rule is expecting an
integer as a string.

See rodjek/puppet-logrotate#46
@p--b
Copy link

p--b commented Sep 21, 2015

Just ran into this. What is stopping this from progressing to a patch release?

@p--b
Copy link

p--b commented Sep 21, 2015

(Looking at it, wouldn't a better fix than those proposed be to just do case "${rotate}" { on :302?)
The problem is that regex matches explicitly (according to the manual[0]) only match strings - so an integer (as demanded) will never match!

[0] - https://docs.puppetlabs.com/puppet/latest/reference/lang_conditional.html#case-matching

@parhamr
Copy link
Author

parhamr commented Sep 21, 2015

Yes, this was with the future parser.

@NoodlesNZ
Copy link

What's stopping this from being released into the forge? I've just run into the same problem when I started using this module today (1.1.1 from the forge).

@kontrafiktion
Copy link

Can I somehow avoid this error:

Evaluation Error: Error while evaluating a Function Call, Logrotate::Rule[wtmp]: rotate must be an integer at .../modules/logrotate/manifests/rule.pp:306:7

without using the "master". The wtmp 'Rule' is inside the logrotate module, so I do not know how to influence/overwrite it.

@p--b
Copy link

p--b commented Oct 14, 2015

Well, the maintainer really should fix their broken module.

Meanwhile, one hacky workaround:

Put the following in a file, say ~/logrotate.patch:

From 29bb3c2b64fd5c5ba718c1100d7f380b86637d13 Mon Sep 17 00:00:00 2001
From: Peter Bridgman <[email protected]>
Date: Mon, 21 Sep 2015 07:23:06 +0100
Subject: [PATCH] Fixes integer validation in Puppet 4

Case regex validation only succeeds on strings, which is somewhat
troublesome when using them to validate integers. By forcing any
passed integers to be strings, we cause integers to match the regex as
desired.
---
 manifests/rule.pp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/manifests/rule.pp b/manifests/rule.pp
index 22b5201..a8b7fe4 100644
--- a/manifests/rule.pp
+++ b/manifests/rule.pp
@@ -296,7 +296,7 @@
     }
   }

-  case $maxage {
+  case "${maxage}" {
     'undef': {}
     /^\d+$/: {}
     default: {
@@ -312,7 +312,7 @@
     }
   }

-  case $rotate {
+  case "${rotate}" {
     'undef': {}
     /^\d+$/: {}
     default: {
@@ -328,7 +328,7 @@
     }
   }

-  case $shredcycles {
+  case "${shredcycles}" {
     'undef': {}
     /^\d+$/: {}
     default: {
@@ -336,7 +336,7 @@
     }
   }

-  case $start {
+  case "${start}" {
     'undef': {}
     /^\d+$/: {}
     default: {

(SPOILER ALERT: this is just #62)

Then:

$ cd /etc/puppetlabs/code/environments/production/modules/logrotate
$ patch -i ~/logrotate.patch -p1

Yes, I know this is horrific. It also makes the problem go away for a bit.

@marcusphi
Copy link

I'm having this too.

@kontrafiktion
Copy link

I just asked Tim per EMail, if he still (wants to) maintain this repo

@hany
Copy link

hany commented Dec 15, 2015

Just ran into this as well. Any update on making a release?

@steventwheeler
Copy link

I also have this problem with v1.1.1 using the above patch works, but is not suitable for a production system.

@jonhattan
Copy link

bump

@jonhattan
Copy link

jonhattan commented Jul 27, 2016

go with this fork, approved by puppet folks https://forge.puppet.com/yo61/logrotate

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 a pull request may close this issue.