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

Enable remote draining capability of Drones via REST interface #260

Merged
merged 12 commits into from
Aug 30, 2022

Conversation

giffels
Copy link
Member

@giffels giffels commented Aug 17, 2022

This pull request enables the possibilty to drain drones in AvailableState via the REST interface in connection with #250.

  • Add get_resource_state method to SqliteRegistry
  • Add database property to Drone
  • Add database_state coroutine to Drone
  • Introduce a check_remote_draining function
  • Add this function to the pipeline of AvailableState
  • Add changelog for this feature

@giffels giffels added the enhancement New feature or request label Aug 17, 2022
@giffels giffels added this to the 0.7.0 - Release milestone Aug 17, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2022

Codecov Report

Merging #260 (4b997da) into master (cc13dfd) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #260      +/-   ##
==========================================
+ Coverage   98.81%   98.82%   +0.01%     
==========================================
  Files          54       54              
  Lines        2192     2215      +23     
==========================================
+ Hits         2166     2189      +23     
  Misses         26       26              
Impacted Files Coverage Δ
tardis/plugins/sqliteregistry.py 100.00% <100.00%> (ø)
tardis/resources/drone.py 98.96% <100.00%> (+0.15%) ⬆️
tardis/resources/dronestates.py 99.35% <100.00%> (+0.02%) ⬆️
tardis/resources/poolfactory.py 100.00% <100.00%> (ø)
tardis/utilities/utils.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@giffels giffels force-pushed the feature/add-remote-draining branch 3 times, most recently from 6873bd0 to 16cc4af Compare August 17, 2022 14:44
@giffels giffels requested review from a team, maxfischer2781 and RHofsaess and removed request for a team August 17, 2022 15:28
@giffels giffels force-pushed the feature/add-remote-draining branch from f8c4b1e to 91a4be4 Compare August 24, 2022 12:33
Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

I've only got some minor issues with language/code health. Please see the comments.

docs/source/changes/260.add_remote_drone_draining.yaml Outdated Show resolved Hide resolved
@@ -98,6 +99,15 @@ def machine_meta_data_translation(
raise


def str_to_state(resources):
Copy link
Member

Choose a reason for hiding this comment

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

I'm somewhat stumped by this function. Looking at the name, I would have assumed it to take a single state name and look that up. Perhaps a name such as load_states would work better? Do you see benefit in a single-name-lookup str2state in dronestates, or is that too trivial?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, load_statessounds much better. I have renamed it accordingly. I think an additional str2state function in dronestates is not reasonable, because the database plugin always returns a listing. In that case with a single element. So, I could either use load_states and get the first entry in the list or use a str2state function and put in the first entry from the database listing. So, it is "gehupft wie gesprungen". ;-)

tardis/utilities/utils.py Outdated Show resolved Hide resolved
@giffels giffels force-pushed the feature/add-remote-draining branch from e941638 to 2c1393e Compare August 29, 2022 13:56
@giffels giffels requested a review from maxfischer2781 August 29, 2022 14:21
maxfischer2781
maxfischer2781 previously approved these changes Aug 29, 2022
Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

[storm trooper move along.gif]

Copy link

@RHofsaess RHofsaess left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

[it's a trap approve.gif]

@giffels giffels merged commit 7808317 into MatterMiners:master Aug 30, 2022
@giffels giffels deleted the feature/add-remote-draining branch August 30, 2022 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants