-
Notifications
You must be signed in to change notification settings - Fork 154
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: do not try to update actionset state when publishing artifacts #2883
Conversation
624e5d9
to
15ea80f
Compare
If there are multiple actions running in parallel, they will populate artifacts while another action is still running. This causes a conflict in update of actionset state. Actionset state should be updated by separate function maybeSetActionSetStateComplete Fixes #2882
15ea80f
to
14a8814
Compare
TOOD: add tests |
for _, p := range as.Phases { | ||
if p.State != crv1alpha1.StateComplete { | ||
log.WithContext(ctx).Print( | ||
"Finished action, but other action's phase is still running. Not setting state to complete.", |
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.
I have a question about this, in the PR desc also it's mentioned that Actionset state should be updated only when all actions complete by onUpdateActionSet
.
What I am not able to understand is, the relation between two actions.
Finished action, but other action's phase is still running.
if backup action is complete should it not be marked complete? Which other action would be wait for before marking backup action to be complete?
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.
I was hoping we can make it work on onUpdateActionSet
, but there are unresolved races there. I will change the description.
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.
if backup action is complete should it not be marked complete
There is no status per action, adding which would be a bigger change than a bugfix. We have status per phase and per actionset currently. So we can't mark an action to be complete (which would make things a bit easier TBH).
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
Change Overview
If there are multiple actions running in parallel, they will populate artifacts while another action is still running.
This causes a conflict in update of actionset state.
Actionset state should be updated only when all actions complete
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan