ncm-sysconfig: Cleanup and modernise codebase #1288

jrha · 2018-06-08T16:12:10Z

As noted in #434 ncm-sysconfig will stay around for the time being, this means the code-base should be kept up-to-date with our current standards for maintainability. This should then allow us to tackle the open issues without introducing (too many) new bugs.

Fix mixed source indentation
Switch to CAF for all file interaction and declare NoAction support.
Factor out filelist read/write to seperate functions.
Add a set of basic Configure unit tests.

ned21 · 2018-06-13T12:05:41Z

ncm-sysconfig/src/main/pan/components/sysconfig/schema.pan

@@ -9,9 +9,7 @@ include 'quattor/schema';

 type component_sysconfig = {
    include structure_component
-    'files' ? string{}{}
+    'files' ? string(1..){}{}


Can you add an annotation describing the structure that is required here in more detail? Also what version of panc does this require? Please call out in the commit log since I am not sure we have got around to deploying the latest release yet.

Sure, I'll improve the docs.
It shouldn't need a recent pan compiler, string length validation has been around forever and the double nested dict comes from the original code from CERN.

I could also break the type apart to make it clearer.

jrha · 2018-06-13T13:52:24Z

@ned21 is the schema easier to follow now?

jrha · 2018-06-13T13:54:42Z

Also @stdweird is it worth me moving the pod into the pm?

stdweird · 2018-06-13T13:56:49Z

@jrha yes, it's very small anyway

stdweird · 2018-06-13T13:57:09Z

ncm-sysconfig/src/main/pan/components/sysconfig/schema.pan

 type component_sysconfig = {
    include structure_component
-    'files' ? string{}{}
+    @{


stdweird · 2018-06-13T13:58:37Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+    # previously managed by this component.  These will have to
+    # be deleted if no longer in the configuration.
+    my %filelist;
+    my $fh = CAF::FileReader->open ("$SYSCONFIGDIR/ncm-sysconfig");


add , log => $self

stdweird · 2018-06-13T13:58:48Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+    my ($self, %filelist) = @_;
+
+    # Write the list of managed configuration files.
+    my $fh = CAF::FileWriter->open("$SYSCONFIGDIR/ncm-sysconfig");


add , log => $self

stdweird · 2018-06-13T13:59:56Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+
+    # Write the list of managed configuration files.
+    my $fh = CAF::FileWriter->open("$SYSCONFIGDIR/ncm-sysconfig");
+    for my $file (keys %filelist) {


i prefer foreach (altough it's identical); but do sort the keys for reproducibility

stdweird · 2018-06-13T14:00:34Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+
+    # Loop over all of the defined files, writing each as necessary.
+    if ( $sysconfig_config->{files} ) {
+        for my $file (sort keys %{$sysconfig_config->{files}}) {


stdweird · 2018-06-13T14:02:10Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+            my $contents = '';
+
+            # Add the prologue if it exists.
+            if (defined($pairs->{"prologue"})) {


no need for " inside the {}

stdweird · 2018-06-13T14:02:49Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+
+            # Add the prologue if it exists.
+            if (defined($pairs->{"prologue"})) {
+                $contents .= $pairs->{"prologue"} . "\n";


make that "$pairs->{prologue}\n"; (at least i find that more readable)

stdweird · 2018-06-13T14:03:36Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+            }
+
+            # Loop over the pairs adding the information to the file.
+            for my $key (sort keys %{$pairs}) {


just %$pairs

stdweird · 2018-06-13T14:04:18Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+            }
+
+            # Add the epilogue if it exists.
+            if (defined($pairs->{"epilogue"})) {


same remarks as prologue

stdweird · 2018-06-13T14:04:37Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+            }
+
+            # Now actually update the file, if needed.
+            my $fh = CAF::FileWriter->open("$SYSCONFIGDIR/$file", backup=>".old");


add , log => $self, also space around the =>

stdweird · 2018-06-13T14:04:49Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+            my $fh = CAF::FileWriter->open("$SYSCONFIGDIR/$file", backup=>".old");
+            print $fh $contents;
+            $fh->close();
+            undef $fh;


what is this for?

ned21 · 2018-06-13T12:09:02Z

ncm-sysconfig/src/main/perl/sysconfig.pm

 our $EC = LC::Exception::Context->new->will_store_all;

-use LC::Check;
+use CAF::FileReader;
+use CAF::FileWriter;


Is this NoAction safe and if so does it need to be included here? (I assume PMComponent macro doesn't make that assumption.

stdweird · 2018-06-14T09:15:50Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+
+=over
+
+=item * C<< /software/components/sysconfig/files >>


this is for schema annotation

sigh fine

stdweird · 2018-06-14T09:16:03Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+This is an dict which has the file name (unescaped) as the key, and
+the content information as the value. The value is an dict.
+
+=item * C<< /software/components/sysconfig/files/<fname>/ >>


same, move to schema

stdweird · 2018-06-14T09:16:46Z

ncm-sysconfig/src/main/perl/sysconfig.pm

+
+=head1 EXAMPLE
+
+    "/software/components/sysconfig/files/scfg" = dict(


also move to schema (you can add a and @example{} after the @documentation of the main component type)

Really? This causes panc to barf on the "

It's happy with the example in the @documentation block, but not @example

what is the error message? is it similar to quattor/pan#127?
i'll try to find where i used it ...

@jrha that's strange. i'm fine with it in the @documenation section, but i thought i already used @example{} somewhere....

[INFO] --- panc-maven-plugin:10.2:check-syntax (check-generated-pan-syntax) @ sysconfig --- parse error [/home/centos/configuration-modules-core/ncm-sysconfig/target/pan/components/sysconfig/schema.pan:24.1-34.1] Encountered " <ERROR> "\' "" at line 2, column 1. Was expecting one of: <EOF> <KEY> ...

hah, so it is similar to quattor/pan#127
i made quattor/pan#204 for followup

stdweird

nice!

stdweird · 2018-06-14T10:12:51Z

ncm-sysconfig/src/main/perl/sysconfig.pm

-use File::Path;
-use File::Basename;
+Readonly my $QUOTE => "\"";
+Readonly my $SYSCONFIGDIR => "/etc/sysconfig"; # The base directory for sysconfig files.



@jrha well, almost nice. can you also add

our $NoActionSupported = 1;

(as per @ned21 request)

Ugh, I had added that already... where did it go?!

stdweird · 2018-06-14T18:03:21Z

@ned21 ok for you?
@jrha do you plan to squash any commits?

ned21 · 2018-06-14T21:14:30Z

Looks like at least two commits have been described as to be squashed prior to merge.

jrha · 2018-06-15T10:40:48Z

Indeed, I'll apply some judicious squashing.

* Switch to CAF for all file interaction and declare NoAction support. * Use logger with FileWriters * Declare no-action support * Factor out filelist read/write to seperate functions. * Add a set of basic Configure unit tests. * Move pod into main source module * Switch nlists to dicts in pod * Move example to schema and remove redundant pod

* Break apart schema into sub-types for clarification * Disallow empty values * Make use of schema in tests * Add annotations based on paraphrased description from pod.

jrha · 2018-06-15T12:03:23Z

All rebased and squashed, have at it.

jrha force-pushed the sysconfig_modernise branch from fe4e6d5 to a9dd6b5 Compare June 11, 2018 14:23

jrha requested a review from stdweird June 11, 2018 14:29

jrha force-pushed the sysconfig_modernise branch 3 times, most recently from 7af84a9 to a57762f Compare June 11, 2018 15:59

ned21 requested changes Jun 13, 2018

View reviewed changes

jrha force-pushed the sysconfig_modernise branch 2 times, most recently from d8a4d6d to a2475b2 Compare June 13, 2018 13:54

stdweird reviewed Jun 13, 2018

View reviewed changes

jrha force-pushed the sysconfig_modernise branch from a2475b2 to d6c5be9 Compare June 13, 2018 13:58

stdweird reviewed Jun 13, 2018

View reviewed changes

ned21 reviewed Jun 13, 2018

View reviewed changes

stdweird reviewed Jun 14, 2018

View reviewed changes

stdweird mentioned this pull request Jun 14, 2018

pan annotations: allow for example pan code quattor/pan#204

Open

stdweird previously approved these changes Jun 14, 2018

View reviewed changes

stdweird reviewed Jun 14, 2018

View reviewed changes

jrha dismissed stdweird’s stale review via 880e89a June 14, 2018 16:12

jrha added this to the 18.6 milestone Jun 14, 2018

jrha added the enhancement label Jun 14, 2018

ned21 previously approved these changes Jun 14, 2018

View reviewed changes

jrha added 2 commits June 15, 2018 11:41

ncm-sysconfig: Fix mixed source indentation

28a99d6

ncm-sysconfig: Update and clarify schema

aa52055

* Break apart schema into sub-types for clarification * Disallow empty values * Make use of schema in tests * Add annotations based on paraphrased description from pod.

jrha dismissed ned21’s stale review via aa52055 June 15, 2018 11:56

jrha force-pushed the sysconfig_modernise branch from 880e89a to aa52055 Compare June 15, 2018 11:56

stdweird approved these changes Jun 15, 2018

View reviewed changes

stdweird merged commit 951b1f1 into quattor:master Jun 15, 2018

jrha deleted the sysconfig_modernise branch June 15, 2018 13:32


		=head1 EXAMPLE

		"/software/components/sysconfig/files/scfg" = dict(


		=over

		=item * C<< /software/components/sysconfig/files >>

ncm-sysconfig: Cleanup and modernise codebase #1288

ncm-sysconfig: Cleanup and modernise codebase #1288

Conversation

jrha commented Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrha commented Jun 13, 2018 • edited Loading

jrha commented Jun 13, 2018

stdweird commented Jun 13, 2018

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

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 left a 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 Jun 14, 2018

ned21 commented Jun 14, 2018

jrha commented Jun 15, 2018

jrha commented Jun 15, 2018

jrha commented Jun 8, 2018 •

edited

Loading

jrha commented Jun 13, 2018 •

edited

Loading