new default null behaviour

[stdweird]

the new default null behaviour (from #175 / issue #93) cuases some issues with our templates, due to the following:

variable NODE_OS_VERSION_SITE_CONFIG ?= null;
...
variable NODE_OS_VERSION_SITE_CONFIG ?= 'site/os/config';

(obviously not in the same template).

anywhow, the 2nd default assignment is now skipped, since null is a valid default value.

?= null is used quite a lot in the core libraries it seems, so there might be quite some fallout.

i still stand by #175, but at least the template released should use ?= undef instead of ?= null

[stdweird]

@ned21 @jrha @jouvin please recompile your trees with 10.7 and let me know how bad this is.

[jrha]

I saw one issue of this, so far only in our SCDB instance. For me this is not an issue.

[jrha]

I guess this sums it up:

object template test;

variable a ?= undef;
variable a ?= null;
variable a ?= 42;

'/a' = a;

pan compiler version: 10.2
{
  "a": 42
}

pan compiler version: 10.5
{
  "a": 42
}

pan compiler version: 10.7
{}

[ned21]

We have discovered the same problem.

variable FILESYSTEM_DATADISK_FS_OPTIONS = {
    if (exists("/system/features/aquilon/filesystems/datadisk-filesystem-selector/mountopts")) {
        value("/system/features/aquilon/filesystems/datadisk-filesystem-selector/mountopts");
    } else {
        null;
    };
};

and then there is some later code that does: variable FILESYSTEM_DATADISK_FS_OPTIONS ?= ...

Fix is straight forward (s/null/undef/) and this appears to be the only instance. However we haven't tested it yet! Which means that if we have other code that relies on if( exists(VARIABLE) ) then we are in trouble because there is no longer anyway within the language to "delete" a variable.

[jouvin]

@stdweird I had no time yet to test 10.7 but I'm pretty sure it'll cause problem in some parts of the template library as we tended to use null rather than undef as a default value for variables. This has the advantage that if it is assigned to a path, it has the effect of deleting the path rather than letting it undef, something that will cause the validation to fail. But in many case, using undef should also work.

Anyway #175 has been a major behaviour change introduced in panc as I said in one of my comment in #93 (unfortunately I was not available to participate more actively in the discussion). It has always been intentional that null is considered as an undef variable (is_defined() returned false for null). And many templates use this property in a more subtle way for construct like:

variable A ?= if ( cond1 ) {
                        'x';
                      } else {
                        null;
                      };

and then later (may be in another template):

variable A ?= 'the_default';

Identifying all the occurences like this may be painful and quite difficult, as may depend on some conditions not well tested by our current configuration. I'm not sure to understand what the real use case for the change was (#93 (comment) was expected to work exactly as it worked before the change). And for me the initial example given in #93 with:

final variable TEST2 = null;
variable TEST2 ?= 'something';

producing an error is quite consistent with final behaviour. What doesn't really make sense for me is assigning null to a final variable but I probably missed a use case that you have...

It's probably too late but I'd really like to see #175 reverted until we have better understanding of the use case that may require the change and its consequences on existing templates, in particular the template library.

[jouvin]

I just gave a try to 10.7 and confirm that it doesn't work for us. It breaks boot_disk() function that returns null preventing the default to be applied in filesystem/config.pan from template-library-standard. In addition, several variables in the recently added templates for Docker also have several variables initialized to null that must be updated. This is the only things I identified in the template library so far.

We had also several site-specific templates that are broken but I managed to fix them and I didn't find a showstopper at this point...

[jouvin]

@jrha are you ready to accept the change from null to undef for the templates I identified and include them in 18.6? It would be good to fix 10.7 support in the TL, except if there is an agreement to release 10.8 shortly with the fix reverted... The PRs with the required fixes are quattor/template-library-core#183 and quattor/template-library-standard#112.

[stdweird]

we recompiled and fixed templates until the results were identical.
we had 4 issues to replace null with undef, but it was a not so straightforward exercise to get this right. i thought i opened 1 or 2 PRs to fix them, but i guess i forgot to do so (but we are not really using the core template libraries).

@ned21 wrt deleting a variable, i thought we agreed that noone needed this.

(also, for the example you have, you can use variable X = value('/path', undef);)

wrt if (exists(VARIABLE)), typically the code will have if (exists(VARIABLE) && is_defined(VARIABLE), and is_defined(VARIABLE) for a variable that doesn't exist is false, so you can drop the exists part.
unless you have code that somehow distinguishes between this. if you want to distinguish, there's also is_null (which returns false for a non-existing variable, so also no exists needed there)

[jrha]

@jouvin I have one more day in the office before holiday, so it's really too late for 18.6, I'd prefer that we ensure that the release notes make it clear that there are known issues using the library with panc 10.7.

[jouvin]

@jhra you are the boss, I let you decide! At the same time, the 2 PRs are really very minimal changes and I cannot really expect problems (in particular for the Docker one as the templates are new in 18.6).

[jrha]

We never expect problems, but yet they seem to keep occurring 😄
However, if you want to test rc1 plus the changes you have made we can discuss merging them when I'm back.

[jouvin]

I'll see what I can do. FYI, the changed in the PR have been deployed in production at LAL (which is running mainly 17.7 at this point but I don't think there was any change to these parts of the TL since then).