Add comment-triggered PR build action

[Icohedron]

Implements a GitHub Workflow for triggering PR builds. Addresses #2199

Known issues:

• The builds checkout the master branch of the base repo rather than the head repo and branch of the PR. This must be fixed. Would need to change these lines of the workflow file. Fixed!
• When there is an error in the comment, the resulting error message mentions the action_argparse.py script used to parse the comment. A minor issue but I see no obvious fix aside from overriding the error messages of Python's argparse or writing a new parser from scratch. (See example error below) Fixed!
• A "Build(s) failed" error message is displayed even when no builds were started. A minor issue but I see no obvious fix. (See example error below)
• A newline at the end of the comment will result in an empty error message. Additionally, one can suffix the comment with || <command> to execute any arbitrary shell command on the GitHub runner. Security issue? I do not see a way to prevent this at the time being. Fixed!
• Depending on when the workflow was cancelled, the status report comment may say "Build(s) successful!" instead of "Build(s) cancelled!" because the runBuild part of the workflow was skipped while the parseComment part of the workflow was successful.

Discussions:

• What parameters should be required, and what should the default values for non-required parameters be?
• Instead of the keyword "action", perhaps we can use a keyword that is more descriptive of what it does?

Potential improvements:

• Instead of hard-coding valid parameter values or accepting all values, we can fetch the valid parameter values from some resource (e.g. the test repository's file structure and/or Jenkins Grinder options)
• Instead of reporting a vague "Build(s) failed" message, give a break-down of which builds failed and which builds succeeded within the comment itself rather than simply giving a hyperlinked workflow ID for the user to click for more details.
• Add additional parameters for custom_target, openjdk_testRepo, and tkg_Repo to pass to the run-aqa action that runs the builds.

Example usage:

Example error:

[llxia]

What parameters should be required, and what should the default values for non-required parameters be?

I think we should have the default values for the following parameters:
--sdk_resource = nightly
--build_list = openjdk
--target = jdk_math
--platform = x86-64_linux
--jdk_version = 8
--jdk_impl = openj9

We can adjust this later to cover the common use case.

[sophia-guo]

The builds checkout the master branch of the base repo rather than the head repo and branch of the PR. This must be fixed. Would need to change these lines of the workflow file.

The event is issue_comment, which is different from the event pull request. Under the current status it need extra steps to get the PR's branch.

There is an open issue in github action and hence have some examples of how to do it. Hope it helpful. actions/checkout#331

Regarding the comment-trigger, it might be helpful to separate if it's a PR comment or an issue comment. If it's a PR comment there will be extra steps to get the branch of PR. If it's an issue comment the branch can be set directly by parameters. ( My understand is for now we can first focus on PR comment)

[sophia-guo]

For job runBuild you may use needs: parseComment. So if job parseComment fail job runBuild will be skipped automatically.

[Icohedron]

What parameters should be required, and what should the default values for non-required parameters be?

I think we should have the default values for the following parameters:
--sdk_resource = nightly
--build_list = openjdk
--target = jdk_math
--platform = x86-64_linux
--jdk_version = 8
--jdk_impl = openj9

We can adjust this later to cover the common use case.

@llxia These defaults appear to fail to build. See this run: 544298906

[Icohedron]

• The builds checkout the master branch of the base repo rather than the head repo and branch of the PR. This must be fixed. Would need to change these lines of the workflow file.

This issue has been fixed! See commit 9128daa

[Icohedron]

• When there is an error in the comment, the resulting error message mentions the action_argparse.py script used to parse the comment. A minor issue but I see no obvious fix aside from overriding the error messages of Python's argparse or writing a new parser from scratch. (See example error below)

This issue has been fixed!

[Icohedron]

New issue: A newline at the end of the comment will result in an empty error message. Additionally, one can suffix the comment with || <command> to execute any arbitrary shell command on the GitHub runner. Security issue? I do not see a way to prevent this at the time being.

Would need to alter this line in action.yml to get the comment body as argument to the Python script while stripping extra whitespace and disallowing additional shell commands and operations.

Fixed!
Also made a PR to fix the same issue in an existing workflow (autoTestPR.yml) #2244

[smlambert]

I think you have described how this vulnerability has to be handled. Checking the comment and allowing only the set of parameters for run-aqa. If anything else is present, then exit with an error message.

[Icohedron]

New issue: A newline at the end of the comment will result in an empty error message. Additionally, one can suffix the comment with || <command> to execute any arbitrary shell command on the GitHub runner. Security issue? I do not see a way to prevent this at the time being.

Would need to alter this line in action.yml to get the comment body as argument to the Python script while stripping extra whitespace and disallowing additional shell commands and operations.

This issue has been fixed! See commit 7d7489a

The fix/remedy to the issue was to store the user input (the comment body) in an environment variable before passing it to the Python script, as is suggested in this GitHub Security Lab article.

[Icohedron]

Instead of the keyword "action", perhaps we can use a keyword that is more descriptive of what it does?

I have changed the keyword to "run aqa". Now instead of "action", use "run aqa".

What parameters should be required, and what should the default values for non-required parameters be?

I think we should have the default values for the following parameters:
--sdk_resource = nightly
--build_list = openjdk
--target = jdk_math
--platform = x86-64_linux
--jdk_version = 8
--jdk_impl = openj9
We can adjust this later to cover the common use case.

These defaults appear to fail to build. See this run: 544298906

The issue was that the target required an underscore prefix. Therefore I have made the argparse python script add an underscore prefix if one is not present.

[smlambert]

@llxia @sophia-guo - looks like this PR is ready for a final review 👍

[sophia-guo]

Would it be nicer to set jdk_impl and platform as choices, similarly to build_list and sdk_source?

[Icohedron]

It would be openj9 and hotspot as the only two choices?

[sophia-guo]

It would be openj9 and hotspot as the only two choices? For jdk_impl @smlambert

[smlambert]

yes those 2 choices (and should we ever need it, in future, we can add jdk_vendor as an argument, but will not include it for now)

[sophia-guo]

I have set platform as 'x86-64_linux ` with extra space by accident and got https://github.com/sophia-guo/openjdk-tests/runs/1919706546?check_suite_focus=true. It would be nice to set platform as choices so the job would fail when do the parseComment instead of runBuild and can't find the runner.

[Icohedron]

I have set platform as 'x86-64_linux ` with extra space by accident and got https://github.com/sophia-guo/openjdk-tests/runs/1919706546?check_suite_focus=true. It would be nice to set platform as choices so the job would fail when do the parseComment instead of runBuild and can't find the runner.

That would be quite a long choices list. The workflow allows github runner platforms, which include various versions of windows, mac, and linux which are constantly updating. Perhaps restrict to only these few?

x86-64_windows [alias for windows-latest]
x86-64_mac     [alias for macos-latest]
x86-64_linux   [alias for ubuntu-latest]
windows-latest
macos-latest
ubuntu-latest

[sophia-guo]

platform is what jdk supports on. Here is the list https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/buildenv/jenkins/openjdk_tests. windows-latest or others are supported virtual-environments in github runners. https://github.com/actions/virtual-environments.

That is no all supported platforms are enabled on github runners.
There is a map between them.

For example x86-64_linux, x86-64_linux_mixed, x86-64_linux_xl can map to any one of ubuntu-16.04, ubuntu-latest or ubuntu-18.04, ubuntu-16.04.

So choices would be [x86-64_linux, x86-64_linux_mixed, x86-64_linux_xl, x86-64_mac , x86-64_mac_mixed, x86-64_mac_xl, x86-64_windows,x86-64_windows_mixed,x86-64_windows_xl]. Going forward some of them will be removed or combined so the list would be smaller.

We can leave it as is and update later if the choices helps.

[sophia-guo]

LGTM

[llxia]

Thanks @Icohedron