Add support for providing a dir to include_conf #151

hogarthj · 2016-09-14T16:13:23Z

With this commit a directory can be provided to include_conf (such as /etc/rsnapshot.d) and then each file inside (skipping files ending with .in to make the test situation easier) will be parsed into the config.

This then allows a base configuration and for whatever method is desired to drop files into this directory to add backup lines (as I imagine the common use case will be) to provide drop in configuration of targets.

This also adds testing for include_conf to ensure it works for no include_conf, with include_conf pointing to a file and include_conf pointing to a directory.

guestisp · 2016-09-20T06:24:30Z

Please include only files ending with ".conf" and not all files

hogarthj · 2016-09-20T07:59:57Z

That's a good call and is far less messier than skipping files ending in .in ;)

There's testing to ensure that the limited inclusion works as well.

bebehei

Great idea. 100 of 100 points.
Albeit I don't like adding new options to the rsnapshot.conf-file, this may be better to use as an extra option like include_dir or include_confdir
no documentation at all... ugh.
- For the current PR code, this could be painful for others, using the include_conf-option already.
- If you change the code, to use a new config-option, please document the config-option.

bebehei

No documentation

bebehei · 2016-09-21T10:00:43Z

I'm testing GH code review features currently. This is my first PR using this, sorry for the inconvenience with it.

hogarthj · 2016-09-21T10:07:06Z

Sorry about the lack of changes to docs ... I'll get that sorted with an update

I did have some thought about just extending include_conf though to handle directories ...

It's a "lightest touch" with no additional options being added.

users/admins who want to include don't need to special case including a directory versus including a conf snippet ... just use include_conf and let rsnapshot act appropriately.

It's a fairly trivial change to add the include_conf_dir option instead ... but I just wanted to present my line of thinking first.

Do you still want include_conf_dir as a separate option to include_conf ?

And no problem on using this for testing the review features :)

bebehei

Great idea. 100 of 100 points.
Albeit I don't like adding new options to the rsnapshot.conf-file, this may be better to use as an extra option like include_dir or include_confdir
no documentation at all... ugh.
- For the current PR code, this could be painful for others, using the include_conf-option already.
- If you change the code, to use a new config-option, please document the config-option.
Also hardcoding *.conf may not be the best. My personal favorite is, to pass a 2nd parameter to the option. This parameter will be a regex (or glob), which will be the regex for included files.
A regular use of this option would be: include_conf_dir /etc/rsnapshot.conf.d/ *.conf

bebehei · 2016-09-21T10:18:16Z

Sorry, I did not get your recent answer before writing the my 'review'.

It's a "lightest touch" with no additional options being added.

👍

Do you still want include_conf_dir as a separate option to include_conf ?

I don't know why, but for some reasons, yes.

users/admins who want to include don't need to special case including a directory versus including a conf snippet ... just use include_conf and let rsnapshot act appropriately.

Most ppl probably expect, that include_conf implicts reading files only.

hogarthj · 2016-09-21T10:41:00Z

That's fine - updating now ... I'm going to do a quick turnaround on the trivial addition of include_conf_dir which only looks for *.conf files ... and then there will be the final addition with the regex pattern after I'm a little happier and have done some testing around that

hogarthj · 2016-09-26T11:24:38Z

@bebehei just in case the review stuff is confusing matters (since it has it listed as changes requested)

The changes have been made - does this look good to you now or would you like any further refining?

hogarthj · 2016-10-06T20:36:12Z

@bebehei As a heads up I've filed a ticket with FESCO to take over this package in Fedora, seeing as it's well out of date there and the maintainer didn't respond to an update bug.

I'd really like to get this change in, if it's now acceptable to you, before pushing the next updates there next week :)

sgpinkus · 2016-10-07T00:23:57Z

@bebehei, @hogarthj Id like to review too please. Will do ASAP today.

rsnapshot-program.pl

sgpinkus · 2016-10-07T01:33:05Z

rsnapshot-program.pl

@@ -6716,6 +6739,28 @@ =head1 CONFIGURATION

 =back

+B<include_conf_dir>   Include any files in this directory at this point, defaults ending in .conf


B<include_conf_dir [conf-file-pattern=*\.conf]> would be better?

sgpinkus · 2016-10-07T01:35:00Z

rsnapshot-program.pl

@@ -6716,6 +6739,28 @@ =head1 CONFIGURATION

 =back

+B<include_conf_dir>   Include any files in this directory at this point, defaults ending in .conf


Include any files in this directory at this point, defaults ending in .conf

Not very clear. It's not including "any files". Suggest replace with:

Include any files in this directory at this point. By default only files ending in .conf are included.

sgpinkus · 2016-10-07T01:41:50Z

t/include_conf/include_conf.t.in

+use SysWrap;
+
+#
+# Write here in brief, what you want to check!


Um what? Your supposed to replace that with your own doc describing your test...

And delete the rest of the instructional comments..

sgpinkus · 2016-10-07T01:42:33Z

t/include_conf_dir/include_conf_dir.t.in

+use SysWrap;
+
+#
+# Write here in brief, what you want to check!


sgpinkus · 2016-10-07T01:50:22Z

rsnapshot-program.pl

+
+=over 4
+
+This is also recursive configuration so it's important to be careful about what snippets


Why is it "snippets" now? They're just included configuration files. Also suggest change to "... so its important included files don't cause an infinite loop".

sgpinkus · 2016-10-07T01:58:34Z

Sorry for the noisy commenting - I thought I was doing a review but I was just commenting... LOL.

Also need to squash your commits, down to one or two at most.

Also note, rsnapshot will blow up on unrecognized options. Thus any new option is essentially not BWC. Pretty sure new options have been added since that config_version check was implemented though. Not sure what the policy is for bumping the config_version version...

hogarthj · 2016-10-26T08:13:52Z

Thanks for the comments/review (although I only just saw them, oddly I didn't get emailed) ... will make the amendments and squash the commits down in the next few days

… within

hogarthj · 2016-11-15T14:25:45Z

Apologies for this taking a while to come back to but life got in the way a bit ...

I've made the changes you suggested, though i'm not sure what CS issue you were referring to before as the code was using tabs for spacing?

As a heads up on an unrelated note Fedora now has 1.4.2 and we'll be pushing the release to EPEL soon.

hogarthj · 2016-12-06T08:59:16Z

@bebehei @sam-at-github any further changes requested?

sgpinkus · 2016-12-08T00:45:33Z

@hogarthj I just pushed the CS fixes I was talking about - not sure why you couldn't see what I was talking about. One more thing: you tests are setup wrong which is why you had to add all those specific pattern to the .gitignore. I reverted the .gitignore. Please observe how other tests are setup and follow that.

hogarthj · 2016-12-08T11:13:22Z

@sam-at-github whitespace blindness? or blame fatigue caused by my daughter? ;)

Thanks for that - no idea how my eyes glossed over it.

I tried to follow another test that was similar, which was what resulted in that. I'll take another look and clear it up to avoid the noise in gitignoe.

simbalion · 2018-04-10T09:45:43Z

Any chance this could get merged? It would be very useful for automating large operations.

DrHyde · 2020-02-28T12:15:23Z

rsnapshot-program.pl

+					$conf_pat = '.*\.conf$';
+				}
+				opendir(CONF_DIR, $value) or die $!;
+				while (my $conf_file = readdir(CONF_DIR)) {


I believe that this will go all weird if any of the files in the directory themselves include an include_conf_dir, as when it recurses it will re-use the CONF_DIR handle. Please either test this to prove it doesn't happen, or read the entire directory into an array and then close it before processing the list of files, or use a lexically scoped handle.

hogarthj force-pushed the include_dir branch from 58a9e59 to 791752b Compare September 14, 2016 16:18

bebehei suggested changes Sep 21, 2016

View reviewed changes

bebehei approved these changes Sep 21, 2016

View reviewed changes

bebehei suggested changes Sep 21, 2016

View reviewed changes

sgpinkus reviewed Oct 7, 2016

View reviewed changes

rsnapshot-program.pl Outdated Show resolved Hide resolved

sgpinkus reviewed Oct 7, 2016

View reviewed changes

rsnapshot-program.pl Outdated Show resolved Hide resolved

sgpinkus reviewed Oct 7, 2016

View reviewed changes

t/include_conf_dir/include_conf_dir.t.in Outdated

use SysWrap;

#

# Write here in brief, what you want to check!

Copy link

Member

sgpinkus Oct 7, 2016

Choose a reason for hiding this comment

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

Dido.

sgpinkus reviewed Oct 7, 2016

View reviewed changes

Add support for providing a dir to include_conf and reading the files…

b6d7bf8

… within

hogarthj force-pushed the include_dir branch from 6e304e5 to b6d7bf8 Compare November 15, 2016 14:23

Fix CS issues. Remoev .gitignore noise.

a698935

DrHyde reviewed Feb 28, 2020

View reviewed changes

sgpinkus added feature incomplete labels Jun 3, 2021

DrHyde force-pushed the master branch from f662aed to 5ef827f Compare December 21, 2021 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for providing a dir to include_conf #151

Add support for providing a dir to include_conf #151

hogarthj commented Sep 14, 2016

guestisp commented Sep 20, 2016

hogarthj commented Sep 20, 2016

bebehei left a comment

bebehei left a comment

bebehei commented Sep 21, 2016

hogarthj commented Sep 21, 2016

bebehei left a comment

bebehei commented Sep 21, 2016

hogarthj commented Sep 21, 2016

hogarthj commented Sep 26, 2016

hogarthj commented Oct 6, 2016

sgpinkus commented Oct 7, 2016

sgpinkus Oct 7, 2016

sgpinkus Oct 7, 2016

sgpinkus Oct 7, 2016

sgpinkus Oct 7, 2016

sgpinkus Oct 7, 2016

sgpinkus Oct 7, 2016

sgpinkus commented Oct 7, 2016 •

edited

hogarthj commented Oct 26, 2016

hogarthj commented Nov 15, 2016

hogarthj commented Dec 6, 2016

sgpinkus commented Dec 8, 2016

hogarthj commented Dec 8, 2016

simbalion commented Apr 10, 2018

DrHyde Feb 28, 2020

		@@ -6716,6 +6739,28 @@ =head1 CONFIGURATION

		=back

		B<include_conf_dir> Include any files in this directory at this point, defaults ending in .conf


		=over 4

		This is also recursive configuration so it's important to be careful about what snippets

Add support for providing a dir to include_conf #151

Are you sure you want to change the base?

Add support for providing a dir to include_conf #151

Conversation

hogarthj commented Sep 14, 2016

guestisp commented Sep 20, 2016

hogarthj commented Sep 20, 2016

bebehei left a comment

Choose a reason for hiding this comment

bebehei left a comment

Choose a reason for hiding this comment

bebehei commented Sep 21, 2016

hogarthj commented Sep 21, 2016

bebehei left a comment

Choose a reason for hiding this comment

bebehei commented Sep 21, 2016

hogarthj commented Sep 21, 2016

hogarthj commented Sep 26, 2016

hogarthj commented Oct 6, 2016

sgpinkus commented Oct 7, 2016

sgpinkus Oct 7, 2016

Choose a reason for hiding this comment

sgpinkus Oct 7, 2016

Choose a reason for hiding this comment

sgpinkus Oct 7, 2016

Choose a reason for hiding this comment

sgpinkus Oct 7, 2016

Choose a reason for hiding this comment

sgpinkus Oct 7, 2016

Choose a reason for hiding this comment

sgpinkus Oct 7, 2016

Choose a reason for hiding this comment

sgpinkus commented Oct 7, 2016 • edited

hogarthj commented Oct 26, 2016

hogarthj commented Nov 15, 2016

hogarthj commented Dec 6, 2016

sgpinkus commented Dec 8, 2016

hogarthj commented Dec 8, 2016

simbalion commented Apr 10, 2018

DrHyde Feb 28, 2020

Choose a reason for hiding this comment

sgpinkus commented Oct 7, 2016 •

edited