Fix problem with ephemeralRunner Succeeded state before build executed #3528
Conversation
kr-sabre
requested review from
mumoshu,
toast-gear,
nikola-jokic,
rentziass and
a team
as code owners
|
But I'm wondering if returning result after each finalizer apply isn't more correct approach... |
| @@ -149,6 +149,11 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ | |||
| return ctrl.Result{}, nil | |||
| } | |||
|
|
|||
| if ephemeralRunner.Status.RunnerId == 0 { | |||
There was a problem hiding this comment.
Hey, I don't think that changing the registration and then patching the finalizers should be the right way to go. However, I noticed that the order in which we applied finalizers is incorrect.
We first need to apply the ephemeralRunnerFinalizerName, then the ephemeralRunnerActionsFinalizerName.
Would you be interested in trying out that approach?
There was a problem hiding this comment.
Sure, will do the tests, but shouldn't we return Result also after ephemeralrunner.actions.github.com/runner-registration-finalizer apply?
There was a problem hiding this comment.
Updated the code, please check now
I don't see such issues with the updated code, let me check a little longer - will be back with update on Monday
There was a problem hiding this comment.
Looks good to me. I'll wait for your feedback on Monday before merging it.
There was a problem hiding this comment.
Can you please also fix the test
There was a problem hiding this comment.
Looks good to me. I'll wait for your feedback on Monday before merging it.
We didn't observe the issue since Wednesday :)
kr-sabre
force-pushed
the
fix/issue_3527_fix
branch
from
fc16e8b
to
de5592f
Compare
nikola-jokic
added
the
gha-runner-scale-set
kr-sabre
force-pushed
the
fix/issue_3527_fix
branch
from
de5592f
to
2ff419b
Compare
kr-sabre
force-pushed
the
fix/issue_3527_fix
branch
from
2ff419b
to
41d58ed
Compare
Fixes #3527
Seems that changing order of actions fixes the issues: