-
Notifications
You must be signed in to change notification settings - Fork 15
Disable pauses for auto pause enabled updates #216
Conversation
Add IT Signed-off-by: Abdul Qadeer <aqadeer@paypal.com>
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 minus one minor nit
.getInstructions() | ||
.getSettings() | ||
.getUpdateStrategy()); | ||
if (isAutoPauseEnabled && instancesSeen.containsKey(key)) { |
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 we check if the instancesSeen contains the key, we can know the update started allowing us to pause for a brief time before the update starts if we do this check. It's probably overkill to do that in this case. I think in this case we can just check to see if it's auto pause enabled by itself.
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.
Yeah on second look it seems redundant. Fixed.
Signed-off-by: Abdul Qadeer <quadeer.leo@gmail.com>
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.
Add IT
Signed-off-by: Abdul Qadeer aqadeer@paypal.com
Description:
This PR prevents pauses on auto pause enabled job updates.
Testing Done:
Ran UTs - all passed
E2E tests (failures unrelated to this commit)