fix(engine): correctly set prune confirmation message in setRunningPhase #25915
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
|
Hi again @agaudreault 👋, while investigating why #23370 unit test was not working I discovered this regression in master linked to an improvement you made a few weeks ago in this PR: #25668 |
|
|
||
| func (sc *syncContext) setRunningPhase(tasks syncTasks, isPendingDeletion bool) { | ||
| if tasks.Len() == 0 { | ||
| sc.setOperationPhase(common.OperationRunning, "") |
There was a problem hiding this comment.
Running should not preserve the last message. Instead, it should by able based on the list of task to determine the message that the sync is waiting for applications to be pruned. In this case, it might not receive the correct list of task if it is empty.
There was a problem hiding this comment.
You should investigate what sets the message to "Waiting for pruning confirmation" and move the logic to setRunningPhase. You will also need to investigate what calls setRunningPhase with an empty list of task.
IMO, not setting the operation to Running in the setRunningPhase is incorrect. There is a unit test missing that should validate that setRunningPhase sets the phase to running if tasks is empty.
There was a problem hiding this comment.
Might be partially fixed by #25916. But https://github.com/agaudreault/argo-cd/blob/fe2760578363385e0dcf156edd40d913c9b81ec8/gitops-engine/pkg/sync/sync_context.go#L1493-L1497 should be moved to setRunningPhase
There was a problem hiding this comment.
Ah ok I see, is something my last push what you would be looking for then?
MrFreezeex
force-pushed
the
fix-empty-task-message
branch
from
127b5f3 to
d805a32
Compare
setRunningPhase was overwriting the message when it receives an empty list of tasks while we were setting the message before setRunningPhase was called which broke the prune confirmation message since commit 95d19f2. This moves the message logic in setRunningPhase with the rest of the message logic and remove the tasks filtering when the sync is pending to allow retrieving the pruning tasks and construct the message indicating to wait for pruning confirmation. Also add a unit test covering this recently broken behavior Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
MrFreezeex
force-pushed
the
fix-empty-task-message
branch
from
d805a32 to
7ef43a9
Compare
MrFreezeex
changed the title
3 participants
Checklist:
Restore the behavior before commit 95d19f2 to not force the message to an empty string if the tasks slice is empty. This broke the Prune=confirm features that set a message but get overwritten here. Also add a Prune=confirm unit test to prevent further regression.