Skip to content
This repository was archived by the owner on Dec 17, 2019. It is now read-only.

Task leases (fixes #18) #28

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Task leases (fixes #18) #28

wants to merge 2 commits into from

Conversation

jerith
Copy link
Member

@jerith jerith commented Apr 20, 2017

No description provided.

@codecov
Copy link

codecov bot commented Apr 20, 2017

Codecov Report

Merging #28 into develop will increase coverage by 2.61%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #28      +/-   ##
===========================================
+ Coverage    60.55%   63.16%   +2.61%     
===========================================
  Files           21       23       +2     
  Lines         1397     1496      +99     
  Branches       157      160       +3     
===========================================
+ Hits           846      945      +99     
  Misses         511      511              
  Partials        40       40
Impacted Files Coverage Δ
xenserver/settings.py 95.23% <100%> (+0.11%) ⬆️
xenserver/task_lease.py 100% <100%> (ø)
xenserver/tests/test_task_lease.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a4d7d2...d2b2756. Read the comment docs.

has been acquired, `False` if an existing active lease is found.
"""
key = _key(task, args)
if Session.objects.filter(

Choose a reason for hiding this comment

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

Would get_or_create be simpler here? It returns a tuple of (object, created)

Copy link
Member Author

Choose a reason for hiding this comment

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

get_or_create doesn't let me check the expire_date in the query, but it does require me to pass it as a default for the creation step. While this makes the creation of a new object easier (it happens as part of get_or_create), it makes handling an existing but expired object harder because I'd have to check and update expire_date as appropriate. It's about the same amount of code as doing it this way, but the logic for checking if we properly got the lease is split up and less readable.

Choose a reason for hiding this comment

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

ah, yes.

@bearnard
Copy link

other than the get_or_create suggestion 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants