Component: support redirection

[stdweird]

Support component redirection (module selection, spma-style)

[ned21]

Not sure if it's a case of not enough coffee yet but I find this a bit hard to follow. The POD is good but I'm missing the bigger picture, e.g. the commit log doesn't say why we are doing this, and there's nothing that tells component authors how to take advantage of it.

[ned21]

I don't understand this comment. You say there's no real point doing this but do it anyway?

[stdweird]

i'll check again. i copied this code from somewhere else in ncm-ncd

[stdweird]

the remark is wrt reporting the warnings also in the previous error block. it won't add much and i can't just put this block before the error block, as it might mess up $@. so you only see the warnings when there's no error.

[ned21]

When does this get printed? It's only verbose not warn or error so can't be too bad but what are the consequences?

[stdweird]

it get printed if you do not override Configure (so you forgot or you need REDIRECT, in which case you don't need to override Configure). sometimes you think you did set REDIRECT, but it fails, and then it is useful to know what went wrong (well, imho).

[ned21]

Feels like an artefact of using an unnatural indirection via a hash, which I think would be avoided by the above?

[stdweird]

this is some limitation of perl OO. in python this would be

class Magic(object):
    REDIRECT = None
    def moremagic(self):
        is self.REDIRECT is not None:
            return do_something(self.REDIRECT)

class Magic2(Magic):
    REDIRECT = {something else}

but you can't do this easily in perl

[ned21]

How does this work? Previously the code was only called if the subclass didn't override the Configure() method. So how does a module author use this new facility?

[stdweird]

if no redirect is defined, it still reports an error as before. that behaviour has not changed.
i'm sure i fully understand what you want to know. you can use the new facility by setting the REDIRECT variable. i can clarify the pod below if needed

[stdweird]

just to give you an idea: before you had to do https://github.com/quattor/configuration-modules-core/blob/master/ncm-spma/src/main/perl/spma.pm; now you can do https://github.com/kwaegema/configuration-modules-core/blob/5fb11b7f293a920e6c63282684a20a804e66c472/ncm-ceph/src/main/perl/ceph.pm

[ned21]

Thanks for the example, but that doesn't really help! (Or rather it helps a lot in understanding the intent of this code.) It seems very unintuitive when reading the code that the parameters in a Readonly hash relate to a profile attribute that then select a file from a subdirectory which is not even named the same as the file. (Ceph/ v ceph). It's much less code but a significantly higher barrier to anyone approaching the code to figure out what's happening.

In particular using a hash to ultimately invoke a method is unintuitive and breaks the OO concept of inheritance. I think we should keep to using inheritance or other well-understood OO concepts. e.g.

# This component usesa sub-module to select the right code to run
sub Configure {
  my ($self, $config) = @_;
  my $submod = $config->getTree("/path/to/module");
  $submod = "Luminous" unless defined $submod;
  return $self->redirect($submod);  # redirect is a method in the super class.

feels more naturally OO for not significantly more code.

[stdweird]

this has been a while, but i don't agree with your view

in your example, the Luminous and /path/to/module are proper constants and as such should be Readonly anyway; so you end up with code like

Readonly $PATH => '/path/to/module';
Readonly $DEFAULT => 'Luminous';

sub Configure {
my ($self, $config) = @_;
  my $submod = $config->getTree($PATH);
  $submod = $DEFAULT unless defined $submod;
  return $self->redirect($submod);  # redirect is a method in the super class.
}

now, what is so OO about having to copy the Configure method each time? (and for completeness, the example is missing a bunch of error handling and with fixed $PATH doesn't allow aliasing, so the full code block to copy/paste will be even bigger)
your other remarks wrt "profile attribute" and naming, that is legacy quattor code. there are no standards, i would definitley prefer not haing to do this and just use the same for all, but for some reaon our base component classes have lower case names, and i can hardly use packager as an attribute name in ceph to point out a certain version/flavour of the software.

maybe if we can settle on a proper attribute name and it's ok to have components start with uppercase, i can make the code do its magic based on single $REDIRECT_DEFAULT reaonly instead of a hashref?

[ned21]

My issue is with this being completely a data definition:

Readonly our $REDIRECT => {
    name => 'release',
    default => 'Luminous',
};

If I am trying to follow this code I am at a dead end. It's necessary to grep the code base for where $REDIRECT is used and then understand the abstraction. Compare this to my example, I can see instantly (because I am familiar with the standard OO abstraction) that I need to check the definition of the redirect function in the super-class.

[jrha]

@stdweird needs rebase