FileWriter: use atomic write #202

stdweird · 2016-11-26T18:46:24Z

Requires #204 and #206
Fixes #148
Fixes #223
Also fixes #189

File::AtomicWrite requires 0.18, rpm can be build using cpanspec -b --packager quattor File::AtomicWrite --force

stdweird · 2016-11-26T18:46:42Z

This requires testing obviously

ned21 · 2016-11-27T14:30:23Z

src/main/perl/FileWriter.pm

                $self->verbose ("Changes to $filename:");
                $self->report ($diff);
+            }
+
+            if (*$self->{options}->{noaction}) {


Is this test the right way round? i.e. this seems to be the block that's executed when noaction is true?

probably. but this commit is already outdated

ned21 · 2016-11-27T14:33:21Z

src/main/perl/FileWriter.pm

+                    $modified = 1;
+                };
+                if ($@) {
+                    $self->verbose("AtomicWrite gave error: $@");


Is this only verbose because throw_error takes care of generating the actual error? In which case debug would be more appropriate to avoid the duplicate message?

with verbose logging to file, anything interesting should be logged; and a failure to write is interesting enough imho.

also lots of code doesn't do any exception handling with FileWriter; at least in this case with verbose_logfile there's another trace where the error came from.

it's "only" verbose because we can hardly log an error here if we try to be as backwards compatible as possible (LC::Check::file also didn't report an error). the thow_error is sort-of legacy usage (afai understand, we want to die or croak in the some near future).
it might make sense to add another option to control to report an error here (default could even be true)

ned21 · 2016-11-27T14:36:51Z

src/test/resources/File/AtomicWrite.pm

+use version;
+
+# Always recent enough
+our $VERSION = 10000.0.0;


I think we should declare here the API version we are mocking. Then if a later version changes, we get a failure and get reminded to go and check we updated the mock to match the behaviour of the real thing?

ned21 · 2016-11-27T14:37:13Z

src/test/resources/File/AtomicWrite.pm

+
+=head1 SYNOPSIS
+
+This is a backup module for File::AtomicWrite.


s/backup/mock/ ?

ned21 · 2016-11-27T14:47:44Z

src/test/perl/modules/filetools.pm

+    my $fn = shift;
+    my $dir = dirname($fn);
+    mkpath $dir if ! -d $dir;
+    open(FH, ">$fn");


Bare filehandles and two element open is considered dangerous. Please use open(my $fh, ">", "$fn"); as per the Perl Best Practice Book.

ned21 · 2016-11-27T14:47:51Z

src/test/perl/modules/filetools.pm

+
+sub readfile
+{
+    open(FH, shift);


ned21 · 2016-11-27T14:50:24Z

src/test/perl/modules/filetools.pm

+    mkpath $dir if ! -d $dir;
+    open(FH, ">$fn");
+    print FH (shift || "ok");
+    close(FH);


Neither open() nor close() do any error handling which is generally bad, although given this is a unit test helper function, perhaps it's not bad in this specific case?

done. now dies with proper message

stdweird · 2016-11-28T07:00:52Z

@ned21 first round of remarks addressed 😄

stdweird · 2017-02-25T16:16:15Z

src/test/perl/modules/filetools.pm

@@ -0,0 +1,33 @@
+package filetools;


switch to new unittest Filetools module

@jrha @ned21 hmm, i can't request changes on my own PR. can you help me not to forget 😄

Don't forget to switch to new unittest Filetools module.

jrha · 2017-02-27T10:50:34Z

src/test/perl/modules/filetools.pm

@@ -0,0 +1,33 @@
+package filetools;


Don't forget to switch to new unittest Filetools module.

stdweird · 2017-03-15T09:07:32Z

@jrha Filetools used
@ned21 what changes do you still need?

jouvin · 2017-03-19T17:35:00Z

I'd like to test it to see in particular if it fixes quattor/maven-tools#139. Could we get the dependencies in quattor externals repo?

stdweird · 2017-03-20T06:07:42Z

@jouvin i mailed them to you and @jrha lasty week 😄

…quattor#189)

…d only write when needed

stdweird · 2017-03-21T07:48:13Z

@ned21 i added another commit to fix #223

ned21 · 2017-03-21T12:54:00Z

The fix to #223 is 2fee451? Can the "Fixes #223" line be added to the commit log? Also any chance of a separate PR in case this doesn't make the cut for 17.3? Then we can get the spma change merged.

stdweird · 2017-03-21T13:00:18Z

@ned21 this is not blocking the spma PR at all, the fix is just to handle the failure in cleaner way, it doesn 't fix the failure itself

…ence before testing it has a is_verbose method (Fixes quattor#223)

stdweird · 2017-03-21T13:03:21Z

@ned21 message updated

ned21 · 2017-03-23T12:06:26Z

I'm good with this PR, waiting on @jrha to confirm the changes he requested have been made.

stdweird · 2017-03-23T12:37:50Z

rpms added to externals repo

stdweird added this to the 16.12 milestone Nov 26, 2016

ned21 reviewed Nov 27, 2016

View reviewed changes

ned21 requested changes Nov 27, 2016

View reviewed changes

stdweird force-pushed the record_diff branch from 1756d4d to 6c070b8 Compare November 28, 2016 07:00

stdweird force-pushed the record_diff branch from 6c070b8 to da317a2 Compare November 28, 2016 07:19

stdweird force-pushed the record_diff branch from da317a2 to 2a3d01e Compare December 6, 2016 06:09

stdweird modified the milestones: 17.2, 16.12 Dec 6, 2016

stdweird force-pushed the record_diff branch from 2a3d01e to 3eed146 Compare February 6, 2017 19:28

stdweird modified the milestones: 17.3, 17.2 Feb 16, 2017

stdweird force-pushed the record_diff branch from 88ca493 to 519ea6f Compare February 25, 2017 15:34

stdweird commented Feb 25, 2017

View reviewed changes

jrha requested changes Feb 27, 2017

View reviewed changes

src/test/perl/modules/filetools.pm Outdated

@@ -0,0 +1,33 @@

package filetools;

Copy link

Member

jrha Feb 27, 2017

Choose a reason for hiding this comment

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

Don't forget to switch to new unittest Filetools module.

stdweird mentioned this pull request Mar 5, 2017

Mocked CAF::FileWriter attempts to create directories quattor/maven-tools#139

Closed

stdweird force-pushed the record_diff branch from 519ea6f to 0bb11f7 Compare March 15, 2017 09:03

ned21 approved these changes Mar 17, 2017

View reviewed changes

stdweird added 6 commits March 20, 2017 07:17

FileReader/Editor: cleanup header

2aae67e

FileWriter: cleanup header, prepare for AtomicWrite instead of LC::Check

f83f317

FileWriter: Restructure close code, add extra metadata to event (fixes …

348ed0c

…quattor#189)

FileWriter: use Text::Diff to determine if file is changed or not, an…

0179b8e

…d only write when needed

FileWriter: switch to File::AtomicWrite

2a58586

Rename unittests

9e1edfe

stdweird added 4 commits March 20, 2017 07:17

FileWriter: factor out _read_contents and add more unittests

b23bcf1

Various unittest issues after rebasing

0eff7bf

filereader unittest: use a file that always exists, even offline

b7b6e54

switch to Test::Quattor::Filetools writefile/readfile

7fcbe50

stdweird force-pushed the record_diff branch from 0bb11f7 to 7fcbe50 Compare March 20, 2017 06:26

stdweird mentioned this pull request Mar 21, 2017

ncm-ncd: modified event reporting under NoAction=1 with new FileWriter quattor/ncm-ncd#109

Merged

stdweird force-pushed the record_diff branch from 5ae0efc to 2fee451 Compare March 21, 2017 07:53

stdweird mentioned this pull request Mar 21, 2017

Filewriter: support sensitive option #226

Merged

FileWriter: is_verbose: make sure the log instance is a blessed refer…

9dc9cae

…ence before testing it has a is_verbose method (Fixes quattor#223)

stdweird force-pushed the record_diff branch from 2fee451 to 9dc9cae Compare March 21, 2017 13:03

jouvin mentioned this pull request Mar 22, 2017

CAF::FileWriter updates files in-place #148

Closed

stdweird mentioned this pull request Mar 23, 2017

ncm-spma: support Solaris pkg exact-install and whitelists quattor/configuration-modules-core#1036

Merged

jrha approved these changes Mar 29, 2017

View reviewed changes

jrha merged commit 4ad8514 into quattor:master Mar 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FileWriter: use atomic write #202

FileWriter: use atomic write #202

stdweird commented Nov 26, 2016 •

edited

Loading

stdweird commented Nov 26, 2016

ned21 Nov 27, 2016

stdweird Nov 27, 2016

ned21 Nov 27, 2016

stdweird Nov 28, 2016

ned21 Nov 27, 2016

stdweird Nov 28, 2016

ned21 Nov 27, 2016

stdweird Nov 28, 2016

ned21 Nov 27, 2016

ned21 Nov 27, 2016

stdweird Nov 28, 2016

ned21 Nov 27, 2016

stdweird Nov 28, 2016

stdweird commented Nov 28, 2016

stdweird Feb 25, 2017

stdweird Feb 25, 2017

jrha Feb 27, 2017

jrha Feb 27, 2017

stdweird commented Mar 15, 2017

jouvin commented Mar 19, 2017

stdweird commented Mar 20, 2017

stdweird commented Mar 21, 2017

ned21 commented Mar 21, 2017

stdweird commented Mar 21, 2017

stdweird commented Mar 21, 2017

ned21 commented Mar 23, 2017

stdweird commented Mar 23, 2017


		=head1 SYNOPSIS

		This is a backup module for File::AtomicWrite.

FileWriter: use atomic write #202

FileWriter: use atomic write #202

Conversation

stdweird commented Nov 26, 2016 • edited Loading

stdweird commented Nov 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdweird commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdweird commented Mar 15, 2017

jouvin commented Mar 19, 2017

stdweird commented Mar 20, 2017

stdweird commented Mar 21, 2017

ned21 commented Mar 21, 2017

stdweird commented Mar 21, 2017

stdweird commented Mar 21, 2017

ned21 commented Mar 23, 2017

stdweird commented Mar 23, 2017

stdweird commented Nov 26, 2016 •

edited

Loading