[ci] improve experience with optional GitHub workflows #3740
Conversation
| submodules: false | ||
| - name: Check that all tests succeeded | ||
| run: | | ||
| workflows=("R valgrind tests") |
There was a problem hiding this comment.
Here we should add optional workflows which failures should block PR merging.
| run: | | ||
| $GITHUB_WORKSPACE/.ci/append_comment.sh \ | ||
| "${{ github.event.client_payload.comment_number }}" \ | ||
| "Status: ${{ job.status }}." |
There was a problem hiding this comment.
Yes, I know that direct link for artifact downloading will be more useful. But unfortunately, it is impossible:
Gets a redirect URL to download an archive for a repository. This URL expires after 1 minute.
https://docs.github.com/en/free-pro-team@latest/rest/reference/actions#download-an-artifact
There was a problem hiding this comment.
ha it's ok, we don't use this job that often and it's only one or two more clicks to get to the artifact
StrikerRUS
requested review from
guolinke,
btrotta,
chivee,
henry0312 and
shiyu1994
There was a problem hiding this comment.
This is a great! Thanks for all the hard work. I think this will be really useful.
I read through all the scripts and the docs on repository_dispatch events and I think I understand what's going on here. Left some very minor comments, but after those are addressed I'm +1 on merging this and testing it out.
| try: | ||
| from urllib import request | ||
| except ImportError: | ||
| import urllib2 as request |
There was a problem hiding this comment.
why was this necessary? I have a lot more experience with requests than urllib, so I haven't seen this pattern
There was a problem hiding this comment.
This is for Python 2/3 compatibility. For the same reason I'm not using with ... statement but manually close connection. I know we dropped Python 2 support some time ago. But default installed Python version in ubuntu-latest container we run our actions in is 2. I really don't want to install conda/new Python version/additional packages because I want to keep Optional checks workflow where this script is used as lightweight as possible with the aim of the fastest execution time. So this Python-agnostic and dependency-free solution will work fine on any default Python version that can be installed in container.
There was a problem hiding this comment.
ok I see, thanks. Fine with me
StrikerRUS
force-pushed
the
optional_runs
branch
from
888d93f
to
84a6394
Compare
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
This PR was created by the inspiration of idea to require optional workflows such as
R valgrind tests(those which takes a lot of time and we run them from time to time) #3439 (comment) either not be run at all or be completed successfully.Unfortunately, it is impossible as of my knowledge. Two main reasons are:
OK, probably it is possible, but will require switching to GitHub Checks from GitHub Statuses and creating own GitHub App. But I think this is an overkill for us.
Due to the limitations described above this PR tries to implement desired behavior as close as possible but there are two cases when maintainer can merge a PR with actually failing option workflow. So please DO NOT do the following scenarios.
Mergebutton. Please after triggering a workflow be patient and allow it to send initial feedback.Required(if they were, it is impossible to implement a case when we do not want to start a particular workflow for a given PR) the first reported status should be checked manually. I believe it is not a very big deal because merge button it case of failure of optional workflow will be grey not green and statuses section will be uncollapsed (see pic. below). But this is applicable only for the first reported status, all further statuses will be counted byOptional checksRequired workflow so that any failures will prevent merging.Also, of course there can happen any network issues that prevent a workflow to send a feedback. But this problem is common to any CI service.
In addition, now optional workflows will be triggered by ordinary comments in PRs, not a review ones. This change is required due to secret values limitation described above. GitHub emits runs triggered by review comments associated with a fork PR made from, and runs "belong" to upstream repo in case of issue comment.
Here are some screens from my fork to show how it will look like:
Unfortunately, I'm afraid that reviewers are not able to test this PR properly, because workflow configs should be in
masterbranch to start working. So, please perform real tests of new chatOps functionality in a follow-up fake PR. Just propose any change from a personal fork (it is important because of secrets issue described above) and try to trigger optional workflows.I have been struggling with this for three months, so for sure this my initial description is lacking some details you want to know about. So feel free to ask, I'll recollect them in memory 🙂 .