-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix problem with ephemeralRunner Succeeded state before build executed #3528
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we should!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'll wait for your feedback on Monday before merging it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also fix the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'll wait for your feedback on Monday before merging it.
We didn't observe the issue since Wednesday :)
fc16e8b
to
de5592f
Compare
de5592f
to
2ff419b
Compare
2ff419b
to
41d58ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Fixes #3527
Seems that changing order of actions fixes the issues: