ncm-cron: cleanup and switch to 00-tqu #1124
Conversation
ncm-cron/src/main/perl/cron.pm
Outdated
| =cut | ||
|
|
||
|
|
||
| # TODO: switch to 17.3.1 for listdir |
There was a problem hiding this comment.
A reminder to fix this before merging
|
@ned21 i piushed the fix. now that 17.3 i out, i can make 17.3.1 rpms for myself |
ncm-cron/src/main/perl/cron.pm
Outdated
|
|
||
| use constant CRON_LOGFILE_SUBDIR => "/var/log/"; | ||
| # TODO: double check if this is true |
There was a problem hiding this comment.
What's the doubt here? Any chance of confirming this before merging?
There was a problem hiding this comment.
ah, yes. there is/was some LC code in here, i will have to doublecheck.
ncm-cron/src/main/perl/cron.pm
Outdated
| # NCM-CRON START: | ||
| # NCM-CRON END: | ||
| my $crondir = $osname eq "solaris" ? $CRONDIR_SOLARIS : $CRONDIR_LINUX; | ||
| my $all_files = $self->listdir($crondir, |
There was a problem hiding this comment.
hmmm, note to self: this is ugly, probably deserves it's own option (listing a dir and returning all files sounds very common for cfgmgt toolkit)
ncm-cron/src/main/perl/cron.pm
Outdated
| if ($to_unlink =~ m{^($linux_crondir/.*$cron_entry_regexp)$}) { | ||
| $to_unlink = $1; # $to_unlink is now untainted | ||
| if ($path =~ m{^($CRONDIR_LINUX/.*$cron_entry_regexp)$}) { | ||
| my $to_unlink = $1; # $to_unlink is now untainted | ||
| $self->info("Deleting $to_unlink"); | ||
| LC::Check::absence($to_unlink, file => "True"); |
There was a problem hiding this comment.
note to self: replace with CAF::Path
|
Can someone summarize what this change is about? |
|
@ttyS4 i'm doing cleanup of all components, see http://www.quattor.org/documentation/2017/02/10/components-spring17-cleaning.html |
|
@ned21 remarks addressed |
|
The Jenkins machine's SSL cert expired so I can't view the results to see why it failed |
ncm-cron/src/main/perl/cron.pm
Outdated
| # NCM-CRON END: | ||
| my $crondir = $osname eq "solaris" ? $CRONDIR_SOLARIS : $CRONDIR_LINUX; | ||
|
|
||
| # Files only. See remark about LC::Check::absence below |
There was a problem hiding this comment.
I think this is confusing. I have to search forward to find another comment (which may well get deleted in the future) to understand it. If I have understood the comment correctly then I think a better way to say it would be:
# Files only. Do not modify listdir() call to return of directories unless
# CAF::Path::cleanup has also been modified to support directories.
There was a problem hiding this comment.
ok, but cleanup should be modified to support a mode where only files are deleted, and anything else is ignored/skipped (that's what the old absence(Files => True) is about
ncm-cron/src/main/perl/cron.pm
Outdated
| if ($path =~ m{^($CRONDIR_LINUX/.*$cron_entry_regexp)$}) { | ||
| my $to_unlink = $1; # $to_unlink is now untainted | ||
| # This used to be done with LC::Check::absence(file => True) | ||
| # but this is a loop over files only, so it's safe to use CAF::Path::cleanup |
There was a problem hiding this comment.
Are the consequences of a directory ending up in here so bad that it should be explicitly checked whether it's a directory or a file before passing to cleanup?
There was a problem hiding this comment.
it would be backwards incompatible, but i don't think this is bad.
…solaris unittests)
|
@ned21 comment modified, i hope it is clearer now |
|
Thanks. I'm good with this - just need to pass the tests now :) |
|
@hpcugentbot retest this please |
|
@ned21 passes tests, are you happy to approve? |
Requires quattor/CAF#230