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

Refactor extend life feature. #42

Merged
merged 2 commits into from May 24, 2016
Merged

Conversation

maltoe
Copy link
Collaborator

@maltoe maltoe commented May 23, 2016

Came late to the party :)

Nice addition @jonp. Would you be ok with the changes I did here? Instead of having a dedicated extend_life method, you can use the extend_life parameter. Also keeps the semantics of lock with extend the same as before, and does not replicate behaviour of try_lock_instances in extend_life method.

 * Remove Client#extend_life and #extend_life! methods,
   offer an additional extend_life option for Client#lock
   instead.
 * Pass options hash down the line to try_lock_instances,
   only there decide which script to use, etc.
 * Fix specs and testing helper.

Fixes #36.
@maltoe maltoe force-pushed the fix_36_refactor_extend_life branch from e536f71 to 7665862 Compare May 23, 2016 08:27
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.561% when pulling 7665862 on fix_36_refactor_extend_life into 1f94462 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.561% when pulling 7665862 on fix_36_refactor_extend_life into 1f94462 on master.

@maltoe
Copy link
Collaborator Author

maltoe commented May 23, 2016

@leandromoreira Coveralls is funny, coverage decreased as I removed a bunch of (covered) lines... The codeship CI tool I cannot access for some reason, seems it is private. Can you tell me what it says?

@leandromoreira
Copy link
Owner

leandromoreira commented May 23, 2016

Can you tell me what it says?

One should not believe in coveralls small percentage :rage4:

The codeship CI tool I cannot access for some reason, seems it is private.

I'll check, I think we can remove it since we already have travis, what do you think?

@maltoe
Copy link
Collaborator Author

maltoe commented May 24, 2016

Sure, as said I can't even view it, so let's remove it :)

@maltoe
Copy link
Collaborator Author

maltoe commented May 24, 2016

Btw, @jonp gave his thumbs-up, so if you're fine with the changes, we could merge this before people start using extend_life

@leandromoreira leandromoreira merged commit b97bd0b into master May 24, 2016
@leandromoreira
Copy link
Owner

Thanks @maltoe and @jonp

if @testing_mode == :bypass
{
validity: ttl,
resource: resource,
value: extend ? extend.fetch(:value) : SecureRandom.uuid
value: options[:extend] ? options[:extend].fetch(:value) : SecureRandom.uuid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options.fetch(:extend, SecureRandom.uuid)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but we need options[:extend][:value]

@maltoe maltoe deleted the fix_36_refactor_extend_life branch February 27, 2018 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants