This is implemented by upstream PR https://github.com/phracek/preupgrade-assistant/pull/108

This is not implemented fully:

 *  preupgrade hooks are ignored completely,

 *  second file/dir is ignored eg.

        deploy_hook postupgrade foo bar

    saves only `foo` (as run_hook), but ignores `bar`

Also, the directory naming is a bit strange:

    /root/preupgrade/hooks/xccdf_api_deploy_hook

Directory is named `xccdf_api_deploy_hook`, but that is not the module's XCCDF name but some pseudo-version of it.  This makes it harder to do reliable automated test, since one has to "guess" the module folder instead of just kowing it.

Also there's no error handling;  I can provide

 *  no second argument (script name)
 *  non-existent file or directory
 *  or directory where script is expected
 *  unsupported hook type

and the function just silently passes.

Preupgrade-assistant sends to a module value called XCCDF_VALUE_MODULE_NAME wich is used for deploying. It seems, like you modules is called api_deploy_hook which is correct. Basically, it is a modules name in your directory tree.

You note about bar is not copied is correct.
But as Bash as Python should have the same syntax.
It means:
    deploy_hook postupgrade myhook.sh
    cp myhook.sh /root/preupgrade/hooks/xccdf_foo/postupgrade/run_hook

This works.

This part is a pretty problematic
    deploy_hook preupgrade myhook.sh myfile myfolder

Nowadays, I don't have an idea how to do it from python code.

It needs a time for analysis.

(In reply to Petr Hracek from comment #8)
> Preupgrade-assistant sends to a module value called XCCDF_VALUE_MODULE_NAME
> wich is used for deploying.  It seems, like you modules is called
> api_deploy_hook which is correct.  Basically, it is a modules name in your
> directory tree.

No, it's not.

 *  XCCDF_VALUE_MODULE_NAME         is 'api_deploy_hook'
 *  rule id (only one seen in HTML) is 'xccdf_preupg_rule_api_deploy_hook_check
 *  folder name                     is 'xccdf_api_deploy_hook'

Which is bad for testing as we have to know both names ("rule id" to reach into XML/HTML and "folder name" to reach for hook artifacts in logs) for every single pre/post upgrade hook test we ever write.

Ideally there should be only one way to name a module, so why not use rule id?  It's already used in XML, HTML reports and preupg --list-rules, so I think it's only consistent to use it here as well.

In fact, when I saw in your code that you are prefixing 'xccdf_', I figured that this is what you want to do (I still can't think of any other reason; I'm sure you did not intend to create a new mix-match name but you effectively did).


> You note about bar is not copied is correct.
> But as Bash as Python should have the same syntax.

I don't understand.  This has nothing to do with Python; I only tested Bash API so far and it's broken.

(Of course Bash and Python should have the same syntax.  Of course this should work the same way in both.)



> It means:
>     deploy_hook postupgrade myhook.sh
>     cp myhook.sh /root/preupgrade/hooks/xccdf_foo/postupgrade/run_hook
> 
> This works.

Yes, and it's just about the only case that works ;)  Any other use case I throw at it fails.


> This part is a pretty problematic
>     deploy_hook preupgrade myhook.sh myfile myfolder
> 
> Nowadays, I don't have an idea how to do it from python code.

I don't see why it should be problematic; it's nothing but copying couple of files and folders.  What about `shutil.copytree`?  Or even if you had to use `Popen`, I can't see what's the challenge here.

(Maybe I'll send PR if I manage to shove it into spacetime.)

Result from current version (preupgrade-assistant-2.1.9-5.el6), Bash API:

deploy_hook() does not deploy anything at all.  /preupgrade/hooks is always empty and following message gets logged:

    Module path is not specfied.

I can't test Python API, but from the code:

    try:
        MODULE_PATH = os.environ['XCCDF_VALUE_MODULE_PATH']
    except KeyError:
        MODULE_PATH = "MODULE_PATH"

...

        hook_dir = "%s/hooks/%s/%s" % (VALUE_TMP_PREUPGRADE, MODULE_PATH, deploy_name)

I'm assuming the same scenario is not possible.  Instead:

 *   if XCCDF_VALUE_MODULE_PATH hasn't been set, string literal 'MODULE_PATH'
     is used, which means that hooks of the same types will get overwritten
     by each other under eg. `hooks/MODULE_PATH/preupgrade`

 *   if XCCDF_VALUE_MODULE_PATH was set to empty string (or any other string
     not specific for a module), same thing happens, just under eg.
    `hooks//preupgrade`.

Another issue from code review:

Neither Bash nor Python API take into account possibility of the same module calling the function twice.  In both cases, the function will try to overwrite the previous content of the hook dir, possibly merging directories in bad way.

Now the question is; should we support more hooks per module?  I think we can get away with not supporting that; it's better to avoid need to solve the naming conflict (we would probably have to number the directories or something) and if someone really needs to launch N scripts, they can always launch them from one hook.

So, possibilities if the taeget dir is already 'occupied' (ie. hook_dir already exists):

 *  (a) always throw error and don't copy anything,

 *  (b) always rm -rf hook_dir silently,

 *  (c) always rm -rf hook_dir silently but warn that the previous hook has been
    removed.

Whatever we do, we need to document that; if we do state it clearly, we can get away with (b) which is the simplest (you don't even need `if`; just `rm -rf "$hook_dir"; mkdir -p "$hook_dir"`).

Also, in Bash variable references are not quoted properly.  Arguments with spaces (or starting with '-') will break the whole thing.

(In reply to Alois Mahdal from comment #15)
> Result from current version (preupgrade-assistant-2.1.9-5.el6), Bash API:
> 
> deploy_hook() does not deploy anything at all.  /preupgrade/hooks is always
> empty and following message gets logged:
> 
>     Module path is not specfied.

MODULE_PATH is specified by XML file. all-xccdf.xml file provides this information.

(In reply to Alois Mahdal from comment #17)
> Another issue from code review:
> 
> Neither Bash nor Python API take into account possibility of the same module
> calling the function twice.  In both cases, the function will try to
> overwrite the previous content of the hook dir, possibly merging directories
> in bad way.
> 
I don't see any benefit, why it should be called twice. 
I would prefer to call hook and exit.

> Now the question is; should we support more hooks per module?  I think we
> can get away with not supporting that; it's better to avoid need to solve
> the naming conflict (we would probably have to number the directories or
> something) and if someone really needs to launch N scripts, they can always
> launch them from one hook.

No please do not complicate it. I would like to support on hook per module.

> 
> So, possibilities if the taeget dir is already 'occupied' (ie. hook_dir
> already exists):

This should not happened at all. Full directory in "hook_system" is identified by MODULE_PATH variable
which is specified in all-xccdf.xml file.

> 
>  *  (a) always throw error and don't copy anything,
> 
>  *  (b) always rm -rf hook_dir silently,
> 
>  *  (c) always rm -rf hook_dir silently but warn that the previous hook has
> been
>     removed.
> 
> Whatever we do, we need to document that; if we do state it clearly, we can
> get away with (b) which is the simplest (you don't even need `if`; just `rm
> -rf "$hook_dir"; mkdir -p "$hook_dir"`).

(In reply to Petr Hracek from comment #20)
> (In reply to Alois Mahdal from comment #17)
> > Another issue from code review:
> > 
> > Neither Bash nor Python API take into account possibility of the same module
> > calling the function twice.  In both cases, the function will try to
> > overwrite the previous content of the hook dir, possibly merging directories
> > in bad way.
> > 
> I don't see any benefit, why it should be called twice. 

Neither do I but it can happen; that's the point.


> > Now the question is; should we support more hooks per module?  I think we
> > can get away with not supporting that; it's better to avoid need to solve
> > the naming conflict (we would probably have to number the directories or
> > something) and if someone really needs to launch N scripts, they can always
> > launch them from one hook.
> 
> No please do not complicate it. I would like to support on hook per module.

Nice to know you agree with me ;)


> > So, possibilities if the taeget dir is already 'occupied' (ie. hook_dir
> > already exists):
> 
> This should not happened at all. Full directory in "hook_system" is
> identified by MODULE_PATH variable
> which is specified in all-xccdf.xml file.

As I said above, if someone calls the function once and then again, we have to deal with it.

It's not a question of what should and what should not happen; we have to define behavior for all (usual, at least) cases.

Well, it's better but still not complete:

 *  No quoting of arguments:  arguments with spaces or other special characters
    will break it.

 *  Non-existent files or dirs (except the hook scripts) are ignored.

 *  It's not clear on which error conditions the function should just print
    error and when it should exit_error.

    I would expect that any error conditions should "crash" the module,
    but exit_error is only called in two cases (double call or bad type).

I've sent PR to Github that attempts to solve all above issues.

~

Also, after reviewing Python function, I believe it's not capable of copying files if they are specified by absolute path:

    if os.path.isdir(arg):
        shutil.copytree(os.path.join(VALUE_CURRENT_DIRECTORY, arg),
                        os.path.join(hook_dir))
    else:
        shutil.copyfile(os.path.join(VALUE_CURRENT_DIRECTORY, arg),
                        os.path.join(hook_dir, arg))

I haven't tested it, but I believe this join will change meaning of an absolute path.

(Frankly, I have no idea why the os.path.join() is even used, although I admit I haven't studied shutil closely.)

Finally the Bash version of the function passes my test.

But still, in the Python version, I can see the problem with joining paths hasn't been solved.

Can you confirm it's OK and you have test coverage for that?

This could be solved during next path.
In case of using absolute path then we will have a trouble. 
Test coverage will be add later on to upstream.

I would suggest to close it. We have a small set of modules which we can fix it during next release.

joining path works fine (tested) except absolute paths.

Wait, are you suggesting to postpone the whole RFE, now when it's 'almost' done (well, the Bash part *is* done).

That seems a bit harsh, given that most of it works.  If you're really unable to fix the issue with absolute path, I suggest just add a note to the doc and fix only the abspath issue in next cyle.

Given that no modules actually use this now, I think we could get away with it.

(But frankly, I don't see how that fix could be complicated, as I have pointed out several times, there's just no need for `os.path.join(VALUE_CURRENT...)`, just remove it and problem is gone ;).  I might send PR if I find out how to create a Python module and test this.)

Of course not. This RFE has to be solved during this release cycle.

But absolute path can be solved either later on or only in upstream, co we can close this bugzilla.

Or even better solution. Alone hook system is used by module itself.
And I don't see any reason why to copy a file from directory which is not related to module itself. E.g. /root, /var/ etc.

Therefore If argument is a file or directory like name with absolute path, then it has to finish with an error status like unsupported behavior.

Deploy_hook API function supports also absolute paths by commit https://github.com/upgrades-migrations/preupgrade-assistant/commit/cda440d13462f53afab2241e8e6d109d98cfd78e

Corresponding dist-git commit: http://pkgs.devel.redhat.com/cgit/rpms/preupgrade-assistant/commit/?h=extras-rhel-6&id=69f342fa09e918d8cf0344d0be3189611167a003

Tested with preupgrade-assistant-2.1.10-1.el6.

Bash version works fine.

The Python version still has problems.

 *  every argument after 2nd one will cause deploy_hook emit an error

    filed as bug 1378205

    https://github.com/upgrades-migrations/preupgrade-assistant/pull/149

 *  argument parsing is broken; it will traceback if mandatory args are
    missing

    filed as bug 1378215

    https://github.com/upgrades-migrations/preupgrade-assistant/pull/150

 *  error message on bad hook type is malformed

    (not filed downstream)
    https://github.com/upgrades-migrations/preupgrade-assistant/pull/151

 *  hook files are deployed to incorrect path

    filed as bug 1378219

    https://github.com/upgrades-migrations/preupgrade-assistant/pull/152

Most of these don't look like blockers, so I'm closing this; if any of above need to be fixed, let's escalate the respective bugs.

(I've also tested with the PRs appied; all seems fine.)

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2016-2616.html