fix(engine): correctly set prune confirmation message in setRunningPhase#25915
Conversation
❌ Preview Environment deleted from BunnyshellAvailable 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
2 times, most recently
from
d805a32 to
7ef43a9
Compare
MrFreezeex
changed the title
MrFreezeex
force-pushed
the
fix-empty-task-message
branch
from
7ef43a9 to
8a174c5
Compare
|
Hi @agaudreault 👋, small reminder of this in case it dropped out of your radar, FYI the test added in this PR still fails on the master branch (so there's still a regression there essentially). |
| return | ||
| } | ||
|
|
||
| { |
There was a problem hiding this comment.
Code looks to be at the right place, but can you reuse the implementation below and set the reason to "pruning confirmation" instead of having a completely new code block? This way, we make sure the message pattern and overall logic is the same.
So in the end, it is something like "Waiting for pruning confirmation of foo/bar and X more" where X is the total number (-1) of resources to prune.
There was a problem hiding this comment.
@agaudreault Oh yes good idea! I just implemented your suggestion let me know if this looks right!
MrFreezeex
force-pushed
the
fix-empty-task-message
branch
from
8a174c5 to
44c3271
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 <arthur.outhenin-chalandre@ledger.fr>
MrFreezeex
force-pushed
the
fix-empty-task-message
branch
from
44c3271 to
cd11dea
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #25915 +/- ##
==========================================
+ Coverage 62.65% 62.69% +0.04%
==========================================
Files 412 412
Lines 55562 55566 +4
==========================================
+ Hits 34814 34839 +25
+ Misses 17426 17411 -15
+ Partials 3322 3316 -6 ☔ View full report in Codecov by Sentry. |
agaudreault
left a comment
agaudreault
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the refactor :)
…ase (argoproj#25915) Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>
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.