[WIP] Avoid unnecessary test restarts on PR approval. [DO NOT MERGE] #4598
Conversation
github-actions
bot
requested review from
eriknordmark,
uncleDecart,
jsfakian and
milan-zededa
|
@OhmSpectator , AFAIK the GHA it won't run from your PR, unfortunately we need to first merge it to master to actually see if it works... that's why a very careful review is needed.... ☝️ @yash-zededa |
OhmSpectator
force-pushed
the
feature/do-not-restart-test-on-approve
branch
from
4f22e69 to
9cb8289
Compare
I hoped to test it in the scope of this PR, because, to be honest, I'm absolutely unsure about the proposed approach... |
|
By the way, I've just noticed that the artifacts from the failed Yetus run do not contain real logs with errors =( |
It just brings change to a .go file to trigger Eden (we need to test it). Signed-off-by: Nikolay Martyanov <[email protected]>
OhmSpectator
force-pushed
the
feature/do-not-restart-test-on-approve
branch
from
9cb8289 to
446f2d2
Compare
There was a problem hiding this comment.
Approving for testing as requested by @OhmSpectator
OhmSpectator
changed the title
Added a new `check_existing_workflow` job to prevent restarting tests if a workflow is already running. This job checks for active test runs and only triggers a new one if no workflow is in progress and the last completed test run failed or was canceled. Modified `get_run_id` to depend on `check_existing_workflow.outputs.should_run`, ensuring tests only start when needed. This avoids redundant test executions on repeated approvals while still allowing re-runs if previous tests failed. Signed-off-by: Nikolay Martyanov <[email protected]>
Turn off check for WF files to prevent tests run Signed-off-by: Nikolay Martyanov <[email protected]>
OhmSpectator
force-pushed
the
feature/do-not-restart-test-on-approve
branch
from
446f2d2 to
45b602a
Compare
|
Did you see that there are changes to |
|
I think I understand you. The logic now is, "when approved, run the advanced tests". You are saying, "sure, but if you approved, and the tests started, and then I approved, don't restart the tests, they already are running." Two requests. First, it takes reading the code to understand that logic (and I am sure I didn't understand it). Can we document it, and make it really clear all of the scenarios and when they should run and when not? Second, if I understood the logic correctly, shouldn't the choice of whether or not approval triggers workflow depend not on failure or not, but instead on whether or not the currently running workflow has the same hash as the current one on approval? Or maybe even just "is anything running"? The concern is that if no workflow is running, and I approve, that shouldn't necessarily trigger a workflow run either; maybe nothing has changed. It really should depend on, "is the hash changed from the last run". |
Yeah, sure. Once I have a verified PoC I will polish the code to make it more understandable. Current PR is more about testing the approach.
Good point. I'll check how to get this info about "if the PR changed". |
@rene Yes. With the current WF condition the eden.yml won't be triggered to test these changes. Even if the condition as for false the eden would be triggered only for the PR approved condition which makes it difficult to test. |
|
Approving the PR to test something. |
|
@yash-zededa, I don't think I'll be working on this PR for the next few days. Feel free to take it over if you'd like. |
|
@OhmSpectator I will take over and optimize it :) |
|
@yash-zededa, yeah, sure, thanks a lot! |
| RUNNING_WORKFLOWS=$(curl -s -L \ | ||
| -H "Authorization: Bearer $GITHUB_TOKEN" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| "https://api.github.com/repos/$REPO/actions/runs?head_sha=$PR_SHA&status=in_progress") |
There was a problem hiding this comment.
Does including the PR_SHA mean that if there was an update to the PR it will run? The description doesn't state this which is why I'm asking.
Added a new
check_existing_workflowjob to prevent restarting tests if a workflow is already running. This job checks for active test runs and only triggers a new one if no workflow is in progress and the last completed test run failed or was canceled.Modified
get_run_idto depend oncheck_existing_workflow.outputs.should_run, ensuring tests only start when needed. This avoids redundant test executions on repeated approvals while still allowing re-runs if previous tests failed.