Add GitHub action for cloning an openQA job mentioned in a PR description #292

Martchus · 2024-02-21T17:08:32Z

This GitHub action allows to clone a job when creating a PR by mentioning it in the PR description via @openqa: Clone https://….

Related ticket: https://progress.opensuse.org/issues/130934

perlpunk · 2024-02-21T18:27:26Z

openqa-clone-and-monitor-job-from-pr

+}
+
+sub _run_cmd ($name, $args, $capture_output = 0) {
+    my $res = $capture_output ? `$name $args` : system "$name $args";


We should probable make sure that the arguments cannot be used to run arbitrary commands, not sure if this would be possible with the current inputs. But better be safe than sorry.
If this is used for pull_request_target, it could be possible to leak our API credentials.

Since we are in perl already, instead of calling openqa-cli ... we might also directly use

use OpenQA::CLI; my $ret = OpenQA::CLI->new->run(@args);

Just that we need to capture stdout. OpenQA::CLI could also offer an option for returning the output in a variable.

We could but we needed to add OpenQA:: to the search path. And we needed to make sure not to break the API in the future.

hm, now I'm wondering why the openQA modules aren't installed in the usual place for perl modules, where it would be found by default.
I understand that for the os-autoinst modules that could be a bad idea, but OpenQA is a clear namespace.

open(my $fh, "-|", $cmd, @args); while (<$fh>) { ... }

And to clarify what I meant and what we discussed in the call: If a user manages to manipulate any of the input variables to contain anything then the secrets can be leaked: '; curl-command-to-post-secrets-to-some-server; '

I now changed it to use system when we don't want to capture the output and open when we want to.

In my tests arguments are passed literally with both commands when at least one argument is given (which is always the case here). So that should address the security concern.

Note that I've also tested the error handling and especially checking the command's exit code really works after the close call (when previously invoking the command via open). So if openqa-clone-job can be started but fails with a non-zero exit status we get the right error message and the code doesn't try to parse the output as JSON (leading to a worse/confusing error message).

perlpunk · 2024-02-22T20:21:24Z

openqa-clone-and-monitor-job-from-pr

+    _run_cmd 'openqa-cli', 'monitor', '--host', $expected_url, @secrets, @job_ids;
+}
+
+$ENV{TEST_MODE} ? 1 : run;


We could save the TEST_MODE thing and do:

Suggested change

$ENV{TEST_MODE} ? 1 : run;

run() unless caller();

foursixnine · 2024-02-23T08:43:17Z

openqa-clone-and-monitor-job-from-pr

+my $expected_url = $ENV{OPENQA_HOST} // 'https://openqa.opensuse.org';
+my $group_id = $ENV{OPENQA_SCHEDULE_GROUP_ID} // 118;
+my $pr_body = $ENV{GH_PR_BODY} or die 'GH_PR_BODY must be set';
+my $gh_repo = $ENV{GH_REPO} or die 'GH_REPO must be set';
+my $gh_ref = $ENV{GH_REF} or die 'GH_REF must be set';
+my $gh_srv = $ENV{GITHUB_SERVER_URL} or die 'GITHUB_SERVER_URL must be set';
+my $api_key = $ENV{OPENQA_API_KEY} or die 'OPENQA_API_KEY must be set';
+my $api_secret = $ENV{OPENQA_API_SECRET} or die 'OPENQA_API_SECRET must be set';
+my @secrets = ('--apikey', $api_key, '--apisecret', $api_secret);
+my @vars = ("BUILD=$gh_repo.git#$gh_ref", "_GROUP_ID=$group_id", "CASEDIR=$gh_srv/$gh_repo.git#$gh_ref");


Assume that somebody will run this as a script, please add at least POD documentation

Not really except for manual testing. It is supposed to be used via a GitHub action. But I'll create POD documentation nevertheless.

foursixnine · 2024-02-23T09:01:23Z

openqa-clone-and-monitor-job-from-pr

+# Note that this script is intended for execution via the GitHub action defined in
+# actions/clone-job/action.yaml.
+
+package openqa_clone_and_monitor_job_from_pr;


This is not needed, as this is a script. An idea on a separate level, why not make it an openQA cli command instead of a wrapper for it?

An idea on a separate level, why not make it an openQA cli command instead of a wrapper for it?

obviously, out of the scope of this PR i'd say

This is needed for testing and will not harm otherwise.

why not make it an openQA cli command instead of a wrapper for it?

This is also invoking the clone script. Additionally, it contains GitHub-specific parts; its usage is tailored for use within a GitHub action (taking "arguments" mainly from environment variables) which doesn't really fit into openqa-cli's style.

Oh, and the openQA repo is already big enough. So I really like to add code that is rather specific to this additional scripts repo (like we did with many of the other scripts contained here). But that also means I cannot really utilize the modules from the openQA repo. Even if the modules would be installed in the normal Perl path (or we adjusted the search path) this would be problematic because when someone would change things on the openQA-side it might break things here (and openQA is not really developed as an API-stable Perl module).

Even if the modules would be installed in the normal Perl path (or we adjusted the search path) this would be problematic because when someone would change things on the openQA-side it might break things here (and openQA is not really developed as an API-stable Perl module).

I'm looking more into proper github action repo as a separate thing, but that is something far in the future :)

the scope of openqa-cli's argument makes a lot of sense

foursixnine · 2024-02-23T13:37:18Z

openqa-clone-and-monitor-job-from-pr

+    print 'No test cloned; the PR description does not contain ';    # uncoverable statement
+    print "a command like '\@openqa: Clone $expected_url/tests/<JOB_ID>'.\n";    # uncoverable statement


will this be shown in the UI? as a comment on the GH pull request?

So far it'll be only shown in the log of the GitHub action. We could extend it later to also write a comment or use the statuses API.

foursixnine · 2024-02-23T13:40:25Z

openqa-clone-and-monitor-job-from-pr

+For local testing you may also invoke the script manually. Have a look at the
+handling of environment variables at the beginning of the script code as you
+need to set certain additional environment variables that are normally supplied


Suggested change

For local testing you may also invoke the script manually. Have a look at the

handling of environment variables at the beginning of the script code as you

need to set certain additional environment variables that are normally supplied

For local testing you may also invoke the script manually. Have a look at the

handling of environment variables at the beginning of the script code as you

need to set certain additional environment variables, the `test/05-clone-and-monitor.t`

can also clarify this script's behavior from user perspective.

Since this is only meant for GH, I think it's ok to redirect the reader to how the test is done.

…tion This GitHub action allows to clone a job when creating a PR by mentioning it in the PR description via `@openqa: Clone https://…`. Related ticket: https://progress.opensuse.org/issues/130934

Martchus · 2024-02-23T16:37:11Z

Marking as ready again as I was able to test it and it works. Of course I only tested it so far on my fork but I guess that should suffice for now (see Martchus/os-autoinst-distri-opensuse#2 for an example; I cancelled the cloned job manually). Later (or next week) I'll provide a PR to use it in the openSUSE distribution.

In addition to that, I suppose it would also make sense to treat OPENQA_HOST not as secret. Otherwise the log looks like this:

 Cloned '***/tests/3957590' into:
 - ***/tests/3959032

Or was there a reason why we even treat the hostname as a secret?

Martchus force-pushed the gh-action branch from eca5837 to 7f4a0ec Compare February 21, 2024 17:15

perlpunk reviewed Feb 21, 2024

View reviewed changes

perlpunk mentioned this pull request Feb 21, 2024

Allow to clone and monitor a job when creating a PR os-autoinst/os-autoinst-distri-opensuse#18697

Merged

Martchus force-pushed the gh-action branch from 7f4a0ec to d1edf02 Compare February 22, 2024 13:04

okurz approved these changes Feb 22, 2024

View reviewed changes

perlpunk reviewed Feb 22, 2024

View reviewed changes

foursixnine requested changes Feb 23, 2024

View reviewed changes

Martchus force-pushed the gh-action branch 2 times, most recently from af60f9c to 7531867 Compare February 23, 2024 11:51

foursixnine reviewed Feb 23, 2024

View reviewed changes

perlpunk approved these changes Feb 23, 2024

View reviewed changes

Martchus force-pushed the gh-action branch from 7531867 to 9db67d2 Compare February 23, 2024 15:54

Martchus marked this pull request as draft February 23, 2024 15:54

Martchus force-pushed the gh-action branch from 9db67d2 to 5c29565 Compare February 23, 2024 16:03

Add GitHub action for cloning an openQA job mentioned in a PR descrip…

29f98b6

…tion This GitHub action allows to clone a job when creating a PR by mentioning it in the PR description via `@openqa: Clone https://…`. Related ticket: https://progress.opensuse.org/issues/130934

Martchus force-pushed the gh-action branch from 5c29565 to 29f98b6 Compare February 23, 2024 16:05

foursixnine approved these changes Feb 23, 2024

View reviewed changes

Martchus marked this pull request as ready for review February 23, 2024 16:32

mergify bot merged commit eebc855 into os-autoinst:master Feb 23, 2024
4 checks passed

Martchus deleted the gh-action branch February 23, 2024 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GitHub action for cloning an openQA job mentioned in a PR description #292

Add GitHub action for cloning an openQA job mentioned in a PR description #292

Martchus commented Feb 21, 2024

perlpunk Feb 21, 2024

perlpunk Feb 21, 2024

Martchus Feb 21, 2024

perlpunk Feb 21, 2024

perlpunk Feb 22, 2024

perlpunk Feb 22, 2024

Martchus Feb 22, 2024

perlpunk Feb 22, 2024

foursixnine Feb 23, 2024

Martchus Feb 23, 2024

foursixnine Feb 23, 2024

foursixnine Feb 23, 2024

Martchus Feb 23, 2024

Martchus Feb 23, 2024

foursixnine Feb 23, 2024

foursixnine Feb 23, 2024

Martchus Feb 23, 2024

foursixnine Feb 23, 2024

Martchus commented Feb 23, 2024

		print 'No test cloned; the PR description does not contain '; # uncoverable statement
		print "a command like '\@openqa: Clone $expected_url/tests/<JOB_ID>'.\n"; # uncoverable statement

Add GitHub action for cloning an openQA job mentioned in a PR description #292

Add GitHub action for cloning an openQA job mentioned in a PR description #292

Conversation

Martchus commented Feb 21, 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

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 Feb 23, 2024