[RFC] Create a hook placeholder #5757

b10n1k · 2024-07-11T12:19:46Z

perl -I lib script/openqa-cli api --hook-scripts twdata --pretty --host http://127.0.0.1:9526 -X POST isos FOO=BARRR DISTRI=Awesome

will get hook/twdata.json and will merge the variables with the ones from stdin.
ofc location and dir name is under consideration. And also I thinkit is possible to extend it to run an actual script if the source is some program

Martchus

The commit message should have details (similar to what you have in the PR description) and a ticket reference. But don't just re-use the PR description. It is full of casing and punctuation issues and too informal (avoid abbreviations like "ofc"). Please generally put a bit more though into your wording.

The commit message subject should also state the why (and not just the what).

I must say it would be nice if we would not have to repeat how to write commit messages that often.

Martchus · 2024-07-11T13:37:43Z

lib/OpenQA/CLI/api.pm

@@ -26,16 +26,30 @@ sub command ($self, @args) {
      'd|data=s' => \$data,
      'f|form' => \my $form,
      'j|json' => \my $json,
+      'H|hook-scripts=s' => \my $hooksrc,


Here it is --hook-scripts and in the example before just --hook-script. I guess it should be the latter because being able to invoke one script must be enough.

Martchus · 2024-07-11T13:40:04Z

lib/OpenQA/CLI/api.pm

-    my $params = $form ? decode_json($data) : $self->parse_params(\@args, \@param_file);
+    my $params;
+    my @data;
+    if ($hooksrc) {


I don't think the presence of a hook script should cause the tool to completely ignore the presence of a data file. If both options are not combinable (but probably they should be combinable) this should lead to a clear error/warning.

Martchus · 2024-07-11T13:46:25Z

lib/OpenQA/CLI/api.pm

+    my @data;
+    if ($hooksrc) {
+	# if $hooksrc is a json  
+        my $hookdata = path("hook/$hooksrc.json")->slurp;


This is very unintuitive. Let's keep it simple and not "surround" the specified path with hook/….json.

Just invoke the specified hook script reading its stdout. Abort with a clear error if it exited with a non-zero return code. Otherwise, parse the output as JSON. There's no reason to involve an intermediate .json file that must be placed according to some conventions.

Note that you can use the quote like operator qx(…) in Perl to invoke a script and get its output. When using it, have a close look at the documentation because error handling is a bit involved (but should be done correctly to get a clear error message in any case).

Martchus · 2024-07-11T13:47:16Z

lib/OpenQA/CLI/api.pm

+    if ($hooksrc) {
+	# if $hooksrc is a json  
+        my $hookdata = path("hook/$hooksrc.json")->slurp;
+	$data = ($hookdata);


Please avoid needless copies. The use of parenthesis here makes no sense as well.

Martchus · 2024-07-11T13:51:11Z

lib/OpenQA/CLI/api.pm

    -d, --data <string>           Content to send with request, alternatively
                                  you can also pipe data to openqa-cli
    -f, --form                    Turn JSON object into form parameters
        --host <host>             Target host, defaults to http://localhost
+    -H, --hook-scripts            Use a hook script!! TDB


I guess you mean TBD but more importantly the parameter name should most likely be:

Suggested change

-H, --hook-scripts Use a hook script!! TDB

-H, --hook-script Use a hook script!! TBD

This parameter is not needed

'pre-script-hook=s' => \my $prehooksrc, 'post-script-hook=s' => \my $posthooksrc,

during the session we also spoke about not having short options for these.

pre-script-hook should be able to run before everything else on the call as requested. post-script-hook should run and modify the data which pass to the API call. https://progress.opensuse.org/issues/162515 Signed-off-by: ybonatakis <[email protected]>

Martchus

The code is too chaotic at this point to do a real review.

Martchus · 2024-07-15T09:58:46Z

lib/OpenQA/CLI/api.pm

+      'pre-script-hook=s' => \my $prehooksrc,
+      'post-script-hook=s' => \my $posthooksrc,


Do we really need a pre and a post hook? Was this idea the outcome of your programming session with @foursixnine?

See motivation part of https://progress.opensuse.org/issues/161168

Martchus · 2024-07-15T10:05:06Z

lib/OpenQA/CLI/api.pm

+    eval {
+	print 'TST ';
+	# https://stackoverflow.com/a/54500161/1462096
+	#my @out = qx($hook_path);


I'm not a fan of IPC::Run. You may use it but you can still also just keep using Perl built-ins, e.g. like I did in one of our scripts: https://github.com/os-autoinst/scripts/blob/b4f88b41c51ef5571e3f96764a99acd43252243c/openqa-clone-and-monitor-job-from-pr#L154

That's IPC::Run3 though (I guss you meant to comment 3 lines below?)

Why not pick one implementation and run with it? (also to remove the dependency on Mojo::IOLoop::ReadWriteProcess perhaps moving that _run_cmd_capturing_output (and similar ones) can be moved to a single place in some other PR.

Looks like the same thing is being implemented over and over...

perlpunk · 2024-07-15T10:25:51Z

lib/OpenQA/CLI/api.pm

+	my ($out, $err);
+	my @out = run3([$hook_path, $data], \$out, \$err);


Suggested change

my ($out, $err);

my @out = run3([$hook_path, $data], \$out, \$err);

my @out = run3([$hook_path, $data], \my $out, \my $err);

okurz

ok, good start. How would discovering and reading hook scripts from an outside place like https://github.com/os-autoinst/openqa-trigger-from-obs look like and what would the hook script accomplish what is not already possible?

Martchus · 2024-07-15T12:58:10Z

and what would the hook script accomplish what is not already possible?

I think nothing but that's also not the point. As I understood it, the idea of the hook script is not about accomplishing something that's not otherwise possible. Of course you can simply invoke openqa-cli with the parameters you need.

I think the hook script is about allowing to invoke openqa-cli with a static set of parameters but still have some parameters dynamically computed after all. So this is just for convenience - so that we don't have to do bigger changes to the architecture of https://github.com/os-autoinst/openqa-trigger-from-obs.

okurz · 2024-07-15T14:43:25Z

[...] So this is just for convenience - so that we don't have to do bigger changes to the architecture of https://github.com/os-autoinst/openqa-trigger-from-obs.

Sorry, I don't see the point in this. In the end we expect that some power users are able to tweak the test schedule based on logic that can be maintained without needing direct changes to os-autoinst nor openQA. I don't yet see how this openqa-cli extension enables that

foursixnine · 2024-07-16T10:19:48Z

lib/OpenQA/CLI/api.pm


  Options:
        --apibase <path>          API base, defaults to /api/v1
        --apikey <key>            API key
        --apisecret <secret>      API secret
    -a, --header <name:value>     One or more additional HTTP headers
-    -D, --data-file <path>        Load content to send with request from file
+    -D, --data-file <path>        Load content to send with request from file.
+                                  Works with when -f is provided


-f is needed for -D to work? if so, make it clear (i guess is missing doc?)

Suggested change

Works with when -f is provided

Works with when -f is provided

foursixnine · 2024-07-16T10:37:51Z

lib/OpenQA/CLI/api.pm

+	use Data::Dumper;
+	# say for path('/home/sri/myapp')->list->each;
+	# run_script check exec permissions
+	$self->_run_script ($_, $data[1]) for path($posthooksrc)->list->grep(qr/post_/)->each;


something important here, is to guarantee execution order, and document it

for i in 1 9 10 19 29 21 01 09 20 100 200 101 110 109 199;do touch {pre_$i,post_$i}; done; foursixnine@pakhet test-sort % perl -Mojo -E 'say $_ for f->path->list->each' /Users/foursixnine/tmp/test-sort/post_01 /Users/foursixnine/tmp/test-sort/post_09 /Users/foursixnine/tmp/test-sort/post_1 /Users/foursixnine/tmp/test-sort/post_10 /Users/foursixnine/tmp/test-sort/post_100 /Users/foursixnine/tmp/test-sort/post_101 /Users/foursixnine/tmp/test-sort/post_109 /Users/foursixnine/tmp/test-sort/post_110 /Users/foursixnine/tmp/test-sort/post_19 /Users/foursixnine/tmp/test-sort/post_199 /Users/foursixnine/tmp/test-sort/post_20 /Users/foursixnine/tmp/test-sort/post_200 /Users/foursixnine/tmp/test-sort/post_21 /Users/foursixnine/tmp/test-sort/post_29 /Users/foursixnine/tmp/test-sort/post_9 /Users/foursixnine/tmp/test-sort/pre_01 /Users/foursixnine/tmp/test-sort/pre_09 /Users/foursixnine/tmp/test-sort/pre_1 /Users/foursixnine/tmp/test-sort/pre_10 /Users/foursixnine/tmp/test-sort/pre_100 /Users/foursixnine/tmp/test-sort/pre_101 /Users/foursixnine/tmp/test-sort/pre_109 /Users/foursixnine/tmp/test-sort/pre_110 /Users/foursixnine/tmp/test-sort/pre_19 /Users/foursixnine/tmp/test-sort/pre_199 /Users/foursixnine/tmp/test-sort/pre_20 /Users/foursixnine/tmp/test-sort/pre_200 /Users/foursixnine/tmp/test-sort/pre_21 /Users/foursixnine/tmp/test-sort/pre_29 /Users/foursixnine/tmp/test-sort/pre_9

I'm also wondering whether it makes sense to run multiple scripts. I guess I would not even have separate pre/post scripts. Or can you explain what the use case of those will be?

Or can you explain what the use case of those will be?

pre_hook: Add very_expensive_testsuite if repodata contains $_SPECIAL_STUFF
post_hook: turn on very_expensive_machine if repodata contains $_SPECIAL_STUFF

one instance of such actions could be spinning up a Grace Hopper machine, or making an api call to a third service

I would expect that the scripts are atomic and independent. I can think of some implications. What happens if one script overwrites previous variables for example.

regarding the order: list() does a sort, so the order is guaranteed. it's alphanumeric, so if you want to order by number it probably makes sense to have the same number of digits, e.g. post_001, post_002 etc.

foursixnine · 2024-07-16T10:38:19Z

lib/OpenQA/CLI/api.pm

      'param-file=s' => \my @param_file,
      'r|retries=i' => \my $retries,
      'X|method=s' => \(my $method = 'GET');

    @args = $self->decode_args(@args);
    die $self->usage unless my $path = shift @args;

+    my $params;
+    my @data;
+    if ($prehooksrc) {


Suggested change

if ($prehooksrc) {

if ($pre_hooks_dir) {

foursixnine · 2024-07-16T10:38:59Z

lib/OpenQA/CLI/api.pm

+      'pre-script-hook=s' => \my $prehooksrc,
+      'post-script-hook=s' => \my $posthooksrc,


Suggested change

'pre-script-hook=s' => \my $prehooksrc,

'post-script-hook=s' => \my $posthooksrc,

'pre-script-hook=s' => \my $pre_hooks_dir,

'post-script-hook=s' => \my $post_hooks_dir,

foursixnine · 2024-07-16T10:39:47Z

lib/OpenQA/CLI.pm

@@ -35,6 +35,9 @@ openqa-cli - provides command-line access to the openQA API
    # Show details for OSD job (prettified JSON)
    openqa-cli api --osd --pretty jobs/4160811

+    # Trigger a product using a hook script
+    openqa-cli api --o3 --hook-script foo -X POST isos


needs to be updated too

foursixnine · 2024-07-16T10:43:25Z

lib/OpenQA/CLI/api.pm

+    eval {
+	print 'TST ';
+	# https://stackoverflow.com/a/54500161/1462096
+	#my @out = qx($hook_path);


Why not pick one implementation and run with it? (also to remove the dependency on Mojo::IOLoop::ReadWriteProcess perhaps moving that _run_cmd_capturing_output (and similar ones) can be moved to a single place in some other PR.

Looks like the same thing is being implemented over and over...

Martchus · 2024-07-29T11:23:47Z

Sorry, I don't see the point in this. In the end we expect that some power users are able to tweak the test schedule based on logic that can be maintained without needing direct changes to os-autoinst nor openQA. I don't yet see how this openqa-cli extension enables that

The logic would not live in os-autoinst or openQA and no direct changes to those repos would be needed. Only this change to generally allow running a hook script would be required. The point was to make things easier on the openqa-trigger-from-obs side of things - especially because we as tools team were asked to help here.

However, with the pre/post-distinction and otherwise too convoluted logic I also don't see this PR being mergeable. The initial idea was to call a single, explicitly specified script. This PR is nothing like that, though. So I tend to close it and do whatever is needed in openqa-trigger-from-obs after all. At least os-autoinst/openqa-trigger-from-obs#255 looks more promising than this openqa-cli PR - and also simpler again.

[RFC] Create a hook placeholder

617952c

Martchus requested changes Jul 11, 2024

View reviewed changes

Martchus requested changes Jul 15, 2024

View reviewed changes

perlpunk reviewed Jul 15, 2024

View reviewed changes

okurz requested changes Jul 15, 2024

View reviewed changes

foursixnine reviewed Jul 16, 2024

View reviewed changes

Martchus mentioned this pull request Jul 29, 2024

Add openqa_cli property to xml os-autoinst/openqa-trigger-from-obs#255

Merged

b10n1k closed this Jul 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Create a hook placeholder #5757

[RFC] Create a hook placeholder #5757

b10n1k commented Jul 11, 2024

Martchus left a comment •

edited

Loading

Martchus Jul 11, 2024

Martchus Jul 11, 2024

Martchus Jul 11, 2024

Martchus Jul 11, 2024

Martchus Jul 11, 2024

foursixnine Jul 16, 2024

Martchus left a comment

Martchus Jul 15, 2024 •

edited

Loading

foursixnine Jul 16, 2024

Martchus Jul 15, 2024

perlpunk Jul 15, 2024

foursixnine Jul 16, 2024

perlpunk Jul 15, 2024

okurz left a comment

Martchus commented Jul 15, 2024

okurz commented Jul 15, 2024

foursixnine Jul 16, 2024

foursixnine Jul 16, 2024

Martchus Jul 16, 2024

foursixnine Jul 16, 2024

b10n1k Jul 18, 2024

perlpunk Jul 18, 2024

foursixnine Jul 16, 2024

foursixnine Jul 16, 2024

foursixnine Jul 16, 2024

foursixnine Jul 16, 2024

Martchus commented Jul 29, 2024 •

edited

Loading

	-H, --hook-scripts Use a hook script!! TDB
	-H, --hook-script Use a hook script!! TBD

		'pre-script-hook=s' => \my $prehooksrc,
		'post-script-hook=s' => \my $posthooksrc,

		my ($out, $err);
		my @out = run3([$hook_path, $data], \$out, \$err);

	my ($out, $err);
	my @out = run3([$hook_path, $data], \$out, \$err);
	my @out = run3([$hook_path, $data], \my $out, \my $err);

	Works with when -f is provided
	Works with when -f is provided

[RFC] Create a hook placeholder #5757

[RFC] Create a hook placeholder #5757

Conversation

b10n1k commented Jul 11, 2024

Martchus left a comment • edited Loading

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

Martchus left a comment

Choose a reason for hiding this comment

Martchus Jul 15, 2024 • edited Loading

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

okurz left a comment

Choose a reason for hiding this comment

Martchus commented Jul 15, 2024

okurz commented Jul 15, 2024

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

Martchus commented Jul 29, 2024 • edited Loading

Martchus left a comment •

edited

Loading

Martchus Jul 15, 2024 •

edited

Loading

Martchus commented Jul 29, 2024 •

edited

Loading