Skip to content
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

emit lock duration when stack is unlocked #877

Merged
merged 3 commits into from Mar 4, 2019

Conversation

Projects
None yet
3 participants
@doodzik
Copy link
Member

doodzik commented Mar 1, 2019

This PR adds support for receiving the start and end time of how long a lock lasted. That can be used to save the data in order to query it later.

doodzik added some commits Mar 1, 2019

@doodzik doodzik changed the title [WIP] emit lock duration when stack is unlocked emit lock duration when stack is unlocked Mar 1, 2019

@doodzik doodzik requested a review from Shopify/pipeline Mar 1, 2019

@DazWorrall
Copy link
Member

DazWorrall left a comment

Test failure looks like it might be flakiness.

Hook.emit(:lock, self, locked: locked?, stack: self)

lock_duration = if previous_changes['lock_reason'].last.blank?
{from: previous_changes['locked_since'].first, until: Time.zone.now}

This comment has been minimized.

@DazWorrall

DazWorrall Mar 4, 2019

Member

The value of this doesn't match the name. Could we emit an integer here (lock_duration_in_seconds)? I think that would make the testing easier too (e.g. probably wouldn't need to add Timecop).

This comment has been minimized.

@casperisfine

casperisfine Mar 4, 2019

Contributor

_in_seconds is a bit verbose for no reason, so I'd say lock_duration, but 👍

This comment has been minimized.

@doodzik

doodzik Mar 4, 2019

Author Member

I wouldn't want to change that as we would loose the start and the end time of the lock. It is better to safe the raw data and do the calculation later on.

This comment has been minimized.

@DazWorrall

DazWorrall Mar 4, 2019

Member

nit: lock_details or similar would have been better in that case, the value of this field is not a duration.

This comment has been minimized.

@doodzik

doodzik Mar 4, 2019

Author Member

makes sense. I'll update that tomorrow.

@DazWorrall DazWorrall requested a review from casperisfine Mar 4, 2019

Show resolved Hide resolved Gemfile Outdated
Hook.emit(:lock, self, locked: locked?, stack: self)

lock_duration = if previous_changes['lock_reason'].last.blank?
{from: previous_changes['locked_since'].first, until: Time.zone.now}

This comment has been minimized.

@casperisfine

casperisfine Mar 4, 2019

Contributor

_in_seconds is a bit verbose for no reason, so I'd say lock_duration, but 👍

@doodzik doodzik requested review from casperisfine and DazWorrall Mar 4, 2019

@doodzik doodzik force-pushed the emit-unlock-events branch from 2436620 to 8a19c11 Mar 4, 2019

@doodzik doodzik merged commit 657c9b9 into master Mar 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.